[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