Re: [dpdk-dev] [PATCH v2] ethdev: change queue release callback

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

On Thu, 2021-09-16 at 10:09 +0200, Thomas Monjalon wrote:
> 15/09/2021 15:02, Xueming Li:
> > To align with other eth device queue configuration callbacks, cleanup
> > queue release callback API by changing RX and TX queue release callback
> > API parameter from queue object to device and queue index.
> 
> Please split the explanation in multiple sentences,
> so it allows taking a breath while reading :)
> A good method is to start with explaining the status:
> 	most callbacks use queue ID
> 	queue release callbacks use queue object
> Then explain what is done:
> 	queue release callbacks are changed to use queue ID
> 	all drivers are adapted
> 	empty callbacks are removed in some drivers
> 
> > This patch allows NULL callback to avoid defining empty callbacks.
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > Cc: Ferruh Yigit <ferruh.yigit@intel.com>
> > Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Cc: "Singh, Aman Deep" <aman.deep.singh@intel.com>
> > ---
> > v2: included new NFP PMD driver
> > 
> > This patch is a preparation of shared Rx queue feature since the rxq
> > object is subject to be used by PMD as shared rxq object which is shared
> > among rx queues in same share group:
> >   https://mails.dpdk.org/archives/dev/2021-July/215575.html
> 
> This note does not fully explain why using the queue ID helps.
> 
> 
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > -typedef void (*eth_queue_release_t)(void *queue);
> > +typedef void (*eth_queue_release_t)(struct rte_eth_dev *dev,
> > +				    uint16_t rx_queue_id);
> 
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > -		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
> 
> Why removing this check?
> It is changing the behaviour.

Some PMD implemented a dummy callback, so I think maybe it good to make
this callback optional to make PMD clean, how do you think?

> 
> > -
> > +		if (dev->dev_ops->rx_queue_release != NULL)
> > +			for (i = nb_queues; i < old_nb_queues; i++)
> > +				(*dev->dev_ops->rx_queue_release)(dev, i);
> >  		rxq = dev->data->rx_queues;
> > -
> > -		for (i = nb_queues; i < old_nb_queues; i++)
> > -			(*dev->dev_ops->rx_queue_release)(rxq[i]);
> >  		rxq = rte_realloc(rxq, sizeof(rxq[0]) * nb_queues,
> >  				RTE_CACHE_LINE_SIZE);
> 
> 
>