[dpdk-dev] [PATCH v1 1/2] net/af_xdp: remove resources when port is closed

[Ferruh Yigit]

On 4/30/2019 8:55 AM, Thomas Monjalon wrote:
> 30/04/2019 09:33, Ferruh Yigit:
>> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
>>> 30/04/2019 00:28, Ferruh Yigit:
>>>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>>>> 29/04/2019 19:00, Ferruh Yigit:
>>>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>>>> resources at the dev_close routine. So all resources previously released
>>>>>>> in remove routine are now released at the dev_close routine, and the
>>>>>>> dev_close routine will be called in driver remove routine in order to
>>>>>>> support removing a device without closing its ports.
>>>>>>>
>>>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>>>> flag during probe stage.
>>>>>>>
>>>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye at intel.com>
>>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>>>>  	if (eth_dev == NULL)
>>>>>>>  		return -1;
>>>>>>>  
>>>>>>> -	internals = eth_dev->data->dev_private;
>>>>>>> -
>>>>>>> -	rte_ring_free(internals->umem->buf_ring);
>>>>>>> -	rte_memzone_free(internals->umem->mz);
>>>>>>> -	rte_free(internals->umem);
>>>>>>> -
>>>>>>> -	rte_eth_dev_release_port(eth_dev);
>>>>>>
>>>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>>>> 'rte_eth_dev_close()' but still needed in  '.remove()' path.
>>>>>
>>>>> I don't understand your comment.
>>>>> Calling the close function looks the right thing to do in "remove".
>>>>
>>>> No concern on calling the 'close'.
>>>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>>>
>>> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>>
>> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
>> private resources.
>> I assume it is confused with 'rte_eth_dev_close()' ...
>>
>>>>>>> -
>>>>>>> +	eth_dev_close(eth_dev);
> 
> Ah yes, I overlooked it.
> Why not calling rte_eth_dev_close()?
> 

It may work, but I am for keeping 'dev_close()' and 'rte_eth_dev_release_port()'
as two steps in '.remove()', I think there is no need to include
'rte_eth_dev_close()' API here and its possible/future side affects etc...