[ovs-dev] [PATCH] bonding: Ignore updelay if there is no active slave.

Jesse Gross jesse at nicira.com
Fri Nov 6 20:42:52 UTC 2009



Ben Pfaff wrote:
> Why does this patch add static data (moving_active_iface in
> bond_enable_slave())?  Is it going to work if there is more than
> one bond on a bridge, or if there is more than one bond across
> all the bridges?
>   

It is essentially a recursion check - the flag may get set at entry to 
the function and is cleared when exiting.  Since we never process more 
than one bond at a time and the flag is not used outside the scope of 
handing a single bond there won't be any problems with multiple bonds or 
bridges.  Obviously it is also possible to pass parameters around that 
signify the same thing but it is just messier and less self-contained.

> Also, based on the comments in bond_enable_slave(), I think that
> this is an optimization.  But I'd rather avoid that optimization,
> because it adds more paths that will be harder to test and
> because this is not an important fast-path.
>   

It is an optimization but I think it is a important one.  It allows us 
to skip three things: some revalidation, choosing an active interface, 
and sending learning packets.  The first two are nice to skip but 
probably not that big a deal.  However, the sending of learning packets 
involves sending a packet for every entry in the learning table, which 
can potentially be quite a large number.  We have to do this once but 
without the check we will do it a second time.  This function is on the 
bond failover path, which means that while we are sitting in this code 
we are potentially losing packets, so I think it actually is worth a 
pretty simple optimization.

This doesn't introduce any new cases that we shouldn't already be 
testing.  This is the code path that lead to the original bug, so we 
should be testing it anyways.  There should be no impact on other 
situations.




More information about the dev mailing list