diff mbox series

[V2] ethdev: fix eth device released repeatedly

Message ID 20211012113934.23611-1-lihuisong@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [V2] ethdev: fix eth device released repeatedly |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

lihuisong (C) Oct. 12, 2021, 11:39 a.m. UTC 
  The rte_eth_dev_pci_generic_remove() will be called to detach an Ethernet
device when App calls rte_dev_remove() to detach a pci device. In addition,
the rte_eth_dev_close() can also detach an Ethernet device.
In secondary process, if App first calls rte_eth_dev_close() and then calls
rte_dev_remove(), because rte_eth_dev_close() doesn't clear "eth_dev->data"
, the address of the released Ethernet device can still be found by device
name. As a result, the Ethernet device will be released repeatedly in this
case. The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
calling rte_eth_dev_close(). Use this state to avoid this problem.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
v1 -> v2:                                                                  
  * fix the commit log description.                                        
                                                                           
RFC -> v1:                                                                 
  * fix commit log and add a judgment for secondary process.
---
 lib/ethdev/ethdev_pci.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Thomas Monjalon Oct. 12, 2021, 3:33 p.m. UTC | #1 
12/10/2021 13:39, Huisong Li:
> The rte_eth_dev_pci_generic_remove() will be called to detach an Ethernet
> device when App calls rte_dev_remove() to detach a pci device. In addition,
> the rte_eth_dev_close() can also detach an Ethernet device.
> In secondary process, if App first calls rte_eth_dev_close() and then calls
> rte_dev_remove(), because rte_eth_dev_close() doesn't clear "eth_dev->data"

It would be clearer if you start this sentence with:
"In secondary process, rte_eth_dev_close() doesn't clear eth_dev->data."
Then you can explain that if calling rte_dev_remove() after rte_eth_dev_close(),
etc...

> , the address of the released Ethernet device can still be found by device
> name. As a result, the Ethernet device will be released repeatedly in this
> case. The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
> calling rte_eth_dev_close(). Use this state to avoid this problem.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> +	/*
> +	 * In secondary process, if applications first call rte_eth_dev_close()
> +	 * and then call this interface, because rte_eth_dev_close() doesn't
> +	 * clear eth_dev->data, the address of the released Ethernet device can
> +	 * still be found by device name. As a result, the Ethernet device will
> +	 * be released repeatedly in this case.
> +	 * The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
> +	 * calling rte_eth_dev_close(). Use this state to avoid this problem.

This is a comment for the commit log.
Inside the code, we should be more to the point.
I suggest this comment:
/* A released port can be found by its name in shared memory. */

> +	 */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY &&

Better to directly compare with RTE_PROC_SECONDARY

> +	    eth_dev->state == RTE_ETH_DEV_UNUSED) {
> +		RTE_ETHDEV_LOG(INFO, "The ethdev port has been released.");

Not sure we need any log here.

> +		return 0;
> +	}
lihuisong (C) Oct. 14, 2021, 3:50 a.m. UTC | #2 
在 2021/10/12 23:33, Thomas Monjalon 写道:
> 12/10/2021 13:39, Huisong Li:
>> The rte_eth_dev_pci_generic_remove() will be called to detach an Ethernet
>> device when App calls rte_dev_remove() to detach a pci device. In addition,
>> the rte_eth_dev_close() can also detach an Ethernet device.
>> In secondary process, if App first calls rte_eth_dev_close() and then calls
>> rte_dev_remove(), because rte_eth_dev_close() doesn't clear "eth_dev->data"
> It would be clearer if you start this sentence with:
> "In secondary process, rte_eth_dev_close() doesn't clear eth_dev->data."
> Then you can explain that if calling rte_dev_remove() after rte_eth_dev_close(),
> etc...
Right. Thanks!😁
>> , the address of the released Ethernet device can still be found by device
>> name. As a result, the Ethernet device will be released repeatedly in this
>> case. The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
>> calling rte_eth_dev_close(). Use this state to avoid this problem.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> +	/*
>> +	 * In secondary process, if applications first call rte_eth_dev_close()
>> +	 * and then call this interface, because rte_eth_dev_close() doesn't
>> +	 * clear eth_dev->data, the address of the released Ethernet device can
>> +	 * still be found by device name. As a result, the Ethernet device will
>> +	 * be released repeatedly in this case.
>> +	 * The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
>> +	 * calling rte_eth_dev_close(). Use this state to avoid this problem.
> This is a comment for the commit log.
> Inside the code, we should be more to the point.
> I suggest this comment:
> /* A released port can be found by its name in shared memory. */
ack
>
>> +	 */
>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY &&
> Better to directly compare with RTE_PROC_SECONDARY
ack
>
>> +	    eth_dev->state == RTE_ETH_DEV_UNUSED) {
>> +		RTE_ETHDEV_LOG(INFO, "The ethdev port has been released.");
> Not sure we need any log here.
ack
>
>> +		return 0;
>> +	}
>
>
> .
lihuisong (C) Oct. 14, 2021, 12:32 p.m. UTC | #3 
Hi, Thomas

*The commit log：*
In secondary process, rte_eth_dev_close() doesn't clear eth_dev->data.
If calling rte_dev_remove() after rte_eth_dev_close(), in 
rte_eth_dev_pci_generic_remove()
function, the released eth device still can be found by its name in 
shared memory.
As a result, the eth device will be released repeatedly. The state of 
the eth device
is modified to RTE_ETH_DEV_UNUSED after rte_eth_dev_close(). So this 
state can
be used to avoid this problem.

Is that will be more clear?

/*
  * A released eth device can be found by its name in shared memory.
  * If the state of the eth device is RTE_ETH_DEV_UNUSED, which means
  * the eth device has been released.
  */

Is it ok to use the above description as a comment in the code?

Hope for your reply.  Thanks.


在 2021/10/12 23:33, Thomas Monjalon 写道:
> 12/10/2021 13:39, Huisong Li:
>> The rte_eth_dev_pci_generic_remove() will be called to detach an Ethernet
>> device when App calls rte_dev_remove() to detach a pci device. In addition,
>> the rte_eth_dev_close() can also detach an Ethernet device.
>> In secondary process, if App first calls rte_eth_dev_close() and then calls
>> rte_dev_remove(), because rte_eth_dev_close() doesn't clear "eth_dev->data"
> It would be clearer if you start this sentence with:
> "In secondary process, rte_eth_dev_close() doesn't clear eth_dev->data."
> Then you can explain that if calling rte_dev_remove() after rte_eth_dev_close(),
> etc...
>
>> , the address of the released Ethernet device can still be found by device
>> name. As a result, the Ethernet device will be released repeatedly in this
>> case. The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
>> calling rte_eth_dev_close(). Use this state to avoid this problem.
>>
>> Signed-off-by: Huisong Li<lihuisong@huawei.com>
>> ---
>> +	/*
>> +	 * In secondary process, if applications first call rte_eth_dev_close()
>> +	 * and then call this interface, because rte_eth_dev_close() doesn't
>> +	 * clear eth_dev->data, the address of the released Ethernet device can
>> +	 * still be found by device name. As a result, the Ethernet device will
>> +	 * be released repeatedly in this case.
>> +	 * The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
>> +	 * calling rte_eth_dev_close(). Use this state to avoid this problem.
> This is a comment for the commit log.
> Inside the code, we should be more to the point.
> I suggest this comment:
> /* A released port can be found by its name in shared memory. */
>
>> +	 */
>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY &&
> Better to directly compare with RTE_PROC_SECONDARY
>
>> +	    eth_dev->state == RTE_ETH_DEV_UNUSED) {
>> +		RTE_ETHDEV_LOG(INFO, "The ethdev port has been released.");
> Not sure we need any log here.
>
>> +		return 0;
>> +	}
>
>
> .
Thomas Monjalon Oct. 14, 2021, 12:50 p.m. UTC | #4 
14/10/2021 14:32, lihuisong (C):
> Hi, Thomas
> 
> *The commit log：*
> In secondary process, rte_eth_dev_close() doesn't clear eth_dev->data.
> If calling rte_dev_remove() after rte_eth_dev_close(), in 
> rte_eth_dev_pci_generic_remove()
> function, the released eth device still can be found by its name in 
> shared memory.
> As a result, the eth device will be released repeatedly. The state of 
> the eth device
> is modified to RTE_ETH_DEV_UNUSED after rte_eth_dev_close(). So this 
> state can
> be used to avoid this problem.
> 
> Is that will be more clear?

Yes, that's clear (at least for me).

> /*
>   * A released eth device can be found by its name in shared memory.
>   * If the state of the eth device is RTE_ETH_DEV_UNUSED, which means
>   * the eth device has been released.
>   */
> 
> Is it ok to use the above description as a comment in the code?

Yes. One small change, I think "which" should be "it".

> Hope for your reply.  Thanks.

Thanks
lihuisong (C) Oct. 15, 2021, 3:03 a.m. UTC | #5 
在 2021/10/14 20:50, Thomas Monjalon 写道:
> 14/10/2021 14:32, lihuisong (C):
>> Hi, Thomas
>>
>> *The commit log：*
>> In secondary process, rte_eth_dev_close() doesn't clear eth_dev->data.
>> If calling rte_dev_remove() after rte_eth_dev_close(), in
>> rte_eth_dev_pci_generic_remove()
>> function, the released eth device still can be found by its name in
>> shared memory.
>> As a result, the eth device will be released repeatedly. The state of
>> the eth device
>> is modified to RTE_ETH_DEV_UNUSED after rte_eth_dev_close(). So this
>> state can
>> be used to avoid this problem.
>>
>> Is that will be more clear?
> Yes, that's clear (at least for me).
>
>> /*
>>    * A released eth device can be found by its name in shared memory.
>>    * If the state of the eth device is RTE_ETH_DEV_UNUSED, which means
>>    * the eth device has been released.
>>    */
>>
>> Is it ok to use the above description as a comment in the code?
> Yes. One small change, I think "which" should be "it".
Thanks. I will fix it.
>
>> Hope for your reply.  Thanks.
> Thanks
>
>
>
>
> .
diff mbox series

Patch

diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
index 8edca82ce8..af01cceddf 100644
--- a/lib/ethdev/ethdev_pci.h
+++ b/lib/ethdev/ethdev_pci.h
@@ -151,6 +151,21 @@  rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
 	if (!eth_dev)
 		return 0;
 
+	/*
+	 * In secondary process, if applications first call rte_eth_dev_close()
+	 * and then call this interface, because rte_eth_dev_close() doesn't
+	 * clear eth_dev->data, the address of the released Ethernet device can
+	 * still be found by device name. As a result, the Ethernet device will
+	 * be released repeatedly in this case.
+	 * The state of the Ethernet device is equal to RTE_ETH_DEV_UNUSED after
+	 * calling rte_eth_dev_close(). Use this state to avoid this problem.
+	 */
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY &&
+	    eth_dev->state == RTE_ETH_DEV_UNUSED) {
+		RTE_ETHDEV_LOG(INFO, "The ethdev port has been released.");
+		return 0;
+	}
+
 	if (dev_uninit) {
 		ret = dev_uninit(eth_dev);
 		if (ret)