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

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

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.

<...>

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

<...>

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

<...>

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

<...>

>  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?

<...>

> +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?

<...>

> 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()'

<...>

> @@ -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'?

<...>

> @@ -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?