[v3,1/2] ethdev: queue release callback optional
Checks
Commit Message
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
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.
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.
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.
@@ -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)