Hi, Could someone take a look at this patch? We hit this issue (drops) on a customer setup and appreciate your comments/suggestions.
Thanx Manu On 17/05/18, 3:29 PM, "ovs-dev-boun...@openvswitch.org on behalf of Manohar Krishnappa Chidambaraswamy" <ovs-dev-boun...@openvswitch.org on behalf of manohar.krishnappa.chidambarasw...@ericsson.com> wrote: 2/2: Fix packet drops on LACP bond after link up Problem: ======== On certain Fortville NICs it has been observed that PHY UP detection can get delayed (sometimes up to 4-5 secs). When the driver fails to fetch PHY status as UP even though its actually UP, LACP packets can get exchanged and LACP slave brought UP. In such a case, the remote end would start sending traffic on that slave, but OVS drops it, as the bond-slave is not yet enabled due to PHY DOWN. Fix: ==== The main intention here is delay LACP negotiation until carrier (PHY) status is read as UP. 1. In port_run()/bundle_run(), cache the carrier status in "struct ofport_dpif"/"struct bond_slave". 2. When carrier state is DOWN, do not send any LACPDUs and drop any received LACPDUs. 3. When LACP state changes or LACPDU is received, trigger re-checking of carrier-state (in port_run()) by incrementing the connectivity sequence number to find out the true carrier state as fast as possible. Signed-off-by: Manohar K C <manohar.krishnappa.chidambarasw...@ericsson.com> CC: Jan Scheurich <jan.scheur...@ericsson.com> CC: Nitin Katiyar <nitin.kati...@ericsson.com> --- v1 1/2: Evaluate lacp may_enable inline in the datapath thread and check for pending "enable" in the bond admissibility check to avoid drops. lib/lacp.c | 51 +++++++++++++++++++++++++++++++++++++------- lib/lacp.h | 6 +++--- ofproto/bond.c | 30 ++++++++++++++++++++++++++ ofproto/bond.h | 3 +++ ofproto/ofproto-dpif-xlate.c | 4 +++- ofproto/ofproto-dpif.c | 11 +++++++--- tests/ofproto-dpif.at | 3 +++ 7 files changed, 93 insertions(+), 15 deletions(-) diff --git a/lib/lacp.c b/lib/lacp.c index 8353746..88621bc 100644 --- a/lib/lacp.c +++ b/lib/lacp.c @@ -33,6 +33,7 @@ #include "unixctl.h" #include "openvswitch/vlog.h" #include "util.h" +#include "ofproto/bond.h" VLOG_DEFINE_THIS_MODULE(lacp); @@ -148,7 +149,7 @@ static void slave_get_actor(struct slave *, struct lacp_info *actor) OVS_REQUIRES(mutex); static void slave_get_priority(struct slave *, struct lacp_info *priority) OVS_REQUIRES(mutex); -static bool slave_may_tx(const struct slave *) +static bool slave_may_tx(const void *bond, const struct slave *) OVS_REQUIRES(mutex); static struct slave *slave_lookup(const struct lacp *, const void *slave) OVS_REQUIRES(mutex); @@ -325,7 +326,7 @@ lacp_is_active(const struct lacp *lacp) OVS_EXCLUDED(mutex) * called on all packets received on 'slave_' with Ethernet Type ETH_TYPE_LACP. */ void -lacp_process_packet(struct lacp *lacp, const void *slave_, +lacp_process_packet(struct lacp *lacp, const void *bond, const void *slave_, const struct dp_packet *packet) OVS_EXCLUDED(mutex) { @@ -333,6 +334,7 @@ lacp_process_packet(struct lacp *lacp, const void *slave_, const struct lacp_pdu *pdu; long long int tx_rate; struct slave *slave; + bool carrier_up = false; lacp_lock(); slave = slave_lookup(lacp, slave_); @@ -348,6 +350,21 @@ lacp_process_packet(struct lacp *lacp, const void *slave_, goto out; } + /* + * On some NICs L1 state reporting is slow. In case LACP packets are + * received while carrier (L1) state is still down, drop the LACPDU and + * trigger re-checking of L1 state. + */ + carrier_up = bond_slave_get_carrier(bond, slave_); + if (!carrier_up) { + seq_change(connectivity_seq_get()); + + VLOG_INFO("carrier still DOWN - conn seq changed for slave %s, " + "dropping packet\n", slave->name); + + goto out; + } + slave->status = LACP_CURRENT; tx_rate = lacp->fast ? LACP_FAST_TIME_TX : LACP_SLOW_TIME_TX; timer_set_duration(&slave->rx, LACP_RX_MULTIPLIER * tx_rate); @@ -521,7 +538,8 @@ lacp_slave_is_current(const struct lacp *lacp, const void *slave_) /* This function should be called periodically to update 'lacp'. */ void -lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex) +lacp_run(struct lacp *lacp, const void *bond, + lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex) { struct slave *slave; @@ -551,7 +569,7 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex) HMAP_FOR_EACH (slave, node, &lacp->slaves) { struct lacp_info actor; - if (!slave_may_tx(slave)) { + if (!slave_may_tx(bond, slave)) { continue; } @@ -580,13 +598,13 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) OVS_EXCLUDED(mutex) /* Causes poll_block() to wake up when lacp_run() needs to be called again. */ void -lacp_wait(struct lacp *lacp) OVS_EXCLUDED(mutex) +lacp_wait(struct lacp *lacp, const void *bond) OVS_EXCLUDED(mutex) { struct slave *slave; lacp_lock(); HMAP_FOR_EACH (slave, node, &lacp->slaves) { - if (slave_may_tx(slave)) { + if (slave_may_tx(bond, slave)) { timer_wait(&slave->tx); } @@ -810,9 +828,26 @@ slave_get_priority(struct slave *slave, struct lacp_info *priority) } static bool -slave_may_tx(const struct slave *slave) OVS_REQUIRES(mutex) +slave_may_tx(const void *bond, const struct slave *slave) OVS_REQUIRES(mutex) { - return slave->lacp->active || slave->status != LACP_DEFAULTED; + bool carrier_up = true; + + if (bond) { + /* + * On some NICs L1 state reporting is slow. If carrier (L1) state is + * still down, do not send LACPDU and trigger re-checking of L1 state. + */ + carrier_up = bond_slave_get_carrier(bond, slave->aux); + if (!carrier_up) { + seq_change(connectivity_seq_get()); + + VLOG_INFO("carrier still DOWN - conn seq changed for slave %s, " + "avoiding tx\n", slave->name); + } + } + + return carrier_up && + (slave->lacp->active || slave->status != LACP_DEFAULTED); } static struct slave * diff --git a/lib/lacp.h b/lib/lacp.h index f35cff5..bf8f156 100644 --- a/lib/lacp.h +++ b/lib/lacp.h @@ -46,7 +46,7 @@ struct lacp *lacp_ref(const struct lacp *); void lacp_configure(struct lacp *, const struct lacp_settings *); bool lacp_is_active(const struct lacp *); -void lacp_process_packet(struct lacp *, const void *slave, +void lacp_process_packet(struct lacp *, const void *bond, const void *slave, const struct dp_packet *packet); enum lacp_status lacp_status(const struct lacp *); @@ -67,8 +67,8 @@ bool lacp_slave_is_current(const struct lacp *, const void *slave_); /* Callback function for lacp_run() for sending a LACP PDU. */ typedef void lacp_send_pdu(void *slave, const void *pdu, size_t pdu_size); -void lacp_run(struct lacp *, lacp_send_pdu *); -void lacp_wait(struct lacp *); +void lacp_run(struct lacp *, const void *bond, lacp_send_pdu *); +void lacp_wait(struct lacp *, const void *bond); struct lacp_slave_stats { /* id */ diff --git a/ofproto/bond.c b/ofproto/bond.c index 11d28e1..9c5448d 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -93,6 +93,7 @@ struct bond_slave { /* Link status. */ bool enabled; /* May be chosen for flows? */ bool may_enable; /* Client considers this slave bondable. */ + bool carrier_up; /* Carrier state from NIC port */ long long delay_expires; /* Time after which 'enabled' may change. */ /* Rebalancing info. Used only by bond_rebalance(). */ @@ -633,6 +634,35 @@ bond_slave_set_may_enable(struct bond *bond, void *slave_, bool may_enable) ovs_rwlock_unlock(&rwlock); } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 60f28e2..0dfa572 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -6322,6 +6322,9 @@ OVS_VSWITCHD_START([dnl other_config:lacp-port-priority=222 \ other_config:lacp-aggregation-key=3333 ]) +ovs-appctl netdev-dummy/set-admin-state p1 up +ovs-appctl netdev-dummy/set-admin-state p2 up + on_exit 'kill `cat test-sflow.pid`' AT_CHECK([ovstest test-sflow --log-file --detach --no-chdir --pidfile 0:127.0.0.1 > sflow.log], [0], [], [ignore]) AT_CAPTURE_FILE([sflow.log]) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev