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
