[PATCH] ethdev: fix dev close in secondary process

[PATCH] ethdev: fix dev close in secondary process

[Min Hu (Connor)]

From: Min Hu <humin29@huawei.com>

Shared memory like port private resources should only be reserved
by primary process. Secondary process should not start dev, and
the state of 'dev_started' is only meaningful to primary process.
While secondary process need to close dev to release process private
resources.

This patch limited the scope of 'dev_started'.

Fixes: febc855b358e ("ethdev: forbid closing started device")
Cc: stable@dpdk.org

Signed-off-by: Min Hu <humin29@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 09abee6345..f34c6580a4 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1574,7 +1574,8 @@ rte_eth_dev_close(uint16_t port_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_started) {
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
+			dev->data->dev_started) {
 		RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
 			       port_id);
 		return -EINVAL;
-- 
2.33.0

Re: [PATCH] ethdev: fix dev close in secondary process

[Andrew Rybchenko]

On 5/27/22 05:35, Min Hu (Connor) wrote:
> From: Min Hu <humin29@huawei.com>
> 
> Shared memory like port private resources should only be reserved
> by primary process. Secondary process should not start dev, and
> the state of 'dev_started' is only meaningful to primary process.
> While secondary process need to close dev to release process private
> resources.
> 
> This patch limited the scope of 'dev_started'.

I agree with the patch since secondary process should not be
obliged to wait for device stop before closing ethdev. In any
case closing ethdev in secondary process should do nothing
harmful to the primary process.

However, the patch description pretends to limit dev_started
scope for secondary processes in general. It is wrong since
secondary processes need the information and that's why it is
stored in shared memory.

> 
> Fixes: febc855b358e ("ethdev: forbid closing started device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu <humin29@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 09abee6345..f34c6580a4 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1574,7 +1574,8 @@ rte_eth_dev_close(uint16_t port_id)
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>   	dev = &rte_eth_devices[port_id];
>   
> -	if (dev->data->dev_started) {
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
> +			dev->data->dev_started) {
>   		RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
>   			       port_id);
>   		return -EINVAL;

Re: [PATCH] ethdev: fix dev close in secondary process

[Stephen Hemminger]

On Tue, 31 May 2022 20:08:55 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:

> On 5/27/22 05:35, Min Hu (Connor) wrote:
> > From: Min Hu <humin29@huawei.com>
> > 
> > Shared memory like port private resources should only be reserved
> > by primary process. Secondary process should not start dev, and
> > the state of 'dev_started' is only meaningful to primary process.
> > While secondary process need to close dev to release process private
> > resources.
> > 
> > This patch limited the scope of 'dev_started'.  
> 
> I agree with the patch since secondary process should not be
> obliged to wait for device stop before closing ethdev. In any
> case closing ethdev in secondary process should do nothing
> harmful to the primary process.
> 
> However, the patch description pretends to limit dev_started
> scope for secondary processes in general. It is wrong since
> secondary processes need the information and that's why it is
> stored in shared memory.
> 
> > 
> > Fixes: febc855b358e ("ethdev: forbid closing started device")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Min Hu <humin29@huawei.com>
> > ---

Also secondary processes are used differently by different application models.

Some applications only use secondary process for information.
But some have a primary process that only inits DPDK and do everything
in a secondary process.

Re: [PATCH] ethdev: fix dev close in secondary process

[Min Hu (Connor)]

Hi,Andrew, Stephen ,
	I fixed the comment, thanks for your comment.

在 2022/6/1 1:40, Stephen Hemminger 写道:
> On Tue, 31 May 2022 20:08:55 +0300
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
> 
>> On 5/27/22 05:35, Min Hu (Connor) wrote:
>>> From: Min Hu <humin29@huawei.com>
>>>
>>> Shared memory like port private resources should only be reserved
>>> by primary process. Secondary process should not start dev, and
>>> the state of 'dev_started' is only meaningful to primary process.
>>> While secondary process need to close dev to release process private
>>> resources.
>>>
>>> This patch limited the scope of 'dev_started'.
>>
>> I agree with the patch since secondary process should not be
>> obliged to wait for device stop before closing ethdev. In any
>> case closing ethdev in secondary process should do nothing
>> harmful to the primary process.
>>
>> However, the patch description pretends to limit dev_started
>> scope for secondary processes in general. It is wrong since
>> secondary processes need the information and that's why it is
>> stored in shared memory.
>>
>>>
>>> Fixes: febc855b358e ("ethdev: forbid closing started device")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Min Hu <humin29@huawei.com>
>>> ---
> 
> Also secondary processes are used differently by different application models.
> 
> Some applications only use secondary process for information.
> But some have a primary process that only inits DPDK and do everything
> in a secondary process.
> .
> 

[PATCH v2] ethdev: fix dev close in secondary process

[Min Hu (Connor)]

From: Min Hu <humin29@huawei.com>

Secondary process need to close dev to release process private resources.
But secondary process should not be obliged to wait for device stop before
closing ethdev.

This patch fixed it.

Fixes: febc855b358e ("ethdev: forbid closing started device")
Cc: stable@dpdk.org

Signed-off-by: Min Hu <humin29@huawei.com>
---
v2:
* fixed comment.
---
 lib/ethdev/rte_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 09abee6345..f34c6580a4 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1574,7 +1574,8 @@ rte_eth_dev_close(uint16_t port_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_started) {
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
+			dev->data->dev_started) {
 		RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
 			       port_id);
 		return -EINVAL;
-- 
2.33.0

Re: [PATCH v2] ethdev: fix dev close in secondary process

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

On Tue, May 31, 2022 at 6:32 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> From: Min Hu <humin29@huawei.com>
>
> Secondary process need to close dev to release process private resources.
> But secondary process should not be obliged to wait for device stop before
> closing ethdev.
>
> This patch fixed it.
>
> Fixes: febc855b358e ("ethdev: forbid closing started device")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu <humin29@huawei.com>
> ---
> v2:
> * fixed comment.
> ---
>  lib/ethdev/rte_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 09abee6345..f34c6580a4 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1574,7 +1574,8 @@ rte_eth_dev_close(uint16_t port_id)
>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>         dev = &rte_eth_devices[port_id];
>
> -       if (dev->data->dev_started) {
It will be good to add the comment as a part of the code as well.
That way someone in future won't have to look at the commit log to
understand the behavior.

> +       if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
> +                       dev->data->dev_started) {
>                 RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
>                                port_id);
>                 return -EINVAL;
> --
> 2.33.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

Re: [PATCH v2] ethdev: fix dev close in secondary process

[Min Hu (Connor)]

Hi, Ajit
v3 has been sent, thanks

在 2022/6/1 10:01, Ajit Khaparde 写道:
> On Tue, May 31, 2022 at 6:32 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>>
>> From: Min Hu <humin29@huawei.com>
>>
>> Secondary process need to close dev to release process private resources.
>> But secondary process should not be obliged to wait for device stop before
>> closing ethdev.
>>
>> This patch fixed it.
>>
>> Fixes: febc855b358e ("ethdev: forbid closing started device")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu <humin29@huawei.com>
>> ---
>> v2:
>> * fixed comment.
>> ---
>>   lib/ethdev/rte_ethdev.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 09abee6345..f34c6580a4 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1574,7 +1574,8 @@ rte_eth_dev_close(uint16_t port_id)
>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>          dev = &rte_eth_devices[port_id];
>>
>> -       if (dev->data->dev_started) {
> It will be good to add the comment as a part of the code as well.
> That way someone in future won't have to look at the commit log to
> understand the behavior.
> 
>> +       if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
>> +                       dev->data->dev_started) {
>>                  RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
>>                                 port_id);
>>                  return -EINVAL;
>> --
>> 2.33.0
>>

Re: [PATCH v2] ethdev: fix dev close in secondary process

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 123 bytes --]

On Tue, May 31, 2022 at 8:18 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> Hi, Ajit
> v3 has been sent, thanks
Thanks

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

[PATCH v3] ethdev: fix dev close in secondary process

[Min Hu (Connor)]

From: Min Hu <humin29@huawei.com>

Secondary process need to close dev to release process private resources.
But secondary process should not be obliged to wait for device stop before
closing ethdev.

This patch fixed it.

Fixes: febc855b358e ("ethdev: forbid closing started device")
Cc: stable@dpdk.org

Signed-off-by: Min Hu <humin29@huawei.com>
---
v3:
* add comment in the code.

v2:
* fixed comment.
---
 lib/ethdev/rte_ethdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 09abee6345..902ff96889 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1574,7 +1574,13 @@ rte_eth_dev_close(uint16_t port_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_started) {
+	/*
+	 * Secondary process need to close dev to release process private
+	 * resources. But secondary process should not be obliged to wait
+	 * for device stop before closing ethdev.
+	 */
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
+			dev->data->dev_started) {
 		RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
 			       port_id);
 		return -EINVAL;
-- 
2.33.0

Re: [PATCH v3] ethdev: fix dev close in secondary process

[Andrew Rybchenko]

On 6/1/22 06:15, Min Hu (Connor) wrote:
> From: Min Hu <humin29@huawei.com>
> 
> Secondary process need to close dev to release process private resources.
> But secondary process should not be obliged to wait for device stop before
> closing ethdev.
> 
> This patch fixed it.
> 
> Fixes: febc855b358e ("ethdev: forbid closing started device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu <humin29@huawei.com>

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