Re: [dpdk-dev] [PATCH V2] ethdev: fix eth device released repeatedly

["lihuisong (C)" <lihuisong@huawei.com>]

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;
>> +	}
>
>
> .