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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/22/2021 10:35 AM, Xueming(Steven) Li wrote:
> Hi Ferruh, 
> 
> Appreciate for the careful check!
> 
> On Tue, 2021-09-21 at 19:13 +0100, Ferruh Yigit wrote:
>> On 9/18/2021 1:35 PM, Xueming Li wrote:
>>> Currently, most ethdev callback API use queue ID as parameter, but Rx
>>> and Tx queue release callback use queue object which is used by Rx and
>>> Tx burst data plane callback.
>>>
>>> To align with other eth device queue configuration callbacks:
>>> - queue release callbacks are changed to use queue ID
>>> - all drivers are adapted
>>>
>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>
>> <...>
>>
>>> -void bnxt_rx_queue_release_op(void *rx_queue)
>>> +void bnxt_rx_queue_release_op(struct rte_eth_dev *dev, uint16_t queue_idx)
>>>  {
>>> -	struct bnxt_rx_queue *rxq = (struct bnxt_rx_queue *)rx_queue;
>>> +	struct bnxt_rx_queue *rxq = dev->data->rx_queues[queue_idx];
>>>  
>>>  	if (rxq) {
>>>  		if (is_bnxt_in_error(rxq->bp))
>>> @@ -273,6 +273,7 @@ void bnxt_rx_queue_release_op(void *rx_queue)
>>>  		rxq->mz = NULL;
>>>  
>>>  		rte_free(rxq);
>>> +		dev->data->rx_queues[queue_idx] = NULL;
>>
>> Similar question as did a few more times (I started to review from bottom), if
>> this change is related to this set and if it should be fixed first before this
>> patch.
>>
> 
> Please check the comments at bottom.
> 
>> <...>
>>
>>> -void bnxt_tx_queue_release_op(void *tx_queue)
>>> +void bnxt_tx_queue_release_op(struct rte_eth_dev *dev, uint16_t queue_idx)
>>>  {
>>> -	struct bnxt_tx_queue *txq = (struct bnxt_tx_queue *)tx_queue;
>>> +	struct bnxt_tx_queue *txq = dev->data->tx_queues[queue_idx];
>>>  
>>>  	if (txq) {
>>>  		if (is_bnxt_in_error(txq->bp))
>>> @@ -83,6 +83,7 @@ void bnxt_tx_queue_release_op(void *tx_queue)
>>>  
>>>  		rte_free(txq->free);
>>>  		rte_free(txq);
>>> +		dev->data->tx_queues[queue_idx] = NULL;
>>
>> ditto.
>>
>>>  	}
>>>  }
>>>  
>>> @@ -115,7 +116,7 @@ int bnxt_tx_queue_setup_op(struct rte_eth_dev *eth_dev,
>>>  	if (eth_dev->data->tx_queues) {
>>>  		txq = eth_dev->data->tx_queues[queue_idx];
>>>  		if (txq) {
>>> -			bnxt_tx_queue_release_op(txq);
>>> +			bnxt_tx_queue_release_op(eth_dev, queue_idx);
>>>  			txq = NULL;
>>
>> For the 'txq = NULL', can the intetion be
>> "eth_dev->data->tx_queues[queue_idx] = NULL", since above
>> 'bnxt_tx_queue_release_op()' adds this assignment. Otherwise it looks unnecessary.
>>
> 
> Correct, removed.
> 
>> <...>
>>
>>> @@ -2421,7 +2421,6 @@ static struct eth_dev_ops dpaa2_ethdev_ops = {
>>>  	.rx_queue_setup    = dpaa2_dev_rx_queue_setup,
>>>  	.rx_queue_release  = dpaa2_dev_rx_queue_release,
>>>  	.tx_queue_setup    = dpaa2_dev_tx_queue_setup,
>>> -	.tx_queue_release  = dpaa2_dev_tx_queue_release,
>>
>> This should be removed in previous patch.
> 
> Nice catch, thanks!
> 
>>
>> <...>
>>
>>> @@ -1536,7 +1536,8 @@ hns3_fake_rx_queue_config(struct hns3_hw *hw, uint16_t nb_queues)
>>>  		/* re-configure */
>>>  		rxq = hw->fkq_data.rx_queues;
>>>  		for (i = nb_queues; i < old_nb_queues; i++)
>>> -			hns3_dev_rx_queue_release(rxq[i]);
>>> +			hns3_dev_rx_queue_release
>>> +				(&rte_eth_devices[hw->data->port_id], i);
>>
>> I think this should be done without driver accesing the global 'rte_eth_devices'
>> array. This may require more help from driver maintainer, and perhaps wrapper
>> functions can be used for the driver for now and maintainer can update it later,
>> if agreed with the driver maintainer.
> 
> There are about 20 direct accessing to the global 'rte_eth_devices',

I already made similar comment to the driver previously, but lets just not add
more usage.

> I'm afraid adding more patches to improve PMD drivers might make this
> patcheset over complex.
> 

Agree to not complex this patch for it.

> Huawei maintainers, could you please address Ferruh's suggestion? just
> a clean up whith simple macro.
> 
> @Min Hu (Connor) <humin29@huawei.com>
> @Yisen Zhuang <yisen.zhuang@huawei.com>
> @Lijun Ou <oulijun@huawei.com>
> 
>>
>> <...>
>>
>>>  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.

>>
>> <...>
>>
>>> +void
>>> +ice_dev_rx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>> +{
>>> +	ice_rx_queue_release(dev->data->rx_queues[qid]);
>>> +}
>>> +
>>> +void
>>> +ice_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>> +{
>>> +	ice_tx_queue_release(dev->data->tx_queues[qid]);
>>> +}
>>> +
>>
>> Is there any specific reason to not update driver but add wrappers for it?
>>
> 
> Same, lots for calls to ice_tx_queue_release(), and in some case qid
> not there.
> 
>> <...>
>>
>>> diff --git a/drivers/net/nfb/nfb_rx.c b/drivers/net/nfb/nfb_rx.c
>>> index d6d4ba9663..3ebb332ae4 100644
>>> --- a/drivers/net/nfb/nfb_rx.c
>>> +++ b/drivers/net/nfb/nfb_rx.c
>>> @@ -176,9 +176,10 @@ nfb_eth_rx_queue_init(struct nfb_device *nfb,
>>>  }
>>>  
>>>  void
>>> -nfb_eth_rx_queue_release(void *q)
>>> +nfb_eth_rx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>>  {
>>> -	struct ndp_rx_queue *rxq = (struct ndp_rx_queue *)q;
>>> +	struct ndp_rx_queue *rxq = dev->data->rx_queues[qid];
>>> +
>>>  	if (rxq->queue != NULL) {
>>>  		ndp_close_rx_queue(rxq->queue);
>>>  		rte_free(rxq);
>>
>> Unrealted with this patch, but this code follows as below, why setting
>> 'rxq->queue = NULL' after 'rxq' is freed? This needs to be fixed separately (
>>
>>
>>       1         if (rxq->queue != NULL) {
>>       2                 ndp_close_rx_queue(rxq->queue);
>>       3                 rte_free(rxq);
>>       4                 rxq->queue = NULL;
>>       5         }
>>
>> Same for 'nfb_eth_tx_queue_release()'
> 
> Yes, looks like rxq->queue need to freed. 

Also shouldn't update a field of a freed object (again not for this patch).

> @Martin Spinler <spinler@cesnet.cz>
> 
>>
>> <...>
>>
>>> @@ -556,7 +558,8 @@ nfp_net_rx_queue_setup(struct rte_eth_dev *dev,
>>>  
>>>  	if (tz == NULL) {
>>>  		PMD_DRV_LOG(ERR, "Error allocating rx dma");
>>> -		nfp_net_rx_queue_release(rxq);
>>> +		nfp_net_rx_queue_release(dev, queue_idx);
>>> +		dev->data->rx_queues[queue_idx] = NULL;
>>
>> Althoug I agree on NULL assignment, not sure if it is related for this patch. It
>> seems this assignment was missing and this patch is fixing it, independent from
>> API change.
>>
>> I did similar comment for other drivers, as a generic comment, what about fixing
>> them in a separate patch first, and do the API convertion later?
>>
>> <...>
>>
>>> @@ -248,16 +248,18 @@ otx_ep_rx_queue_setup(struct rte_eth_dev *eth_dev, uint16_t q_no,
>>>   * Release the receive queue/ringbuffer. Called by
>>>   * the upper layers.
>>>   *
>>> - * @param rxq
>>> - *    Opaque pointer to the receive queue to release
>>> + * @param dev
>>> + *    Pointer to the structure rte_eth_dev
>>> + * @param q_no
>>> + *    Queue number
>>>   *
>>
>> For rest of the drivers, comments updated as following, I suggest keeping
>> consistent wording on all drivers:
>>
>> @param dev
>>   Pointer to Ethernet device structure.
>> @param q_no
>>   Receive queue index.
>> <...>
>>
>>>  
>>> +static void
>>> +qede_dev_rx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>> +{
>>> +	qede_rx_queue_release(dev->data->rx_queues[qid]);
>>> +}
>>> +
>>> +static void
>>> +qede_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
>>> +{
>>> +	qede_tx_queue_release(dev->data->tx_queues[qid]);
>>> +}
>>> +
>>
>> Technically this wrapping can be done for all drivers, I can see it simplifies
>> the implementation for this patch but not sure if it is best for the driver.
>>
>> And since in other drivers implemenation is changed instead of this wrapper, is
>> there a specific reason to not update the implementation for 'qede'?
> 
> Depends on how many internal calls to orginal release function, if too
> much, adding a wrapper looks easier. If not too much calls, the is
> changed directly.
> 
>>
>> <...>
>>
>>> @@ -872,6 +871,7 @@ nicvf_dev_tx_queue_release(void *sq)
>>>  			txq->txbuffs = NULL;
>>>  		}
>>>  		rte_free(txq);
>>> +		dev->data->tx_queues[qid] = NULL;
>>
>> This may be required but seems not related to changing release API parameters
>> ('eth_dev_txq_release()' already sets txq[id] to NULL).
>>
>> And if this change is required, same can be required for
>> 'nicvf_dev_rx_queue_release()', what about dropping this change from set and
>> make a separate patch for it if required?
> 
> In function nicvf_dev_tx_queue_setup(), orginal logic is:
>   - init txq_obj
>   - init other resources, call txq_release(txq_obj) on error
>   - dev->data->txqs[qid] = txq_obj
> With this patch, the order changed:
>   - init txq obj
>   - dev->data->txqs[qid] = txq_obj
>   - init other resources, call txq_release(dev,qid) on error
>     So it's important to clear txq_obj from dev->data->txqs[].
>     W/o this line, have to clear it after each call of txq_release.
> 

You are right, I thought NULL assignment is required for the original code too
but no, it is required because of reordering. So please ignore all similar
comments. (perhaps bnxt one can have a fix before this patch, but since it is
small OK to have it here)

So only major issue is having an agreement on hns3, ice, i40e & qede
implementations with their maintainers.