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

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

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'm afraid adding more patches to improve PMD drivers might make this
patcheset over complex.

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.

> 
> <...>
> 
> > +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. 
@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.