From: "Xueming(Steven) Li" <xuemingl@nvidia.com>
To: "yisen.zhuang@huawei.com" <yisen.zhuang@huawei.com>,
"oulijun@huawei.com" <oulijun@huawei.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"humin29@huawei.com" <humin29@huawei.com>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
"spinler@cesnet.cz" <spinler@cesnet.cz>
Cc: "zhouguoyang@huawei.com" <zhouguoyang@huawei.com>,
"mczekaj@marvell.com" <mczekaj@marvell.com>,
"radhac@marvell.com" <radhac@marvell.com>,
"sthotton@marvell.com" <sthotton@marvell.com>,
Matan Azrad <matan@nvidia.com>,
"kirankumark@marvell.com" <kirankumark@marvell.com>,
"rmody@marvell.com" <rmody@marvell.com>,
"beilei.xing@intel.com" <beilei.xing@intel.com>,
"chenbo.xia@intel.com" <chenbo.xia@intel.com>,
"vburru@marvell.com" <vburru@marvell.com>,
"somnath.kotur@broadcom.com" <somnath.kotur@broadcom.com>,
"jiawenwu@trustnetic.com" <jiawenwu@trustnetic.com>,
"skori@marvell.com" <skori@marvell.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
"heinrich.kuhn@corigine.com" <heinrich.kuhn@corigine.com>,
"asomalap@amd.com" <asomalap@amd.com>,
"andrew.rybchenko@oktetlabs.ru" <andrew.rybchenko@oktetlabs.ru>,
"yongwang@vmware.com" <yongwang@vmware.com>,
"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
"hkalra@marvell.com" <hkalra@marvell.com>,
"shaibran@amazon.com" <shaibran@amazon.com>,
"chas3@att.com" <chas3@att.com>,
"cloud.wangxiaoyun@huawei.com" <cloud.wangxiaoyun@huawei.com>,
"sthemmin@microsoft.com" <sthemmin@microsoft.com>,
"jerinj@marvell.com" <jerinj@marvell.com>,
"qiming.yang@intel.com" <qiming.yang@intel.com>,
"pnalla@marvell.com" <pnalla@marvell.com>,
NBU-Contact-Thomas Monjalon <thomas@monjalon.net>,
"mk@semihalf.com" <mk@semihalf.com>,
"srinivasan@marvell.com" <srinivasan@marvell.com>,
"mw@semihalf.com" <mw@semihalf.com>,
"keith.wiles@intel.com" <keith.wiles@intel.com>,
"xiao.w.wang@intel.com" <xiao.w.wang@intel.com>,
"xuanziyang2@huawei.com" <xuanziyang2@huawei.com>,
"mtetsuyah@gmail.com" <mtetsuyah@gmail.com>,
"qi.z.zhang@intel.com" <qi.z.zhang@intel.com>,
"g.singh@nxp.com" <g.singh@nxp.com>,
"aboyer@pensando.io" <aboyer@pensando.io>,
"steven.webster@windriver.com" <steven.webster@windriver.com>,
"evgenys@amazon.com" <evgenys@amazon.com>,
"johndale@cisco.com" <johndale@cisco.com>,
"irusskikh@marvell.com" <irusskikh@marvell.com>,
"dsinghrawat@marvell.com" <dsinghrawat@marvell.com>,
"shshaikh@marvell.com" <shshaikh@marvell.com>,
"lironh@marvell.com" <lironh@marvell.com>,
Slava Ovsiienko <viacheslavo@nvidia.com>,
"aman.deep.singh@intel.com" <aman.deep.singh@intel.com>,
"sachin.saxena@oss.nxp.com" <sachin.saxena@oss.nxp.com>,
"rahul.lakkireddy@chelsio.com" <rahul.lakkireddy@chelsio.com>,
"matt.peters@windriver.com" <matt.peters@windriver.com>,
"jianwang@trustnetic.com" <jianwang@trustnetic.com>,
"skoteshwar@marvell.com" <skoteshwar@marvell.com>,
"zr@semihalf.com" <zr@semihalf.com>,
NBU-Contact-longli <longli@microsoft.com>,
"jingjing.wu@intel.com" <jingjing.wu@intel.com>,
"igorch@amazon.com" <igorch@amazon.com>,
"grive@u256.net" <grive@u256.net>,
"haiyue.wang@intel.com" <haiyue.wang@intel.com>,
"jgrajcia@cisco.com" <jgrajcia@cisco.com>,
"hyonkim@cisco.com" <hyonkim@cisco.com>,
"ndabilpuram@marvell.com" <ndabilpuram@marvell.com>
Subject: Re: [dpdk-dev] [PATCH v5 2/2] ethdev: change queue release callback
Date: Wed, 22 Sep 2021 09:35:45 +0000 [thread overview]
Message-ID: <0007ffea2d8fd0eaa02bda6db5764fd057e909d1.camel@nvidia.com> (raw)
In-Reply-To: <5dbc3817-924b-9bdd-5176-f971ed63c493@intel.com>
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 <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.
>
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) <humin29@huawei.com>
@Yisen Zhuang <yisen.zhuang@huawei.com>
@Lijun Ou <oulijun@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 <spinler@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.
next prev parent reply other threads:[~2021-09-22 9:35 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-27 3:41 [dpdk-dev] [RFC] " Xueming Li
2021-07-28 7:40 ` Andrew Rybchenko
2021-08-09 14:39 ` Singh, Aman Deep
2021-08-09 15:31 ` Ferruh Yigit
2021-08-10 8:03 ` Xueming(Steven) Li
2021-08-10 8:54 ` Ferruh Yigit
2021-08-10 9:07 ` Xueming(Steven) Li
2021-08-11 11:57 ` Ferruh Yigit
2021-08-11 12:13 ` Xueming(Steven) Li
2021-08-12 14:29 ` Xueming(Steven) Li
2021-09-26 11:25 ` Xueming(Steven) Li
2021-08-11 13:45 ` [dpdk-dev] [PATCH v1] " Xueming Li
2021-09-15 13:02 ` [dpdk-dev] [PATCH v2] " Xueming Li
2021-09-15 13:36 ` Xueming(Steven) Li
2021-09-16 8:09 ` Thomas Monjalon
2021-09-16 15:43 ` Xueming(Steven) Li
2021-09-16 15:50 ` Thomas Monjalon
2021-09-17 9:40 ` Xueming(Steven) Li
2021-09-17 9:39 ` [dpdk-dev] [PATCH v3 0/2] " Xueming Li
2021-09-17 9:39 ` [dpdk-dev] [PATCH v3 1/2] ethdev: queue release callback optional Xueming Li
2021-09-17 11:29 ` Andrew Rybchenko
2021-09-17 11:53 ` Andrew Rybchenko
2021-09-17 14:33 ` Xueming(Steven) Li
2021-09-17 9:39 ` [dpdk-dev] [PATCH v3 2/2] ethdev: change queue release callback Xueming Li
2021-09-17 11:49 ` Andrew Rybchenko
2021-09-17 14:31 ` Xueming(Steven) Li
2021-09-17 14:28 ` [dpdk-dev] [PATCH v4 0/2] " Xueming Li
2021-09-17 14:28 ` [dpdk-dev] [PATCH v4 1/2] ethdev: make queue release callback optional Xueming Li
2021-09-18 6:44 ` Andrew Rybchenko
2021-10-05 22:00 ` Thomas Monjalon
2021-09-17 14:28 ` [dpdk-dev] [PATCH v4 2/2] ethdev: change queue release callback Xueming Li
2021-09-18 6:50 ` Andrew Rybchenko
2021-09-18 12:39 ` Xueming(Steven) Li
2021-09-18 12:35 ` [dpdk-dev] [PATCH v5 0/2] " Xueming Li
2021-09-18 12:35 ` [dpdk-dev] [PATCH v5 1/2] ethdev: make queue release callback optional Xueming Li
2021-09-21 16:23 ` Ferruh Yigit
2021-09-18 12:35 ` [dpdk-dev] [PATCH v5 2/2] ethdev: change queue release callback Xueming Li
2021-09-21 18:13 ` Ferruh Yigit
2021-09-22 9:35 ` Xueming(Steven) Li [this message]
2021-09-22 10:57 ` Ferruh Yigit
2021-09-22 12:54 ` Xueming(Steven) Li
2021-09-29 13:57 ` Xueming(Steven) Li
2021-10-05 16:38 ` Ferruh Yigit
2021-10-06 7:55 ` Xueming(Steven) Li
2021-10-06 8:04 ` Ferruh Yigit
2021-10-06 11:19 ` Xueming(Steven) Li
[not found] ` <2d2e9329b076c022418efd7b38ff280cf3ed1af4.camel@nvidia.com>
[not found] ` <56f7537a-bfc0-e4b8-72e8-c382ef0e2dbd@huawei.com>
[not found] ` <8e2c2f96265dc17af0564befb3918f1a8ea5154a.camel@nvidia.com>
2021-09-29 14:04 ` [dpdk-dev] Fwd: " Xueming(Steven) Li
2021-09-30 15:17 ` [dpdk-dev] [PATCH v6 0/2] " Xueming Li
2021-09-30 15:17 ` [dpdk-dev] [PATCH v6 1/2] ethdev: make queue release callback optional Xueming Li
2021-10-05 22:04 ` Thomas Monjalon
2021-09-30 15:17 ` [dpdk-dev] [PATCH v6 2/2] ethdev: change queue release callback Xueming Li
2021-10-03 7:38 ` Matan Azrad
2021-10-03 21:00 ` Ajit Khaparde
2021-10-06 10:21 ` Somnath Kotur
2021-10-06 11:18 ` [dpdk-dev] [PATCH v7 0/2] " Xueming Li
2021-10-06 11:18 ` [dpdk-dev] [PATCH v7 1/2] ethdev: make queue release callback optional Xueming Li
2021-10-06 15:38 ` Hemant Agrawal
2021-10-08 8:16 ` Xu, Rosen
2021-10-06 11:18 ` [dpdk-dev] [PATCH v7 2/2] ethdev: change queue release callback Xueming Li
2021-10-06 17:20 ` Ferruh Yigit
2021-10-11 8:28 ` Thomas Monjalon
2021-10-11 13:11 ` Ferruh Yigit
2021-10-06 17:25 ` [dpdk-dev] [PATCH v7 0/2] " Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0007ffea2d8fd0eaa02bda6db5764fd057e909d1.camel@nvidia.com \
--to=xuemingl@nvidia.com \
--cc=aboyer@pensando.io \
--cc=ajit.khaparde@broadcom.com \
--cc=aman.deep.singh@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=asomalap@amd.com \
--cc=beilei.xing@intel.com \
--cc=chas3@att.com \
--cc=chenbo.xia@intel.com \
--cc=cloud.wangxiaoyun@huawei.com \
--cc=dev@dpdk.org \
--cc=dsinghrawat@marvell.com \
--cc=evgenys@amazon.com \
--cc=ferruh.yigit@intel.com \
--cc=g.singh@nxp.com \
--cc=grive@u256.net \
--cc=haiyue.wang@intel.com \
--cc=heinrich.kuhn@corigine.com \
--cc=hemant.agrawal@nxp.com \
--cc=hkalra@marvell.com \
--cc=humin29@huawei.com \
--cc=hyonkim@cisco.com \
--cc=igorch@amazon.com \
--cc=irusskikh@marvell.com \
--cc=jerinj@marvell.com \
--cc=jgrajcia@cisco.com \
--cc=jianwang@trustnetic.com \
--cc=jiawenwu@trustnetic.com \
--cc=jingjing.wu@intel.com \
--cc=johndale@cisco.com \
--cc=keith.wiles@intel.com \
--cc=kirankumark@marvell.com \
--cc=lironh@marvell.com \
--cc=longli@microsoft.com \
--cc=matan@nvidia.com \
--cc=matt.peters@windriver.com \
--cc=maxime.coquelin@redhat.com \
--cc=mczekaj@marvell.com \
--cc=mk@semihalf.com \
--cc=mtetsuyah@gmail.com \
--cc=mw@semihalf.com \
--cc=ndabilpuram@marvell.com \
--cc=oulijun@huawei.com \
--cc=pnalla@marvell.com \
--cc=qi.z.zhang@intel.com \
--cc=qiming.yang@intel.com \
--cc=radhac@marvell.com \
--cc=rahul.lakkireddy@chelsio.com \
--cc=rmody@marvell.com \
--cc=sachin.saxena@oss.nxp.com \
--cc=shaibran@amazon.com \
--cc=shshaikh@marvell.com \
--cc=skori@marvell.com \
--cc=skoteshwar@marvell.com \
--cc=somnath.kotur@broadcom.com \
--cc=spinler@cesnet.cz \
--cc=srinivasan@marvell.com \
--cc=steven.webster@windriver.com \
--cc=sthemmin@microsoft.com \
--cc=sthotton@marvell.com \
--cc=thomas@monjalon.net \
--cc=vburru@marvell.com \
--cc=viacheslavo@nvidia.com \
--cc=xiao.w.wang@intel.com \
--cc=xuanziyang2@huawei.com \
--cc=yisen.zhuang@huawei.com \
--cc=yongwang@vmware.com \
--cc=zhouguoyang@huawei.com \
--cc=zr@semihalf.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).