Skip to contentSkip to conversation

mlx5 RDMA LAG fixes

PATCH In review Leon Romanovsky 2 participants 7 messages Aug 16, 2023 – Aug 21, 2023

Subsystems (5)
Networking Network drivers InfiniBand Mellanox mlx5 core +1 more subsystems
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

cover mlx5 RDMA LAG fixes

From: Leon Romanovsky <[email protected]>

Hi,

These two not urgent fixes to mlx5 RDMA LAG logic.

Thanks

Mark Bloch (2):
RDMA/mlx5: Get upper device only if device is lagged RDMA/mlx5: Send correct port events

 drivers/infiniband/hw/mlx5/main.c             | 57 ++++++++++++++-----
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 29 ++++++++++
 include/linux/mlx5/driver.h                   |  2 +
 3 files changed, 75 insertions(+), 13 deletions(-)
Signature · 1 line
2.41.0
author
#1 lore raw

PATCH 1/2 RDMA/mlx5: Get upper device only if device is lagged

From: Mark Bloch <[email protected]>

If the RDMA device isn't in LAG mode there is no need to try to get the upper device.

Signed-off-by Mark Bloch <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/main.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/infiniband/hw/mlx5/main.c +15 -7
index f0b394ed7452..215d7b0add8f 100644
@@ -195,12 +195,18 @@ static int mlx5_netdev_event(struct notifier_block *this,
 	case NETDEV_CHANGE:
 	case NETDEV_UP:
 	case NETDEV_DOWN: {
-		struct net_device *lag_ndev = mlx5_lag_get_roce_netdev(mdev);
 		struct net_device *upper = NULL;
 
-		if (lag_ndev) {
-			upper = netdev_master_upper_dev_get(lag_ndev);
-			dev_put(lag_ndev);
+		if (ibdev->lag_active) {
+			struct net_device *lag_ndev;
+
+			lag_ndev = mlx5_lag_get_roce_netdev(mdev);
+			if (lag_ndev) {
+				upper = netdev_master_upper_dev_get(lag_ndev);
+				dev_put(lag_ndev);
+			} else {
+				goto done;
+			}
 		}
 
 		if (ibdev->is_rep)

@@ -254,9 +260,11 @@ static struct net_device *mlx5_ib_get_netdev(struct ib_device *device,
 	if (!mdev)
 		return NULL;
 
-	ndev = mlx5_lag_get_roce_netdev(mdev);
-	if (ndev)
-		goto out;
+	if (ibdev->lag_active) {
+		ndev = mlx5_lag_get_roce_netdev(mdev);
+		if (ndev)
+			goto out;
+	}
 
 	/* Ensure ndev does not disappear before we invoke dev_hold()
 	 */
Signature · 1 line
2.41.0
author
#6 lore raw

PATCH 2/2 RDMA/mlx5: Send correct port events

In reply to Leon Romanovsky

From: Mark Bloch <[email protected]>

When operating in switchdev mode and with an active LAG, the function mlx5_lag_get_roce_netdev() fails to return a valid net device as this function is designed specifically for RoCE LAGs.

Consequently, this issue resulted in the driver sending incorrect event reports. To address this, a new API is introduced to properly obtain the net device. Additionally, some code logic is cleaned up during this modification.

Signed-off-by Mark Bloch <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/main.c             | 39 +++++++++++++++----
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 29 ++++++++++++++
 include/linux/mlx5/driver.h                   |  2 +
 3 files changed, 62 insertions(+), 8 deletions(-)
drivers/infiniband/hw/mlx5/main.c +31 -8
index 215d7b0add8f..8b98200bd94c 100644
@@ -159,6 +159,29 @@ static struct mlx5_roce *mlx5_get_rep_roce(struct mlx5_ib_dev *dev,
 	return NULL;
 }
 
+static bool mlx5_netdev_send_event(struct mlx5_ib_dev *dev,
+				   struct net_device *ndev,
+				   struct net_device *upper,
+				   struct mlx5_roce *roce)
+{
+	if (!dev->ib_active)
+		return false;
+
+	/* Event is about our upper device */
+	if (upper == ndev)
+		return true;
+
+	/* RDMA device not in lag and not in switchdev */
+	if (!dev->is_rep && !upper && ndev == roce->netdev)
+		return true;
+
+	/* RDMA device in switchdev */
+	if (dev->is_rep && ndev == roce->netdev)
+		return true;
+
+	return false;
+}
+
 static int mlx5_netdev_event(struct notifier_block *this,
 			     unsigned long event, void *ptr)
 {

@@ -200,7 +223,7 @@ static int mlx5_netdev_event(struct notifier_block *this,
 		if (ibdev->lag_active) {
 			struct net_device *lag_ndev;
 
-			lag_ndev = mlx5_lag_get_roce_netdev(mdev);
+			lag_ndev = mlx5_lag_get_netdev(mdev);
 			if (lag_ndev) {
 				upper = netdev_master_upper_dev_get(lag_ndev);
 				dev_put(lag_ndev);

@@ -209,13 +232,13 @@ static int mlx5_netdev_event(struct notifier_block *this,
 			}
 		}
 
-		if (ibdev->is_rep)
+		if (ibdev->is_rep) {
 			roce = mlx5_get_rep_roce(ibdev, ndev, upper, &port_num);
-		if (!roce)
-			return NOTIFY_DONE;
-		if ((upper == ndev ||
-		     ((!upper || ibdev->is_rep) && ndev == roce->netdev)) &&
-		    ibdev->ib_active) {
+			if (!roce)
+				return NOTIFY_DONE;
+		}
+
+		if (mlx5_netdev_send_event(ibdev, ndev, upper, roce)) {
 			struct ib_event ibev = { };
 			enum ib_port_state port_state;
 

@@ -260,7 +283,7 @@ static struct net_device *mlx5_ib_get_netdev(struct ib_device *device,
 	if (!mdev)
 		return NULL;
 
-	if (ibdev->lag_active) {
+	if (!ibdev->is_rep && ibdev->lag_active) {
 		ndev = mlx5_lag_get_roce_netdev(mdev);
 		if (ndev)
 			goto out;
drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c +29 -0
index f0a074b2fcdf..83298e9addd3 100644
@@ -1498,6 +1498,35 @@ struct net_device *mlx5_lag_get_roce_netdev(struct mlx5_core_dev *dev)
 }
 EXPORT_SYMBOL(mlx5_lag_get_roce_netdev);
 
+struct net_device *mlx5_lag_get_netdev(struct mlx5_core_dev *dev)
+{
+	struct net_device *ndev = NULL;
+	struct mlx5_lag *ldev;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&lag_lock, flags);
+	ldev = mlx5_lag_dev(dev);
+
+	if (!(ldev && __mlx5_lag_is_active(ldev)))
+		goto unlock;
+
+	for (i = 0; i < ldev->ports; i++) {
+		if (ldev->pf[i].dev == dev) {
+			ndev = ldev->pf[i].netdev;
+			break;
+		}
+	}
+
+	if (ndev)
+		dev_hold(ndev);
+
+unlock:
+	spin_unlock_irqrestore(&lag_lock, flags);
+	return ndev;
+}
+EXPORT_SYMBOL(mlx5_lag_get_netdev);
+
 u8 mlx5_lag_get_slave_port(struct mlx5_core_dev *dev,
 			   struct net_device *slave)
 {
include/linux/mlx5/driver.h +2 -0
index 25d0528f9219..bc7e3a974f62 100644
@@ -1160,6 +1160,8 @@ bool mlx5_lag_is_master(struct mlx5_core_dev *dev);
 bool mlx5_lag_is_shared_fdb(struct mlx5_core_dev *dev);
 bool mlx5_lag_is_mpesw(struct mlx5_core_dev *dev);
 struct net_device *mlx5_lag_get_roce_netdev(struct mlx5_core_dev *dev);
+
+struct net_device *mlx5_lag_get_netdev(struct mlx5_core_dev *dev);
 u8 mlx5_lag_get_slave_port(struct mlx5_core_dev *dev,
 			   struct net_device *slave);
 int mlx5_lag_query_cong_counters(struct mlx5_core_dev *dev,
Signature · 1 line
2.41.0
maintainer
#2 lore raw

Re: [PATCH rdma-next 1/2] RDMA/mlx5: Get upper device only if device is lagged

In reply to Leon Romanovsky
Quoted 16 lines · Leon Romanovsky

From: Mark Bloch <[email protected]>

If the RDMA device isn't in LAG mode there is no need to try to get the upper device.

Signed-off-by Mark Bloch <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/main.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/infiniband/hw/mlx5/main.c +0 -0
index f0b394ed7452..215d7b0add8f 100644

drivers/infiniband/hw/mlx5/main.c static int mlx5_netdev_event(struct notifier_block *this, · line 200

Quoted 10 lines
drivers/infiniband/hw/mlx5/main.c +1 -4
@@ -195,12 +195,18 @@ static int mlx5_netdev_event(struct notifier_block *this,
 	case NETDEV_CHANGE:
 	case NETDEV_UP:
 	case NETDEV_DOWN: {
-		struct net_device *lag_ndev = mlx5_lag_get_roce_netdev(mdev);
 		struct net_device *upper = NULL;
 
-		if (lag_ndev) {
-			upper = netdev_master_upper_dev_get(lag_ndev);
-			dev_put(lag_ndev);
+		if (ibdev->lag_active) {

Needs locking to read lag_active

Jason

maintainer
#3 lore raw

Re: [PATCH rdma-next 1/2] RDMA/mlx5: Get upper device only if device is lagged

Quoted 23 lines · Jason Gunthorpe
Quoted 22 lines · Leon Romanovsky

From: Mark Bloch <[email protected]>

If the RDMA device isn't in LAG mode there is no need to try to get the upper device.

Signed-off-by Mark Bloch <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/main.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/infiniband/hw/mlx5/main.c +0 -1
index f0b394ed7452..215d7b0add8f 100644
@@ -195,12 +195,18 @@ static int mlx5_netdev_event(struct notifier_block *this,
 	case NETDEV_CHANGE:
 	case NETDEV_UP:
 	case NETDEV_DOWN: {
-		struct net_device *lag_ndev = mlx5_lag_get_roce_netdev(mdev);
 		struct net_device *upper = NULL;
Quoted 6 lines

drivers/infiniband/hw/mlx5/main.c static int mlx5_netdev_event(struct notifier_block *this, · line 200

Quoted 4 lines
drivers/infiniband/hw/mlx5/main.c +1 -3
-		if (lag_ndev) {
-			upper = netdev_master_upper_dev_get(lag_ndev);
-			dev_put(lag_ndev);
+		if (ibdev->lag_active) {

Needs locking to read lag_active

Specifically the use of the bitfield looks messed up.. If lag_active and some others were set only during probe it could be OK.

But mixing other stuff that is being written concurrently is not OK to do like this. (eg ib_active via a mlx5 notifier)

Jason

author
#4 lore raw

Re: [PATCH rdma-next 1/2] RDMA/mlx5: Get upper device only if device is lagged

Quoted 31 lines · Jason Gunthorpe
Quoted 23 lines · Jason Gunthorpe
Quoted 22 lines · Leon Romanovsky

From: Mark Bloch <[email protected]>

If the RDMA device isn't in LAG mode there is no need to try to get the upper device.

Signed-off-by Mark Bloch <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/main.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/infiniband/hw/mlx5/main.c +0 -1
index f0b394ed7452..215d7b0add8f 100644
@@ -195,12 +195,18 @@ static int mlx5_netdev_event(struct notifier_block *this,
 	case NETDEV_CHANGE:
 	case NETDEV_UP:
 	case NETDEV_DOWN: {
-		struct net_device *lag_ndev = mlx5_lag_get_roce_netdev(mdev);
 		struct net_device *upper = NULL;
Quoted 6 lines

drivers/infiniband/hw/mlx5/main.c static int mlx5_netdev_event(struct notifier_block *this, · line 200

Quoted 4 lines
drivers/infiniband/hw/mlx5/main.c +1 -3
-		if (lag_ndev) {
-			upper = netdev_master_upper_dev_get(lag_ndev);
-			dev_put(lag_ndev);
+		if (ibdev->lag_active) {

Needs locking to read lag_active

Quoted 2 lines

Specifically the use of the bitfield looks messed up.. If lag_active and some others were set only during probe it could be OK.

All fields except ib_active are static and set during probe.

Quoted 3 lines

But mixing other stuff that is being written concurrently is not OK to do like this. (eg ib_active via a mlx5 notifier)

What you are looking is the following change, did I get you right?

drivers/infiniband/hw/mlx5/mlx5_ib.h +1 -1
index 9d0c56b59ed2..ee73113717b2 100644
@@ -1094,7 +1094,7 @@ struct mlx5_ib_dev {
        /* serialize update of capability mask
         */
        struct mutex                    cap_mask_mutex;
-       u8                              ib_active:1;
+       bool                            ib_active;
        u8                              is_rep:1;
        u8                              lag_active:1;
        u8                              wc_support:1;
Quoted 2 lines

Jason

maintainer
#5 lore raw

Re: [PATCH rdma-next 1/2] RDMA/mlx5: Get upper device only if device is lagged

Quoted 48 lines · Leon Romanovsky
Quoted 31 lines · Jason Gunthorpe
Quoted 23 lines · Jason Gunthorpe
Quoted 22 lines · Leon Romanovsky

From: Mark Bloch <[email protected]>

If the RDMA device isn't in LAG mode there is no need to try to get the upper device.

Signed-off-by Mark Bloch <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/main.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/infiniband/hw/mlx5/main.c +0 -1
index f0b394ed7452..215d7b0add8f 100644
@@ -195,12 +195,18 @@ static int mlx5_netdev_event(struct notifier_block *this,
 	case NETDEV_CHANGE:
 	case NETDEV_UP:
 	case NETDEV_DOWN: {
-		struct net_device *lag_ndev = mlx5_lag_get_roce_netdev(mdev);
 		struct net_device *upper = NULL;
Quoted 6 lines

drivers/infiniband/hw/mlx5/main.c static int mlx5_netdev_event(struct notifier_block *this, · line 200

Quoted 4 lines
drivers/infiniband/hw/mlx5/main.c +1 -3
-		if (lag_ndev) {
-			upper = netdev_master_upper_dev_get(lag_ndev);
-			dev_put(lag_ndev);
+		if (ibdev->lag_active) {

Needs locking to read lag_active

Quoted 2 lines

Specifically the use of the bitfield looks messed up.. If lag_active and some others were set only during probe it could be OK.

All fields except ib_active are static and set during probe.

Quoted 3 lines

But mixing other stuff that is being written concurrently is not OK to do like this. (eg ib_active via a mlx5 notifier)

What you are looking is the following change, did I get you right?

drivers/infiniband/hw/mlx5/mlx5_ib.h +0 -0
index 9d0c56b59ed2..ee73113717b2 100644

drivers/infiniband/hw/mlx5/mlx5_ib.h struct mlx5_ib_dev { · line 1100

Quoted 8 lines
drivers/infiniband/hw/mlx5/mlx5_ib.h +1 -1
@@ -1094,7 +1094,7 @@ struct mlx5_ib_dev {
        /* serialize update of capability mask
         */
        struct mutex                    cap_mask_mutex;
-       u8                              ib_active:1;
+       bool                            ib_active;
        u8                              is_rep:1;
        u8                              lag_active:1;
        u8                              wc_support:1;

That helps, but it still needs some kind of concurrency management for ib_active

Jason