* [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
@ 2024-07-18 19:35 longli
2024-07-19 4:37 ` Wei Hu
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: longli @ 2024-07-18 19:35 UTC (permalink / raw)
To: Ferruh Yigit, Andrew Rybchenko, Wei Hu
Cc: dev, Stephen Hemminger, stable, Long Li
From: Stephen Hemminger <stephen@networkplumber.org>
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 <stephen@networkplumber.org>
Fixes: 45c83603087e ("net/netvsc: support MTU set")
Cc: stable@dpdk.org
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/net/netvsc/hn_vf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/netvsc/hn_vf.c b/drivers/net/netvsc/hn_vf.c
index 6b3d0eb0c8..b664beaa5d 100644
--- a/drivers/net/netvsc/hn_vf.c
+++ b/drivers/net/netvsc/hn_vf.c
@@ -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;
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
2024-07-18 19:35 [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU longli
@ 2024-07-19 4:37 ` Wei Hu
2024-07-19 20:39 ` Ferruh Yigit
2024-07-21 21:24 ` Ferruh Yigit
2 siblings, 0 replies; 6+ messages in thread
From: Wei Hu @ 2024-07-19 4:37 UTC (permalink / raw)
To: longli, Ferruh Yigit, Andrew Rybchenko
Cc: dev, Stephen Hemminger, stable, Long Li
> Subject: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
>
> From: Stephen Hemminger <stephen@networkplumber.org>
>
> 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 <stephen@networkplumber.org>
>
> Fixes: 45c83603087e ("net/netvsc: support MTU set")
> Cc: stable@dpdk.org
> Signed-off-by: Long Li <longli@microsoft.com>
Reviewed-by: Wei Hu <weh@microsoft.com>
> ---
> drivers/net/netvsc/hn_vf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/netvsc/hn_vf.c b/drivers/net/netvsc/hn_vf.c index
> 6b3d0eb0c8..b664beaa5d 100644
> --- a/drivers/net/netvsc/hn_vf.c
> +++ b/drivers/net/netvsc/hn_vf.c
> @@ -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;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
2024-07-18 19:35 [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU longli
2024-07-19 4:37 ` Wei Hu
@ 2024-07-19 20:39 ` Ferruh Yigit
2024-07-19 21:20 ` Stephen Hemminger
2024-07-21 21:24 ` Ferruh Yigit
2 siblings, 1 reply; 6+ messages in thread
From: Ferruh Yigit @ 2024-07-19 20:39 UTC (permalink / raw)
To: longli, Andrew Rybchenko, Wei Hu; +Cc: dev, Stephen Hemminger, stable, Long Li
On 7/18/2024 8:35 PM, longli@linuxonhyperv.com wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
>
> 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 <stephen@networkplumber.org>
>
> Fixes: 45c83603087e ("net/netvsc: support MTU set")
> Cc: stable@dpdk.org
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
> drivers/net/netvsc/hn_vf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/netvsc/hn_vf.c b/drivers/net/netvsc/hn_vf.c
> index 6b3d0eb0c8..b664beaa5d 100644
> --- a/drivers/net/netvsc/hn_vf.c
> +++ b/drivers/net/netvsc/hn_vf.c
> @@ -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()
```
> 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);
>
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()
> rte_rwlock_read_unlock(&hv->vf_lock);
>
> return ret;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
2024-07-19 20:39 ` Ferruh Yigit
@ 2024-07-19 21:20 ` Stephen Hemminger
2024-07-21 21:24 ` Ferruh Yigit
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2024-07-19 21:20 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: longli, Andrew Rybchenko, Wei Hu, dev, stable, Long Li
On Fri, 19 Jul 2024 21:39:04 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> > --- a/drivers/net/netvsc/hn_vf.c
> > +++ b/drivers/net/netvsc/hn_vf.c
> > @@ -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?
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.
>
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
2024-07-19 21:20 ` Stephen Hemminger
@ 2024-07-21 21:24 ` Ferruh Yigit
0 siblings, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2024-07-21 21:24 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: longli, Andrew Rybchenko, Wei Hu, dev, stable, Long Li
On 7/19/2024 10:20 PM, Stephen Hemminger wrote:
> On Fri, 19 Jul 2024 21:39:04 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
>>> --- a/drivers/net/netvsc/hn_vf.c
>>> +++ b/drivers/net/netvsc/hn_vf.c
>>> @@ -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?
>
> 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
>>
>> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU
2024-07-18 19:35 [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU longli
2024-07-19 4:37 ` Wei Hu
2024-07-19 20:39 ` Ferruh Yigit
@ 2024-07-21 21:24 ` Ferruh Yigit
2 siblings, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2024-07-21 21:24 UTC (permalink / raw)
To: longli, Andrew Rybchenko, Wei Hu; +Cc: dev, Stephen Hemminger, stable, Long Li
On 7/18/2024 8:35 PM, longli@linuxonhyperv.com wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
>
> 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.
>
> Fixes: 45c83603087e ("net/netvsc: support MTU set")
> Cc: stable@dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Long Li <longli@microsoft.com>
>
Applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-21 21:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-18 19:35 [PATCH] net/netvsc: use rte_eth_dev_set_mtu to set VF MTU longli
2024-07-19 4:37 ` Wei Hu
2024-07-19 20:39 ` Ferruh Yigit
2024-07-19 21:20 ` Stephen Hemminger
2024-07-21 21:24 ` Ferruh Yigit
2024-07-21 21:24 ` Ferruh Yigit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).