[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