net/netvsc: use rte_eth_dev_set_mtu to set VF MTU

PATCH Applied [email protected] 4 participants 6 messages Jul 18, 2024 – Jul 21, 2024 Network drivers

lore

Share & tools

lore

[email protected]

author

2024-07-18 19:35 UTC #0 lore raw

PATCH 1/1 net/netvsc: use rte_eth_dev_set_mtu to set VF MTU

From: Stephen Hemminger <[email protected]>

The current code uses unnecessary locking to set VF MTU, resulting in deadlock on hot add/remove path. Fix this by using rte_eth_dev_set_mtu() to set VF MTU.

Signed-off-by Stephen Hemminger <[email protected]>

Fixes 45c83603087e ("net/netvsc: support MTU set")

Cc [email protected]

Signed-off-by Long Li <[email protected]>

---
 drivers/net/netvsc/hn_vf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/netvsc/hn_vf.c +2 -2

index 6b3d0eb0c8..b664beaa5d 100644
@@ -264,7 +264,7 @@ int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv)
 			goto exit;
 		}
 
-		ret = hn_vf_mtu_set(dev, dev->data->mtu);
+		ret = rte_eth_dev_set_mtu(port, dev->data->mtu);
 		if (ret) {
 			PMD_DRV_LOG(ERR, "Failed to set VF MTU");
 			goto exit;

@@ -796,7 +796,7 @@ int hn_vf_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 	rte_rwlock_read_lock(&hv->vf_lock);
 	vf_dev = hn_get_vf_dev(hv);
 	if (hv->vf_ctx.vf_vsc_switched && vf_dev)
-		ret = vf_dev->dev_ops->mtu_set(vf_dev, mtu);
+		ret = rte_eth_dev_set_mtu(vf_dev->data->port_id, mtu);
 	rte_rwlock_read_unlock(&hv->vf_lock);
 
 	return ret;

Signature · 1 line

2.43.0

Wei Hu

Reviewed-by on v1 1/1

2024-07-19 04:40 UTC #1 lore raw

Quoted 8 lines

Subject: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
From: Stephen Hemminger <[email protected]>
The current code uses unnecessary locking to set VF MTU, resulting in deadlock on hot add/remove path. Fix this by using rte_eth_dev_set_mtu() to set VF MTU.

Quoted 5 lines

Signed-off-by Stephen Hemminger <[email protected]>
Fixes 45c83603087e ("net/netvsc: support MTU set")
Cc [email protected]
Signed-off-by Long Li <[email protected]>

Reviewed-by Wei Hu <[email protected]>

Quoted 30 lines

---
 drivers/net/netvsc/hn_vf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/netvsc/hn_vf.c index renamed from drivers/net/netvsc/hn_vf.c +0 -0

6b3d0eb0c8..b664beaa5d 100644

drivers/net/netvsc/hn_vf.c +1 -1

@@ -264,7 +264,7 @@ int hn_vf_add(struct rte_eth_dev *dev, struct
 hn_data *hv)
 			goto exit;
 		}
 
-		ret = hn_vf_mtu_set(dev, dev->data->mtu);
+		ret = rte_eth_dev_set_mtu(port, dev->data->mtu);
 		if (ret) {
 			PMD_DRV_LOG(ERR, "Failed to set VF MTU");

drivers/net/netvsc/hn_vf.c +1 -1

 			goto exit;

@@ -796,7 +796,7 @@ int hn_vf_mtu_set(struct rte_eth_dev *dev, uint16_t
 mtu)
 	rte_rwlock_read_lock(&hv->vf_lock);
 	vf_dev = hn_get_vf_dev(hv);
 	if (hv->vf_ctx.vf_vsc_switched && vf_dev)
-		ret = vf_dev->dev_ops->mtu_set(vf_dev, mtu);
+		ret = rte_eth_dev_set_mtu(vf_dev->data->port_id, mtu);
 	rte_rwlock_read_unlock(&hv->vf_lock);

return ret;
--
2.43.0

Ferruh Yigit

2024-07-19 20:39 UTC #2 lore raw

In reply to [email protected]

Quoted 20 lines · On 7/18/2024 8:35 PM, [email protected] wrote:

From: Stephen Hemminger <[email protected]>
The current code uses unnecessary locking to set VF MTU, resulting in deadlock on hot add/remove path. Fix this by using rte_eth_dev_set_mtu() to set VF MTU.
Signed-off-by Stephen Hemminger <[email protected]>
Fixes 45c83603087e ("net/netvsc: support MTU set")
Cc [email protected]
Signed-off-by Long Li <[email protected]>
---
 drivers/net/netvsc/hn_vf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/netvsc/hn_vf.c +0 -0
index 6b3d0eb0c8..b664beaa5d 100644

drivers/net/netvsc/hn_vf.c int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv) · line 268

Quoted 6 lines

drivers/net/netvsc/hn_vf.c +1 -1

@@ -264,7 +264,7 @@ int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv)
 			goto exit;
 		}
 
-		ret = hn_vf_mtu_set(dev, dev->data->mtu);
+		ret = rte_eth_dev_set_mtu(port, dev->data->mtu);

As 'rte_eth_dev_set_mtu()' calls 'hn_vf_mtu_set()' in the call chain, won't it cause same problem?

Does it help to make unlocked version of 'hn_vf_mtu_set()':

```
_hn_vf_mtu_set()
	// set mtu without lock

hn_vf_mtu_set()
	lock()
	_hn_vf_mtu_set()
	unlock()
```

drivers/net/netvsc/hn_vf.c int hn_vf_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) · line 800

Quoted 10 lines

drivers/net/netvsc/hn_vf.c +0 -0

 		if (ret) {
 			PMD_DRV_LOG(ERR, "Failed to set VF MTU");

drivers/net/netvsc/hn_vf.c +1 -1

 			goto exit;

@@ -796,7 +796,7 @@ int hn_vf_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 	rte_rwlock_read_lock(&hv->vf_lock);
 	vf_dev = hn_get_vf_dev(hv);
 	if (hv->vf_ctx.vf_vsc_switched && vf_dev)
-		ret = vf_dev->dev_ops->mtu_set(vf_dev, mtu);
+		ret = rte_eth_dev_set_mtu(vf_dev->data->port_id, mtu);

Won't this cause a cyclic call:

rte_eth_dev_set_mtu()

	hn_dev_mtu_set()
		hn_vf_mtu_set()
			rte_eth_dev_set_mtu()

Quoted 3 lines

drivers/net/netvsc/hn_vf.c +0 -0
 	rte_rwlock_read_unlock(&hv->vf_lock);
 
return ret;

Stephen Hemminger

2024-07-19 21:21 UTC #3 lore raw

Quoted 6 lines · On Fri, 19 Jul 2024 21:39:04 +0100 Ferruh Yigit <[email protected]> wrote:

Quoted 6 lines

drivers/net/netvsc/hn_vf.c +0 -0

@@ -264,7 +264,7 @@ int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv)
 			goto exit;
 		}

Quoted 6 lines

drivers/net/netvsc/hn_vf.c int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv)
Quoted 3 lines
drivers/net/netvsc/hn_vf.c +1 -1
-		ret = hn_vf_mtu_set(dev, dev->data->mtu);
+		ret = rte_eth_dev_set_mtu(port, dev->data->mtu);
 
As 'rte_eth_dev_set_mtu()' calls 'hn_vf_mtu_set()' in the call chain, won't it cause same problem?

The port is the vf so it will call the set_mtu on the VF not the netvsc device so it is not in the call chain.

Quoted 11 lines

Does it help to make unlocked version of 'hn_vf_mtu_set()':
```
_hn_vf_mtu_set()
	// set mtu without lock

hn_vf_mtu_set()
	lock()
	_hn_vf_mtu_set()
	unlock()
```

That was original proposal, but using rte_eth_dev_set_mtu() on the VF port has more error checking.

Ferruh Yigit

2024-07-21 21:25 UTC #4 lore raw

Quoted 16 lines · Stephen Hemminger

Quoted 6 lines · On Fri, 19 Jul 2024 21:39:04 +0100 Ferruh Yigit <[email protected]> wrote:
Quoted 6 lines
drivers/net/netvsc/hn_vf.c +0 -0
@@ -264,7 +264,7 @@ int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv)
 			goto exit;
 		}
 
Quoted 6 lines
drivers/net/netvsc/hn_vf.c int hn_vf_add(struct rte_eth_dev *dev, struct hn_data *hv)
Quoted 3 lines
drivers/net/netvsc/hn_vf.c +1 -1
-		ret = hn_vf_mtu_set(dev, dev->data->mtu);
+		ret = rte_eth_dev_set_mtu(port, dev->data->mtu);
 
As 'rte_eth_dev_set_mtu()' calls 'hn_vf_mtu_set()' in the call chain, won't it cause same problem?

Quoted 3 lines

The port is the vf so it will call the set_mtu on the VF not the netvsc device so it is not in the call chain.

ack

Quoted 14 lines

Quoted 11 lines
Does it help to make unlocked version of 'hn_vf_mtu_set()':
```
_hn_vf_mtu_set()
	// set mtu without lock

hn_vf_mtu_set()
	lock()
	_hn_vf_mtu_set()
	unlock()
```
That was original proposal, but using rte_eth_dev_set_mtu() on the VF port has more error checking.

Ferruh Yigit

Applied

2024-07-21 21:25 UTC #5 lore raw

In reply to [email protected]

Quoted 6 lines · On 7/18/2024 8:35 PM, [email protected] wrote:

From: Stephen Hemminger <[email protected]>
The current code uses unnecessary locking to set VF MTU, resulting in deadlock on hot add/remove path. Fix this by using rte_eth_dev_set_mtu() to set VF MTU.

Quoted 6 lines

Fixes 45c83603087e ("net/netvsc: support MTU set")
Cc [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by Long Li <[email protected]>

Applied to dpdk-next-net/main, thanks.