[dpdk-dev] [PATCH v5 2/2] ethdev: change queue release callback

[Ferruh Yigit]

On 10/6/2021 8:55 AM, Xueming(Steven) Li wrote:
> On Tue, 2021-10-05 at 17:38 +0100, Ferruh Yigit wrote:
>> On 9/29/2021 2:57 PM, Xueming(Steven) Li wrote:
>>> On Wed, 2021-09-22 at 12:54 +0000, Xueming(Steven) Li wrote:
>>>> On Wed, 2021-09-22 at 11:57 +0100, Ferruh Yigit wrote:
>>>>>>>
>>>>>>> <...>
>>>>>>>
>>>>>>>>    void
>>>>>>>> -i40e_dev_rx_queue_release(void *rxq)
>>>>>>>> +i40e_dev_rx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>>>>>>> +{
>>>>>>>> +	i40e_rx_queue_release(dev->data->rx_queues[qid]);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void
>>>>>>>> +i40e_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>>>>>>> +{
>>>>>>>> +	i40e_tx_queue_release(dev->data->tx_queues[qid]);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>
>>>>>>> Is there any specific reason to not update driver but add wrappers for it?
>>>>>>
>>>>>> Some caller don't have queue ID on hand, adding wrapper seems more
>>>>>> convinient.
>>>>>>
>>>>>
>>>>> Convinient for the patch, but not sure convinient for the driver.
>>>>>
>>>>> As mentioned before, not sure about approach to update some driver and add
>>>>> wrappers for some others.
>>>>>
>>>>> qede, ice and i40e seems not updated, I am for syncronizing with their
>>>>> maintainers before proceed.
>>>>>
>>>>>>
>>>>
>>>> For qede, qede_tx_queue_release(txq_obj) is called by
>>>> qede_alloc_tx_queue_mem(dev, qid), while upper caller
>>>> qede_tx_queue_setup() doesn't always save txq_obj to dev->data->txqs[].
>>>>
>>>> For ice and i40e, it's similar, ice_tx_queue_release() is used to free
>>>> txq, but some txq isn't saved into dev, please refer to
>>>> ice_fdir_setup(), wrapper is needed.
>>>>
>>>> These 3 PMDs create rxq/txq that not saved in dev->data, can't change
>>>> parameter to dev+qid for such case, that's why wrapper was there.
>>>>
>>>
>>> Hi Ferruh,
>>>
>>> No response from qede, ice and i40e. Basically the original queue
>>> release api is shared by private queues(not registered to ethdev),
>>> can't access by index, that why a warpper was there. To avoid more
>>> rebase in last minute for this big patch, do you think we could close
>>> it?
>>>
>>
>> I see the reason and since there is no update from maintainers, to keep
>> the ball rolling agree to continue with wrappers, those PMDs can send
>> incremental patches if required.
>>
>>> BTW, from feedback from hns3, I will post a new version to add the
>>> macro.
>>>
>>
>> I have concern about this one, how accessing to the global variable
>> 'rte_eth_devices' via a macro improves the situation?
>>
>> Can you please make wrappers for hns3 driver too, we can follow it later
>> with driver maintainer?
> 
> hns3 doesn't need a wrapper. The macro isn't related to wrapper, just
> for the rte_eth_devices access as you suggested:
> 	&rte_eth_devices[hw->data->port_id]
> 

I suggested not to access global 'rte_eth_devices' variable from driver, and v6 has
a macro in the driver [1] to access the same variable, hiding it behind a macro
is not changing anything.
Since your updates adds more access to 'rte_eth_devices' [2], my suggestion was
do a quick wrapper solution for the driver until it is properly updated.

[1]
#define HNS3_DEV(hw) &rte_eth_devices[(hw)->data->port_id]

[2]
	-	hns3_dev_rx_queue_release(rxq[i]);
	+	hns3_dev_rx_queue_release(HNS3_DEV(hw), i);