diff mbox series

[v3,1/2] ethdev: queue release callback optional

Message ID 20210917093915.350863-2-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Headers
Series ethdev: change queue release callback |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Xueming Li Sept. 17, 2021, 9:39 a.m. UTC 
  Some drivers don't need Rx and Tx queue release callback, make it
optional.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 lib/ethdev/rte_ethdev.c | 48 +++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

Comments

Andrew Rybchenko Sept. 17, 2021, 11:29 a.m. UTC | #1 
On 9/17/21 12:39 PM, Xueming Li wrote:
> Some drivers don't need Rx and Tx queue release callback, make it
> optional.
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

LGTM, but please, consider one nit below

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

> ---
>  lib/ethdev/rte_ethdev.c | 48 +++++++++++++++++------------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index daf5ca9242..2f316d1646 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -905,12 +905,11 @@ eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
>  			return -(ENOMEM);
>  		}
>  	} else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */
> -		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
> -
>  		rxq = dev->data->rx_queues;
>  
> -		for (i = nb_queues; i < old_nb_queues; i++)
> -			(*dev->dev_ops->rx_queue_release)(rxq[i]);
> +		if (dev->dev_ops->rx_queue_release != NULL)
> +			for (i = nb_queues; i < old_nb_queues; i++)
> +				(*dev->dev_ops->rx_queue_release)(rxq[i]);

Since 'if' body has more than one line, I'd add curly brackets
around to make it a bit easier to read and more robust against
future changes.

Similar note is applicable to many similar cases in the patch.
Andrew Rybchenko Sept. 17, 2021, 11:53 a.m. UTC | #2 
On 9/17/21 2:29 PM, Andrew Rybchenko wrote:
> On 9/17/21 12:39 PM, Xueming Li wrote:
>> Some drivers don't need Rx and Tx queue release callback, make it
>> optional.
>>
>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> 
> LGTM, but please, consider one nit below
> 
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
>> ---
>>  lib/ethdev/rte_ethdev.c | 48 +++++++++++++++++------------------------
>>  1 file changed, 20 insertions(+), 28 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index daf5ca9242..2f316d1646 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -905,12 +905,11 @@ eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
>>  			return -(ENOMEM);
>>  		}
>>  	} else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */
>> -		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
>> -
>>  		rxq = dev->data->rx_queues;
>>  
>> -		for (i = nb_queues; i < old_nb_queues; i++)
>> -			(*dev->dev_ops->rx_queue_release)(rxq[i]);
>> +		if (dev->dev_ops->rx_queue_release != NULL)
>> +			for (i = nb_queues; i < old_nb_queues; i++)
>> +				(*dev->dev_ops->rx_queue_release)(rxq[i]);
> 
> Since 'if' body has more than one line, I'd add curly brackets
> around to make it a bit easier to read and more robust against
> future changes.
> 
> Similar note is applicable to many similar cases in the patch.
> 

Reviewed the next patch I realize one thing:
Who is responsible for setting dev->data->rxq[rx_queue_id] to
NULL if release callback is not specified. IMHO, it is
inconsistent to keep it filled in after release. I think that
the generic code in ethdev must care about it regardless
callback presence. It means that we need helper function
which cares about it in single place. Also it means that
we can't optimize loops like this. We need the loop anyway
to set all queue pointers to NULL.
Xueming Li Sept. 17, 2021, 2:33 p.m. UTC | #3 
On Fri, 2021-09-17 at 14:53 +0300, Andrew Rybchenko wrote:
> On 9/17/21 2:29 PM, Andrew Rybchenko wrote:
> > On 9/17/21 12:39 PM, Xueming Li wrote:
> > > Some drivers don't need Rx and Tx queue release callback, make it
> > > optional.
> > > 
> > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > 
> > LGTM, but please, consider one nit below
> > 
> > Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > 
> > > ---
> > >  lib/ethdev/rte_ethdev.c | 48 +++++++++++++++++------------------------
> > >  1 file changed, 20 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > > index daf5ca9242..2f316d1646 100644
> > > --- a/lib/ethdev/rte_ethdev.c
> > > +++ b/lib/ethdev/rte_ethdev.c
> > > @@ -905,12 +905,11 @@ eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
> > >  			return -(ENOMEM);
> > >  		}
> > >  	} else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */
> > > -		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
> > > -
> > >  		rxq = dev->data->rx_queues;
> > >  
> > > -		for (i = nb_queues; i < old_nb_queues; i++)
> > > -			(*dev->dev_ops->rx_queue_release)(rxq[i]);
> > > +		if (dev->dev_ops->rx_queue_release != NULL)
> > > +			for (i = nb_queues; i < old_nb_queues; i++)
> > > +				(*dev->dev_ops->rx_queue_release)(rxq[i]);
> > 
> > Since 'if' body has more than one line, I'd add curly brackets
> > around to make it a bit easier to read and more robust against
> > future changes.
> > 
> > Similar note is applicable to many similar cases in the patch.
> > 
> 
> Reviewed the next patch I realize one thing:
> Who is responsible for setting dev->data->rxq[rx_queue_id] to
> NULL if release callback is not specified. IMHO, it is
> inconsistent to keep it filled in after release. I think that
> the generic code in ethdev must care about it regardless
> callback presence. It means that we need helper function
> which cares about it in single place. Also it means that
> we can't optimize loops like this. We need the loop anyway
> to set all queue pointers to NULL.

Yes, a dedicate function make things more clear, thanks! updated in v4.
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index daf5ca9242..2f316d1646 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -905,12 +905,11 @@  eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 			return -(ENOMEM);
 		}
 	} else if (dev->data->rx_queues != NULL && nb_queues != 0) { /* re-configure */
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
-
 		rxq = dev->data->rx_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->rx_queue_release)(rxq[i]);
+		if (dev->dev_ops->rx_queue_release != NULL)
+			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);
 		if (rxq == NULL)
@@ -925,12 +924,11 @@  eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 		dev->data->rx_queues = rxq;
 
 	} else if (dev->data->rx_queues != NULL && nb_queues == 0) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, -ENOTSUP);
-
 		rxq = dev->data->rx_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->rx_queue_release)(rxq[i]);
+		if (dev->dev_ops->rx_queue_release != NULL)
+			for (i = nb_queues; i < old_nb_queues; i++)
+				(*dev->dev_ops->rx_queue_release)(rxq[i]);
 
 		rte_free(dev->data->rx_queues);
 		dev->data->rx_queues = NULL;
@@ -1145,12 +1143,11 @@  eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 			return -(ENOMEM);
 		}
 	} else if (dev->data->tx_queues != NULL && nb_queues != 0) { /* re-configure */
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release, -ENOTSUP);
-
 		txq = dev->data->tx_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->tx_queue_release)(txq[i]);
+		if (dev->dev_ops->tx_queue_release != NULL)
+			for (i = nb_queues; i < old_nb_queues; i++)
+				(*dev->dev_ops->tx_queue_release)(txq[i]);
 		txq = rte_realloc(txq, sizeof(txq[0]) * nb_queues,
 				  RTE_CACHE_LINE_SIZE);
 		if (txq == NULL)
@@ -1165,12 +1162,11 @@  eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 		dev->data->tx_queues = txq;
 
 	} else if (dev->data->tx_queues != NULL && nb_queues == 0) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release, -ENOTSUP);
-
 		txq = dev->data->tx_queues;
 
-		for (i = nb_queues; i < old_nb_queues; i++)
-			(*dev->dev_ops->tx_queue_release)(txq[i]);
+		if (dev->dev_ops->tx_queue_release != NULL)
+			for (i = nb_queues; i < old_nb_queues; i++)
+				(*dev->dev_ops->tx_queue_release)(txq[i]);
 
 		rte_free(dev->data->tx_queues);
 		dev->data->tx_queues = NULL;
@@ -2112,9 +2108,8 @@  rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 
 	rxq = dev->data->rx_queues;
 	if (rxq[rx_queue_id]) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
-					-ENOTSUP);
-		(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
+		if (dev->dev_ops->rx_queue_release != NULL)
+			(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
 		rxq[rx_queue_id] = NULL;
 	}
 
@@ -2248,9 +2243,8 @@  rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EBUSY;
 	rxq = dev->data->rx_queues;
 	if (rxq[rx_queue_id] != NULL) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
-					-ENOTSUP);
-		(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
+		if (dev->dev_ops->rx_queue_release != NULL)
+			(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
 		rxq[rx_queue_id] = NULL;
 	}
 	ret = (*dev->dev_ops->rx_hairpin_queue_setup)(dev, rx_queue_id,
@@ -2316,9 +2310,8 @@  rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 
 	txq = dev->data->tx_queues;
 	if (txq[tx_queue_id]) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
-					-ENOTSUP);
-		(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
+		if (dev->dev_ops->tx_queue_release != NULL)
+			(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
 		txq[tx_queue_id] = NULL;
 	}
 
@@ -2428,9 +2421,8 @@  rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 		return -EBUSY;
 	txq = dev->data->tx_queues;
 	if (txq[tx_queue_id] != NULL) {
-		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
-					-ENOTSUP);
-		(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
+		if (dev->dev_ops->tx_queue_release != NULL)
+			(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
 		txq[tx_queue_id] = NULL;
 	}
 	ret = (*dev->dev_ops->tx_hairpin_queue_setup)