[ovs-dev] [PATCH v4 1/2] Avoid packet drop on LACP bond after link up
Ben Pfaff
blp at ovn.org
Thu Feb 28 22:58:38 UTC 2019
On Thu, Feb 28, 2019 at 01:56:02PM +0000, Nitin Katiyar wrote:
> Problem:
> ========
> During port DOWN->UP of link (slave) in a LACP bond, after receiving the
> LACPDU with SYNC set for both actor and partner, the bond-slave remains
> "disabled" until OVS main thread runs LACP state machine and eventually
> "enables" the bond-slave. With this, we have observed delays in the order
> of 350ms and packets are dropped in OVS due to bond-admissibility check
> (packets received on slave in "disabled" state are dropped).
>
> Fix:
> ====
> When a LACPDU is received, evaluate whether LACP slave can be enabled
> (slave_may_enable()) and set LACP slave's may_enable from the datapath
> thread itself. When may_enable = TRUE, it means L1 state is UP and
> LACP-SYNC is done and it is waiting for the main thread to enable the
> slave. Relax the check in bond_check_admissibility() to check for both
> "enable" and "may_enable" of the LACP slave. This would avoid dropping
> of packets until the main thread enables the slave from bundle_run().
>
> Signed-off-by: Manohar Krishnappa Chidambaraswamy <manukc at gmail.com>
> Co-authored-by: Manohar Krishnappa Chidambaraswamy <manukc at gmail.com>
> Signed-off-by: Nitin Katiyar <nitin.katiyar at ericsson.com>
Thanks for the patch!
The debug message here in bond_check_admissibility() is not as
self-explanatory as we usually expect:
+ if (slave && (verdict != BV_ACCEPT)) {
+ VLOG_DBG_RL(&rl, "slave= %s actv-slave= %d may_enable %d enable %d "
+ "LACP %d verdict(A/D/M=0/1/2) %d\n", slave->name,
+ (bond->active_slave == slave), slave->may_enable,
+ slave->enabled, bond->lacp_status, verdict);
+ }
I'd recommend using yes/no or true/false for booleans in the output, and
explanatory names or phrases for the verdict instead of a number. The
spaces after the equals signs look kind of odd too. Usually we'd try to
express the overall logic message as a sentence, too, instead of just a
bunch of variables.
Otherwise this patch seems good to me.
Thanks,
Ben.
More information about the dev
mailing list