Project

General

Profile

Actions

Bug #14814

open

mlxcx panic tearing down 4 port aggr

Added by Garrett D'Amore 3 months ago. Updated 3 months ago.

Status:
In Progress
Priority:
Normal
Category:
driver - device drivers
Start date:
Due date:
% Done:

80%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

Description

We observed a panic trying to remove an interface from a mellanox ethernet driver that had failed to completely unconfigure its MAC address. The failure mode was an attempt to rebuild the link, but this resulted in a panic in mac_add_vlan.

If we're failing to unconfigure, then attempting to restore the configuration is probably a doomed effort anyway -- any assumptions we might be tempted to make about the state of the underlying hardware are likely flawed. We should log a warning, and just carry on, because the safest thing to do at this point is to decommission the interface (which is what we are trying to do anyway.)

We have a fix for this at RackTop, which we have been able to validate.

Here's the panic message we saw:

NOTICE: mlxcx1 link up, 25000 Mbps, full duplex
WARNING: aggr1015: failed to remove a MAC HW filters because of error 0x5
panic[cpu9]/thread=fffffdfa11ef44c0:
assertion failed: map->ma_nusers 0 (0x1 0x0), file: ../../common/io/mac/mac.c, line: 5846

Now it turns out that error 5 is returned by the mellanox driver when trying to unconfigure a MAC address that was never properly configured.

Why did this happen? Well, in our case it was probably because we had configured too many addresses.

Actions #1

Updated by Garrett D'Amore 3 months ago

Here's our code for this at RackTop, which has been shipping for nearly a year now.

commit 325177887786145808adff04a8a58b9be1338f1b
Author: Garrett D'Amore <garrett@damore.org>
Date:   Mon Aug 16 12:44:18 2021 -0700

    BSR-7422 Another 4 port mlxcx aggr related panic - after adding and remove interfaces

    Do not attempt to "repair" a device that has failed to properly
    tear down it's mac or vlan configuration -- just report it in the
    log and carry, since we can't know the state of the hardware at
    this point anyway.  This should resolve the dangling reference
    caused by the failure at teardown.

diff --git a/usr/src/uts/common/io/mac/mac.c b/usr/src/uts/common/io/mac/mac.c
index 41d3ee5fe1..75684f6a20 100644
--- a/usr/src/uts/common/io/mac/mac.c
+++ b/usr/src/uts/common/io/mac/mac.c
@@ -23,7 +23,7 @@
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright 2020 Joyent, Inc.
  * Copyright 2015 Garrett D'Amore <garrett@damore.org>
- * Copyright 2020 RackTop Systems, Inc.
+ * Copyright 2021 RackTop Systems, Inc.
  */

 /*
@@ -5589,13 +5589,13 @@ bail:
     return (err);
 }

-int
+void
 mac_remove_macaddr_vlan(mac_address_t *map, uint16_t vid)
 {
     mac_vlan_t    *mvp;
     mac_impl_t    *mip = map->ma_mip;
     mac_group_t    *group = map->ma_group;
-    int        err = 0;
+    int        err;

     ASSERT(MAC_PERIM_HELD((mac_handle_t)mip));
     VERIFY3P(map, ==, mac_find_macaddr(mip, map->ma_addr));
@@ -5610,8 +5610,16 @@ mac_remove_macaddr_vlan(mac_address_t *map, uint16_t vid)

     if (MAC_GROUP_HW_VLAN(group) &&
         map->ma_type == MAC_ADDRESS_TYPE_UNICAST_CLASSIFIED &&
-        ((err = mac_group_remvlan(group, vid)) != 0))
-        return (err);
+        ((err = mac_group_remvlan(group, vid)) != 0)) {
+        /*
+         * If the hardware driver fails to remove the mac,
+         * there isn't much we can do to repair it.  We log
+         * it, but then carry on although the hardware will
+         * potentially be in an indeterminate state.
+         */
+        cmn_err(CE_WARN, "Failed to remove VLAN %u from group %d on " 
+            "MAC %s: %d.", vid, group->mrg_index, mip->mi_name, err);
+    }

     if (mvp != NULL)
         mac_rem_vlan(map, mvp);
@@ -5622,7 +5630,7 @@ mac_remove_macaddr_vlan(mac_address_t *map, uint16_t vid)
      */
     map->ma_nusers--;
     if (map->ma_nusers > 0)
-        return (0);
+        return;

     VERIFY3S(map->ma_nusers, ==, 0);

@@ -5638,68 +5646,39 @@ mac_remove_macaddr_vlan(mac_address_t *map, uint16_t vid)
          * don't advertise RINGS capability.
          */
         if (group == NULL)
-            return (0);
+            return;

         if ((err = mac_group_remmac(group, map->ma_addr)) != 0) {
-            if (vid == VLAN_ID_NONE)
-                map->ma_untagged = B_TRUE;
-            else
-                (void) mac_add_vlan(map, vid);
-
             /*
-             * If we fail to remove the MAC address HW
-             * filter but then also fail to re-add the
-             * VLAN HW filter then we are in a busted
-             * state. We do our best by logging a warning
-             * and returning the original 'err' that got
-             * us here. At this point, traffic for this
-             * address + VLAN combination will be dropped
-             * until the user reboots the system. In the
-             * future, it would be nice to have a system
-             * that can compare the state of expected
-             * classification according to mac to the
-             * actual state of the provider, and report
-             * and fix any inconsistencies.
+             * If the hardware fails to remove the mac address,
+             * we can't really assume anything about the state
+             * of the hardware.  Most likely we are in the process
+             * of tearing down the mac handle anyway, so log it
+             * and carry on.
              */
-            if (MAC_GROUP_HW_VLAN(group)) {
-                int err2;
-
-                err2 = mac_group_addvlan(group, vid);
-                if (err2 != 0) {
-                    cmn_err(CE_WARN, "Failed to readd VLAN" 
-                        " %u to group %d on MAC %s: %d.",
-                        vid, group->mrg_index, mip->mi_name,
-                        err2);
-                }
-            }
-
-            map->ma_nusers = 1;
-            return (err);
+            cmn_err(CE_WARN, "Failed removing group %d MAC %s: %d",
+                group->mrg_index, mip->mi_name, err);
         }

         map->ma_group = NULL;
         break;
     case MAC_ADDRESS_TYPE_UNICAST_PROMISC:
-        err = i_mac_promisc_set(mip, B_FALSE);
+        if ((err = i_mac_promisc_set(mip, B_FALSE)) != 0) {
+            cmn_err(CE_WARN, "Failed disabling promiscuous %s: %d",
+                mip->mi_name, err);
+        }
         break;
     default:
         panic("Unexpected ma_type 0x%x, file: %s, line %d",
             map->ma_type, __FILE__, __LINE__);
     }

-    if (err != 0) {
-        map->ma_nusers = 1;
-        return (err);
-    }
-
     /*
      * We created MAC address for the primary one at registration, so we
      * won't free it here. mac_fini_macaddr() will take care of it.
      */
     if (bcmp(map->ma_addr, mip->mi_addr, map->ma_len) != 0)
         mac_free_macaddr(map);
-
-    return (0);
 }

 /*
@@ -7324,12 +7303,7 @@ mac_rx_move_macaddr(mac_client_impl_t *mcip, mac_group_t *fgrp,
     if (mac_check_macaddr_shared(mcip->mci_unicast))
         return (EINVAL);

-    err = mac_remove_macaddr_vlan(mcip->mci_unicast, vid);
-
-    if (err != 0) {
-        mac_rx_client_restart((mac_client_handle_t)mcip);
-        return (err);
-    }
+    mac_remove_macaddr_vlan(mcip->mci_unicast, vid);

     /*
      * If this isn't the primary MAC address then the
diff --git a/usr/src/uts/common/io/mac/mac_datapath_setup.c b/usr/src/uts/common/io/mac/mac_datapath_setup.c
index 55b9feb19c..6a68bde4a2 100644
--- a/usr/src/uts/common/io/mac/mac_datapath_setup.c
+++ b/usr/src/uts/common/io/mac/mac_datapath_setup.c
@@ -21,7 +21,7 @@
 /*
  * Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright 2018 Joyent, Inc.
- * Copyright 2020 RackTop Systems, Inc.
+ * Copyright 2021 RackTop Systems, Inc.
  */

 #include <sys/types.h>
@@ -3175,16 +3175,7 @@ mac_datapath_teardown(mac_client_impl_t *mcip, flow_entry_t *flent,
          * any hardware filters assigned to this client.
          */
         if (mcip->mci_unicast != NULL) {
-            int err;
-
-            err = mac_remove_macaddr_vlan(mcip->mci_unicast, vid);
-
-            if (err != 0) {
-                cmn_err(CE_WARN, "%s: failed to remove a MAC HW" 
-                    " filters because of error 0x%x",
-                    mip->mi_name, err);
-            }
-
+            mac_remove_macaddr_vlan(mcip->mci_unicast, vid);
             mcip->mci_unicast = NULL;
         }

diff --git a/usr/src/uts/common/sys/mac_impl.h b/usr/src/uts/common/sys/mac_impl.h
index da645ad382..c7f5cb59b8 100644
--- a/usr/src/uts/common/sys/mac_impl.h
+++ b/usr/src/uts/common/sys/mac_impl.h
@@ -21,6 +21,7 @@
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright 2019 Joyent, Inc.
+ * Copyright 2021 RackTop Systems, Inc.
  */

 #ifndef    _SYS_MAC_IMPL_H
@@ -837,7 +838,7 @@ extern int mac_start_ring(mac_ring_t *);
 extern void mac_stop_ring(mac_ring_t *);
 extern int mac_add_macaddr_vlan(mac_impl_t *, mac_group_t *, uint8_t *,
     uint16_t, boolean_t);
-extern int mac_remove_macaddr_vlan(mac_address_t *, uint16_t);
+extern void mac_remove_macaddr_vlan(mac_address_t *, uint16_t);

 extern void mac_set_group_state(mac_group_t *, mac_group_state_t);
 extern void mac_group_add_client(mac_group_t *, mac_client_impl_t *);
Actions #2

Updated by Garrett D'Amore 3 months ago

Note that the above fix was verified by QA (we had a reproducible test case which failed before this, and no longer fails after it), and the code has been through a lot of additional test and use since then ... the underlying mellanox driver is our preferred driver for high performance configurations.

Actions #3

Updated by Electric Monk 3 months ago

  • Gerrit CR set to 2242
Actions

Also available in: Atom PDF