diff mbox series

[ovs-dev] lacp: Add support to recognize LACP Marker RX PDUs.

Message ID 1573546139-6441-1-git-send-email-nitin.katiyar@ericsson.com
State Accepted
Commit 9248e103e4a0f7b531fc775f54ad01b33efe89e4
Headers show
Series [ovs-dev] lacp: Add support to recognize LACP Marker RX PDUs. | expand

Commit Message

Li,Rongqing via dev Nov. 12, 2019, 8:08 a.m. UTC
OVS does not support the LACP Marker protocol. Typically, ToR switches
send a LACP Marker PDU when restarting LACP negotiation following a link
flap or LACP timeout.

When a LACP Marker PDU is received, OVS logs the following warning and
drops the packet:
    “lacp(pmdXXX)|WARN|bond-prv: received an unparsable LACP PDU.”

As the above message is logged around the same time the link flap or
LACP down events are logged, it gives a misleading impression that the
reception of an unparsable LACP PDU is the reason for the LACP down
event.

The proposed patch does not add support for the LACP Marker protocol.
It simply recognizes LACP Marker packets, drops them and logs a clear
message indicating that a Marker packet was a received. A counter to
track the number of such packets received is also added.

Signed-off-by: Nitin katiyar <nitin.katiyar@ericsson.com>
---
 lib/lacp.c | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

Comments

Ben Pfaff Nov. 22, 2019, 1:30 a.m. UTC | #1
On Tue, Nov 12, 2019 at 09:08:59AM +0100, Nitin katiyar via dev wrote:
> OVS does not support the LACP Marker protocol. Typically, ToR switches
> send a LACP Marker PDU when restarting LACP negotiation following a link
> flap or LACP timeout.
> 
> When a LACP Marker PDU is received, OVS logs the following warning and
> drops the packet:
>     “lacp(pmdXXX)|WARN|bond-prv: received an unparsable LACP PDU.”
> 
> As the above message is logged around the same time the link flap or
> LACP down events are logged, it gives a misleading impression that the
> reception of an unparsable LACP PDU is the reason for the LACP down
> event.
> 
> The proposed patch does not add support for the LACP Marker protocol.
> It simply recognizes LACP Marker packets, drops them and logs a clear
> message indicating that a Marker packet was a received. A counter to
> track the number of such packets received is also added.
> 
> Signed-off-by: Nitin katiyar <nitin.katiyar@ericsson.com>

Thanks a lot!  I did not know about LACP Marker PDUs before.  I applied
this to master.
diff mbox series

Patch

diff --git a/lib/lacp.c b/lib/lacp.c
index 16c823b..705d88f 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -86,6 +86,12 @@  BUILD_ASSERT_DECL(LACP_PDU_LEN == sizeof(struct lacp_pdu));
 
 /* Implementation. */
 
+enum pdu_subtype {
+    SUBTYPE_UNUSED = 0,
+    SUBTYPE_LACP,       /* Link Aggregation Control Protocol. */
+    SUBTYPE_MARKER,     /* Link Aggregation Marker Protocol. */
+};
+
 enum slave_status {
     LACP_CURRENT,   /* Current State.  Partner up to date. */
     LACP_EXPIRED,   /* Expired State.  Partner out of date. */
@@ -130,6 +136,7 @@  struct slave {
 
     uint32_t count_rx_pdus;         /* dot3adAggPortStatsLACPDUsRx */
     uint32_t count_rx_pdus_bad;     /* dot3adAggPortStatsIllegalRx */
+    uint32_t count_rx_pdus_marker;  /* dot3adAggPortStatsMarkerPDUsRx */
     uint32_t count_tx_pdus;         /* dot3adAggPortStatsLACPDUsTx */
     uint32_t count_link_expired;    /* Num of times link expired */
     uint32_t count_link_defaulted;  /* Num of times link defaulted */
@@ -183,12 +190,13 @@  compose_lacp_pdu(const struct lacp_info *actor,
     pdu->collector_delay = htons(0);
 }
 
-/* Parses 'b' which represents a packet containing a LACP PDU.  This function
- * returns NULL if 'b' is malformed, or does not represent a LACP PDU format
+/* Parses 'p' which represents a packet containing a LACP PDU. This function
+ * returns NULL if 'p' is malformed, or does not represent a LACP PDU format
  * supported by OVS.  Otherwise, it returns a pointer to the lacp_pdu contained
- * within 'b'. */
+ * within 'p'. It also returns the subtype of PDU.*/
+
 static const struct lacp_pdu *
-parse_lacp_packet(const struct dp_packet *p)
+parse_lacp_packet(const struct dp_packet *p, enum pdu_subtype *subtype)
 {
     const struct lacp_pdu *pdu;
 
@@ -198,8 +206,13 @@  parse_lacp_packet(const struct dp_packet *p)
     if (pdu && pdu->subtype == 1
         && pdu->actor_type == 1 && pdu->actor_len == 20
         && pdu->partner_type == 2 && pdu->partner_len == 20) {
+        *subtype = SUBTYPE_LACP;
         return pdu;
-    } else {
+    } else if (pdu && pdu->subtype == SUBTYPE_MARKER) {
+        *subtype = SUBTYPE_MARKER;
+        return NULL;
+    } else{
+        *subtype = SUBTYPE_UNUSED;
         return NULL;
     }
 }
@@ -336,6 +349,7 @@  lacp_process_packet(struct lacp *lacp, const void *slave_,
     long long int tx_rate;
     struct slave *slave;
     bool lacp_may_enable = false;
+    enum pdu_subtype subtype;
 
     lacp_lock();
     slave = slave_lookup(lacp, slave_);
@@ -344,11 +358,20 @@  lacp_process_packet(struct lacp *lacp, const void *slave_,
     }
     slave->count_rx_pdus++;
 
-    pdu = parse_lacp_packet(packet);
-    if (!pdu) {
-        slave->count_rx_pdus_bad++;
-        VLOG_WARN_RL(&rl, "%s: received an unparsable LACP PDU.", lacp->name);
-        goto out;
+    pdu = parse_lacp_packet(packet, &subtype);
+    switch (subtype) {
+        case SUBTYPE_LACP:
+            break;
+        case SUBTYPE_MARKER:
+            slave->count_rx_pdus_marker++;
+            VLOG_DBG("%s: received a LACP marker PDU.", lacp->name);
+            goto out;
+        case SUBTYPE_UNUSED:
+        default:
+            slave->count_rx_pdus_bad++;
+            VLOG_WARN_RL(&rl, "%s: received an unparsable LACP PDU.",
+                         lacp->name);
+            goto out;
     }
 
     /* On some NICs L1 state reporting is slow. In case LACP packets are
@@ -367,7 +390,7 @@  lacp_process_packet(struct lacp *lacp, const void *slave_,
 
     slave->ntt_actor = pdu->partner;
 
-    /* Update our information about our partner if it's out of date.  This may
+    /* Update our information about our partner if it's out of date. This may
      * cause priorities to change so re-calculate attached status of all
      * slaves.  */
     if (memcmp(&slave->partner, &pdu->actor, sizeof pdu->actor)) {
@@ -1054,9 +1077,11 @@  lacp_print_stats(struct ds *ds, struct lacp *lacp) OVS_REQUIRES(mutex)
     for (i = 0; i < shash_count(&slave_shash); i++) {
         slave = sorted_slaves[i]->data;
         ds_put_format(ds, "\nslave: %s:\n", slave->name);
+        ds_put_format(ds, "  TX PDUs: %u\n", slave->count_tx_pdus);
         ds_put_format(ds, "  RX PDUs: %u\n", slave->count_rx_pdus);
         ds_put_format(ds, "  RX Bad PDUs: %u\n", slave->count_rx_pdus_bad);
-        ds_put_format(ds, "  TX PDUs: %u\n", slave->count_tx_pdus);
+        ds_put_format(ds, "  RX Marker Request PDUs: %u\n",
+                      slave->count_rx_pdus_marker);
         ds_put_format(ds, "  Link Expired: %u\n",
                       slave->count_link_expired);
         ds_put_format(ds, "  Link Defaulted: %u\n",