[Xorp-hackers] heavy CPU use for xorp_fea on startup

[Ben Greear]

Pavlin Radoslavov wrote:
> Ben Greear <greearb at candelatech.com> wrote:
>
>   
>> Pavlin Radoslavov wrote:
>>     
>>>>> In your implementation you need to be very careful that you capture
>>>>> all places inside IfTree that are related to the new pif_index to
>>>>> vif mapping.
>>>>>   
>>>>>       
>>>>>           
>>>> I was thinking on the way home:  Maybe just map if-index to if-name.  If 
>>>> the mapping
>>>> lookup fails, do a long slow linear lookup and if the object is found, 
>>>> add it to the if-index -> map.
>>>> If it succeeds, then lookup the vif by way of the existing hash, 
>>>> double-check the if-index is
>>>> correct (if not, do a slow lookup).
>>>>     
>>>>         
>>> I think this adds lots of complexity.
>>> Just a simple if-index to vif-name-pointer should be sufficient.
>>> Off the top of my head, you need to consider the following places
>>> inside IfTree that will affect the mapping:
>>>   
>>>       
>> Maybe I'm too paranoid..but any code can get an if, and then muck with it's
>> vifs.  The mapping is external to the if and vif objects, so it would be 
>> hard
>> to make sure no one can ever screw up a listing.
>>     
>
> I presume we are talking of a map that is internal to IfTree.
> Any addition/removal of a vif should use the add_vif() and
> remove_vif() methods so those methods need to take care of updating
> the internal map as well.
> Indeed, to be on the safe side, the IfTreeInterface::vifs()
> method that returns a non-const reference to the vifs map should
> be removed. This obviously requires refactoring in other parts
> of the FEA. For the sake of moving things forward we can ignore it
> for now, but a warning comment should be added to the VifMap& vifs()
> method.
>
>   
>> Also, an ifname can change while the ifindex remains the same, or
>> a new interface with the same name but a different ifindex can be
>> created.
>>     
>
> The ifname and the vifname cannot change, because they are the
> unique ID of an interface/vif. If they change, this will be
> translated into delete/add sequence to the the IfTree and that
> should take care of the ifindex update.
> The ifindex is also unique per interface/vif so there shouldn't be
> more than one interface/vif with the same ifindex (modulo ifindex of
> 0 which is invalid index).
>   
I need to re-read your email when I'm fresh....but in the meantime, here 
is a partial diff from
my tree with the hashing changes.  It compiles, but not tested yet.  We 
can remove the fallbacks
to the linear searches if/when we are sure all the boundary cases are 
resolved.  In the meantime,
I don't think it will really hurt anything, and there is enough logging 
to clue us in should we
still need to work on the hash...

Comments welcome.

Thanks,
Ben


-- 
Ben Greear <greearb at candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: fea_hash.patch
Type: text/x-patch
Size: 9618 bytes
Desc: not available
Url : http://mailman.ICSI.Berkeley.EDU/pipermail/xorp-hackers/attachments/20080304/2746e2f5/attachment-0001.bin