From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Feifei Wang" <Feifei.Wang2@arm.com>,
"Kamalakshitha Aligeri" <Kamalakshitha.Aligeri@arm.com>,
<Yuying.Zhang@intel.com>, <beilei.xing@intel.com>,
<olivier.matz@6wind.com>, <andrew.rybchenko@oktetlabs.ru>,
<bruce.richardson@intel.com>, <konstantin.ananyev@huawei.com>,
"Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com>
Cc: <dev@dpdk.org>, "nd" <nd@arm.com>,
"Ruifeng Wang" <Ruifeng.Wang@arm.com>, "nd" <nd@arm.com>
Subject: RE: [PATCH 1/2] net/i40e: replace put function
Date: Thu, 9 Feb 2023 12:30:30 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8771F@smartserver.smartshare.dk> (raw)
In-Reply-To: <AS8PR08MB77184EC477CA484495AC3742C8D99@AS8PR08MB7718.eurprd08.prod.outlook.com>
> From: Feifei Wang [mailto:Feifei.Wang2@arm.com]
> Sent: Thursday, 9 February 2023 11.59
>
> Hi, Morten
>
> > 发件人: Morten Brørup <mb@smartsharesystems.com>
> > 发送时间: Thursday, February 9, 2023 5:34 PM
> >
> > > From: Kamalakshitha Aligeri [mailto:kamalakshitha.aligeri@arm.com]
> > > Sent: Thursday, 9 February 2023 07.25
> > >
> > > Integrated zero-copy put API in mempool cache in i40e PMD.
> > > On Ampere Altra server, l3fwd single core's performance improves by
> 5%
> > > with the new API
> > >
> > > Signed-off-by: Kamalakshitha Aligeri
> <kamalakshitha.aligeri@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Reviewed-by: Feifei Wang <feifei.wang2@arm.com>
> > > ---
> > > Link:
> > > https://patchwork.dpdk.org/project/dpdk/patch/20221227151700.80887-
> 1-
> > > mb@smartsharesystems.com/
> > >
> > > .mailmap | 1 +
> > > drivers/net/i40e/i40e_rxtx_vec_common.h | 34
> > > ++++++++++++++++++++-----
> > > 2 files changed, 28 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/.mailmap b/.mailmap
> > > index 75884b6fe2..05a42edbcf 100644
> > > --- a/.mailmap
> > > +++ b/.mailmap
> > > @@ -670,6 +670,7 @@ Kai Ji <kai.ji@intel.com> Kaiwen Deng
> > > <kaiwenx.deng@intel.com> Kalesh AP
> > > <kalesh-anakkur.purayil@broadcom.com>
> > > Kamalakannan R <kamalakannan.r@intel.com>
> > > +Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
> > > Kamil Bednarczyk <kamil.bednarczyk@intel.com> Kamil Chalupnik
> > > <kamilx.chalupnik@intel.com> Kamil Rytarowski
> > > <kamil.rytarowski@caviumnetworks.com>
> > > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > index fe1a6ec75e..80d4a159e6 100644
> > > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > > @@ -95,17 +95,37 @@ i40e_tx_free_bufs(struct i40e_tx_queue *txq)
> > >
> > > n = txq->tx_rs_thresh;
> > >
> > > - /* first buffer to free from S/W ring is at index
> > > - * tx_next_dd - (tx_rs_thresh-1)
> > > - */
> > > + /* first buffer to free from S/W ring is at index
> > > + * tx_next_dd - (tx_rs_thresh-1)
> > > + */
> > > txep = &txq->sw_ring[txq->tx_next_dd - (n - 1)];
> > >
> > > if (txq->offloads & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) {
> > > - for (i = 0; i < n; i++) {
> > > - free[i] = txep[i].mbuf;
> > > - /* no need to reset txep[i].mbuf in vector path */
> > > + struct rte_mempool *mp = txep[0].mbuf->pool;
> > > + struct rte_mempool_cache *cache =
> > > rte_mempool_default_cache(mp, rte_lcore_id());
> > > +
> > > + if (!cache || n > RTE_MEMPOOL_CACHE_MAX_SIZE) {
> >
> > If the mempool has a cache, do not compare n to
> > RTE_MEMPOOL_CACHE_MAX_SIZE. Instead, call
> > rte_mempool_cache_zc_put_bulk() to determine if n is acceptable for
> zero-
> > copy.
> >
>
> > It looks like this patch behaves incorrectly if the cache is
> configured to be
> > smaller than RTE_MEMPOOL_CACHE_MAX_SIZE. Let's say the cache size is
> 8,
> > which will make the flush threshold 12. If n is 32, your code will
> not enter this
> > branch, but proceed to call rte_mempool_cache_zc_put_bulk(), which
> will
> > return NULL, and then you will goto done.
> >
> > Obviously, if there is no cache, fall back to the standard
> > rte_mempool_put_bulk().
>
> Agree with this. I think we ignore the case that (cache -> flushthresh
> < n < RTE_MEMPOOL_CACHE_MAX_SIZE).
>
> Our goal is that if (!cache || n > cache -> flushthresh), we can put
> the buffers
> into mempool directly.
>
> Thus maybe we can change as:
> struct rte_mempool_cache *cache = rte_mempool_default_cache(mp,
> rte_lcore_id());
> if (!cache || n > cache -> flushthresh) {
> for (i = 0; i < n ; i++)
> free[i] = txep[i].mbuf;
> if (!cache) {
> rte_mempool_generic_put;
> goto done;
> } else if {
> rte_mempool_ops_enqueue_bulk;
> goto done;
> }
> }
>
> If we can change like this?
Since I consider "flushthreshold" private to the cache structure, it shouldn't be accessed directly. If its meaning changes in the future, you will have to rewrite the PMD code again. Use the mempool API instead of accessing the mempool structures directly. (Yeah, I know the mempool and mempool cache structures are not marked as internal, and thus formally public, but I still dislike accessing their internals from outside the mempool library.)
I would change to something like:
struct rte_mempool_cache *cache;
void **cache_objs;
cache = rte_mempool_default_cache(mp, rte_lcore_id());
if (unlikely(cache == NULL))
goto fallback;
/* Try zero-copy put. */
cache_objs = rte_mempool_cache_zc_put_bulk(cache, mp, n);
if (unlikely(cache_objs == NULL))
goto fallback;
/* Zero-copy put. */
/* no need to reset txep[i].mbuf in vector path */
for (i = 0; i < n; i++)
cache_objs[i] = txep[i].mbuf;
goto done;
fallback:
/* Ordinary put. */
/* no need to reset txep[i].mbuf in vector path */
for (i = 0; i < n ; i++)
free[i] = txep[i].mbuf;
rte_mempool_generic_put(mp, free, n, cache);
goto done;
>
> >
> > > + for (i = 0; i < n ; i++)
> > > + free[i] = txep[i].mbuf;
> > > + if (!cache) {
> > > + rte_mempool_generic_put(mp, (void
> > **)free, n,
> > > cache);
> > > + goto done;
> > > + }
> > > + if (n > RTE_MEMPOOL_CACHE_MAX_SIZE) {
> > > + rte_mempool_ops_enqueue_bulk(mp, (void
> > **)free,
> > > n);
> > > + goto done;
> > > + }
> > > + }
> > > + void **cache_objs;
> > > +
> > > + cache_objs = rte_mempool_cache_zc_put_bulk(cache, mp,
> > n);
> > > + if (cache_objs) {
> > > + for (i = 0; i < n; i++) {
> > > + cache_objs[i] = txep->mbuf;
> > > + /* no need to reset txep[i].mbuf in vector
> > path
> > > */
> > > + txep++;
> > > + }
> > > }
> > > - rte_mempool_put_bulk(free[0]->pool, (void **)free, n);
> > > goto done;
> > > }
> > >
> > > --
> > > 2.25.1
> > >
next prev parent reply other threads:[~2023-02-09 11:30 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-09 6:25 Kamalakshitha Aligeri
2023-02-09 6:25 ` [PATCH 2/2] test/mempool: add zero-copy API's Kamalakshitha Aligeri
2023-02-09 9:15 ` Morten Brørup
2023-02-09 9:34 ` [PATCH 1/2] net/i40e: replace put function Morten Brørup
2023-02-09 10:58 ` 回复: " Feifei Wang
2023-02-09 11:30 ` Morten Brørup [this message]
2023-02-10 2:43 ` Feifei Wang
2023-02-10 6:54 ` [PATCH v2 0/2] Integrated mempool cache zero-copy API's Kamalakshitha Aligeri
2023-02-10 6:54 ` [PATCH v2 1/2] net/i40e: replace put function Kamalakshitha Aligeri
2023-02-10 7:28 ` Morten Brørup
2023-02-10 15:20 ` Honnappa Nagarahalli
2023-02-13 18:18 ` [PATCH v3 0/2] Integrated mempool cache zero-copy API's Kamalakshitha Aligeri
2023-02-13 18:18 ` [PATCH v3 1/2] net/i40e: replace put function Kamalakshitha Aligeri
2023-02-14 8:50 ` Morten Brørup
2023-02-17 2:02 ` Lu, Wenzhuo
2023-02-17 7:52 ` Morten Brørup
2023-02-17 10:05 ` Ferruh Yigit
2023-02-17 11:24 ` Morten Brørup
2023-02-17 14:25 ` Aaron Conole
2023-02-17 14:25 ` Aaron Conole
2023-02-13 18:18 ` [PATCH v3 2/2] test/mempool: add zero-copy API's Kamalakshitha Aligeri
2023-02-21 5:52 ` [PATCH v4 0/2] Integrated mempool cache " Kamalakshitha Aligeri
2023-02-21 5:52 ` [PATCH v4 1/2] net/i40e: replace put function Kamalakshitha Aligeri
2023-02-21 5:52 ` [PATCH v4 2/2] test/mempool: add zero-copy API's Kamalakshitha Aligeri
2023-02-10 6:54 ` [PATCH v2 " Kamalakshitha Aligeri
2023-02-10 7:33 ` Morten Brørup
2023-02-20 13:52 ` Thomas Monjalon
2023-02-21 20:18 ` Kamalakshitha Aligeri
2023-02-22 8:01 ` Thomas Monjalon
2023-02-22 8:24 ` Morten Brørup
2023-02-22 12:40 ` Thomas Monjalon
2023-02-22 16:32 ` Morten Brørup
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=98CBD80474FA8B44BF855DF32C47DC35D8771F@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=Feifei.Wang2@arm.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Kamalakshitha.Aligeri@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=Yuying.Zhang@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=beilei.xing@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@huawei.com \
--cc=nd@arm.com \
--cc=olivier.matz@6wind.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).