diff mbox series

net/netvsc: use rte_eth_dev_set_mtu to set VF MTU

Message ID 1721331316-8821-1-git-send-email-longli@linuxonhyperv.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/netvsc: use rte_eth_dev_set_mtu to set VF MTU |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build fail github build: failed
ci/intel-Functional success Functional PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Long Li July 18, 2024, 7:35 p.m. UTC 
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(-)

Comments

Wei Hu July 19, 2024, 4:37 a.m. UTC | #1 
> 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
Ferruh Yigit July 19, 2024, 8:39 p.m. UTC | #2 
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;
Stephen Hemminger July 19, 2024, 9:20 p.m. UTC | #3 
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.
Ferruh Yigit July 21, 2024, 9:24 p.m. UTC | #4 
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.
Ferruh Yigit July 21, 2024, 9:24 p.m. UTC | #5 
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.
diff mbox series

Patch

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;