[RFC,V1] ethdev: fix the issue that dev uninit may be called twice

  Ethernet devices in DPDK can be released by rte_eth_dev_close() and
rte_dev_remove(). However, these two APIs do not have explicit invocation
restrictions. In other words, at the ethdev layer, calling
rte_eth_dev_close() and then rte_dev_remove() or rte_eal_hotplug_remove()
is allowed. In such a bad scenario, the primary process may be fine, but it
may cause that dev_unint() in the secondary process will be called twice,
and even other serious problems. So this patch fixes it.

Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 lib/ethdev/ethdev_pci.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

Hi Huison,

On 8/2/2021 6:16 PM, Huisong Li wrote:
> Ethernet devices in DPDK can be released by rte_eth_dev_close() and
> rte_dev_remove(). However, these two APIs do not have explicit invocation
> restrictions. In other words, at the ethdev layer, calling
> rte_eth_dev_close() and then rte_dev_remove() or rte_eal_hotplug_remove()
> is allowed. In such a bad scenario, the primary process may be fine, but it
> may cause that dev_unint() in the secondary process will be called twice,

Shouldn't dev_unint() for Secondary process, simply return with no-action.

> and even other serious problems. So this patch fixes it.
>
> Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   lib/ethdev/ethdev_pci.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
> index 8edca82..14a0e01 100644
> --- a/lib/ethdev/ethdev_pci.h
> +++ b/lib/ethdev/ethdev_pci.h
> @@ -151,6 +151,19 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
>   	if (!eth_dev)
>   		return 0;
>   
> +	/*
> +	 * The eth_dev->data->name doesn't be cleared by the secondary precess,
Can we reprase above sentence "doesn't be cleared "
> +	 * so above "eth_dev" isn't NULL after rte_eth_dev_close() called.
> +	 * Namely, whether "eth_dev" is NULL cannot be used to determine whether
> +	 * an ethdev port has been released.
> +	 * For both primary precess and secondary precess, eth_dev->state is
s/ precess / process
> +	 * RTE_ETH_DEV_UNUSED, which means the ethdev port has been released.
> +	 */
> +	if (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)
  

Hi, Singh, Aman Deep

Sorry, I missed your review. Thank you for your review.😁

在 2021/8/18 17:47, Singh, Aman Deep 写道:
>
> Hi Huison,
>
> On 8/2/2021 6:16 PM, Huisong Li wrote:
>> Ethernet devices in DPDK can be released by rte_eth_dev_close() and
>> rte_dev_remove(). However, these two APIs do not have explicit invocation
>> restrictions. In other words, at the ethdev layer, calling
>> rte_eth_dev_close() and then rte_dev_remove() or rte_eal_hotplug_remove()
>> is allowed. In such a bad scenario, the primary process may be fine, but it
>> may cause that dev_unint() in the secondary process will be called twice,
> Shouldn't dev_unint() for Secondary process, simply return with no-action.

The prerequisite is that the secondary process does not have any 
resources that need to be released.

However, some resources from secondary process may also need to be 
released. For example, mp action

registered by rte_mp_action_register() is used to for multi-process 
communication. It should be unloaded

when all eth devices driven by one PMD in a process are removed. In 
order to achieve the above purpose,

secondary process may have data recording the number of device to decide 
when to deregister the action.

Of course, this is just the case. In short, secondary process may have 
its own private data or resources to

be released.

It is mentioned in RFC v2. Please go to discussion line of RFC v2.

>> and even other serious problems. So this patch fixes it.
>>
>> Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")
>>
>> Signed-off-by: Huisong Li<lihuisong@huawei.com>
>> ---
>>   lib/ethdev/ethdev_pci.h | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
>> index 8edca82..14a0e01 100644
>> --- a/lib/ethdev/ethdev_pci.h
>> +++ b/lib/ethdev/ethdev_pci.h
>> @@ -151,6 +151,19 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
>>   	if (!eth_dev)
>>   		return 0;
>>   
>> +	/*
>> +	 * The eth_dev->data->name doesn't be cleared by the secondary precess,
> Can we reprase above sentence "doesn't be cleared "
ok
>> +	 * so above "eth_dev" isn't NULL after rte_eth_dev_close() called.
>> +	 * Namely, whether "eth_dev" is NULL cannot be used to determine whether
>> +	 * an ethdev port has been released.
>> +	 * For both primary precess and secondary precess, eth_dev->state is
> s/ precess / process
Thanks. RFC v2 has corrected it.
>> +	 * RTE_ETH_DEV_UNUSED, which means the ethdev port has been released.
>> +	 */
>> +	if (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)