All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: Kai Ji <kai.ji@intel.com>
Cc: dev@dpdk.org, stable@dpdk.org,
	Bruce Richardson <bruce.richardson@intel.com>,
	Konstantin Ananyev <konstantin.ananyev@huawei.com>,
	Jie Liu <liujie5@linkdatatechnology.com>
Subject: Re: [PATCH v3] net/sxe2: replace private mempool cache bypass with rte_mbuf_raw_free_bulk
Date: Tue, 22 Sep 2026 09:24:31 -0700	[thread overview]
Message-ID: <20260922092431.0c78a054@phoenix.local> (raw)
In-Reply-To: <20260921155214.2690954-1-kai.ji@intel.com>

On Mon, 21 Sep 2026 15:52:13 +0000
Kai Ji <kai.ji@intel.com> wrote:

> The AVX-512 TX completion path directly manipulated the mempool cache
> internals (cache->objs, cache->len, cache->flushthresh) instead of using
> the mempool API.  This pattern is the same private bypass that existed in
> the Intel common TX library before it was removed by commit 062d6fe5d00d
> ("net/intel: do not bypass mbuf lib for buffer fast-free") for the same
> reason: it omits mbuf instrumentation (history marking) and reaches
> directly into mempool cache internals, including the flushthresh field
> that is now obsolete (kept only for API/ABI compatibility), making the
> private fast path fragile against mempool cache layout changes.
> 
> Replace with a single rte_mbuf_raw_free_bulk() call, matching the Intel
> common library.  The RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE contract in
> rte_ethdev.h requires the application to guarantee that per-queue all
> mbufs come from the same mempool, have refcnt == 1, and are direct;
> that documented guarantee, whose @see already points to
> rte_mbuf_raw_free_bulk(), is exactly what makes this call correct.  The
> compiler inlines the bulk-free call to eliminate the overhead
> difference.
> 
> Fixes: 0af0bdcdcf83 ("net/sxe2: add AVX512 Rx and Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
> ---

AI review had some suggestions here. They seem good:

Review: [PATCH v3] net/sxe2: replace private mempool cache bypass with
rte_mbuf_raw_free_bulk

Applied cleanly to main (6bbb7b3). common/sxe2 + net/sxe2 build clean
with -Dwerror=true, AVX512 object included.

The code change is correct. rte_mbuf_raw_free_bulk() takes the mbuf
array directly and the static_assert covers the cast. Remaining
comments are on the tags and the commit message.

Warning

  Drop "Cc: stable@dpdk.org". net/sxe2 first shipped in v26.07
  (0af0bdcdcf83 is contained in v26.07-rc2 onward); 25.11 LTS does
  not have this driver, so there is no stable branch to backport to.

  The old code was not functionally broken on current mempool:
  flushthresh is still initialized to cache->size, objs[] is still
  2 * RTE_MEMPOOL_CACHE_MAX_SIZE, and rs_thresh is capped at 64, so
  the cache invariant held. What is lost is mbuf history marking
  and debug sanity checks. That is a cleanup, not a stable fix;
  the Fixes: tag is optional.

  Commit message is too long for a 35 line deletion. Also "The
  compiler inlines the bulk-free call to eliminate the overhead
  difference" is an unsupported claim; either give throughput
  numbers or drop the sentence. Suggest:

    The AVX512 Tx free path writes directly into the mempool
    cache (objs, len, flushthresh). This skips mbuf history
    marking and depends on mempool cache internals; flushthresh
    is now obsolete.

    Use rte_mbuf_raw_free_bulk(), as done for net/intel in
    commit 062d6fe5d00d ("net/intel: do not bypass mbuf lib for
    buffer fast-free"). MBUF_FAST_FREE guarantees single pool,
    refcnt 1 and direct mbufs per queue.

Info

  The "(rs_thresh & 31) == 0" condition only existed to feed the
  32-wide unrolled AVX512 copy loop. rs_thresh is validated to
  32..64 in sxe2_txrx_vec.c, so e.g. rs_thresh=48 currently falls
  back to the per-mbuf prefree path even with MBUF_FAST_FREE set.
  With rte_mbuf_raw_free_bulk() the guard serves no purpose; drop
  it.

  No v2 -> v3 changelog below the "---".

      reply	other threads:[~2026-09-22 16:24 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-20 15:50 [dpdk-dev v1] net/sxe2: replace private mempool cache bypass with rte_mbuf_raw_free_bulk Kai Ji
2026-08-21 18:03 ` Stephen Hemminger
2026-08-27 15:10 ` [dpdk-dev v2] " Kai Ji
2026-09-21 15:52   ` [PATCH v3] " Kai Ji
2026-09-22 16:24     ` Stephen Hemminger [this message]

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=20260922092431.0c78a054@phoenix.local \
    --to=stephen@networkplumber.org \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=kai.ji@intel.com \
    --cc=konstantin.ananyev@huawei.com \
    --cc=liujie5@linkdatatechnology.com \
    --cc=stable@dpdk.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.