DPDK patches and discussions
 help / color / mirror / Atom feed
* [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).