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 <xuemi...@nvidia.com>
> > Reviewed-by: Andrew Rybchenko <andrew.rybche...@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) <humi...@huawei.com>
@Yisen Zhuang <yisen.zhu...@huawei.com>
@Lijun Ou <ouli...@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 <spin...@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.
