All of lore.kernel.org
 help / color / mirror / Atom feed
From: Breno Leitao <leitao@debian.org>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	 Christian Brauner <brauner@kernel.org>,
	oleg@redhat.com, josh@joshtriplett.org, Jan Kara <jack@suse.cz>,
	 jlayton@kernel.org, axboe@kernel.dk, shakeel.butt@linux.dev,
	 linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel-team@meta.com
Subject: Re: [PATCH v2 0/4] fs/pipe: unify the page pools into a single per-pipe pool
Date: Wed, 8 Jul 2026 08:11:03 -0700	[thread overview]
Message-ID: <ak5nXRnSSXfA2B75@gmail.com> (raw)
In-Reply-To: <rac56njg4yorvhc5oyrme6bt7ej2vs64umbqkz2np32wpjbcjj@wohli2ntub4s>

On Wed, Jul 08, 2026 at 03:18:58PM +0200, Mateusz Guzik wrote:
> On Wed, Jul 08, 2026 at 05:09:19AM -0700, Breno Leitao wrote:
> > On Tue, Jul 07, 2026 at 05:29:20PM +0200, Mateusz Guzik wrote:
> > > On Tue, Jul 7, 2026 at 5:05 PM Breno Leitao <leitao@debian.org> wrote:
> > > >
> > > > TL;DR: This simplifies the pipe code, unify the page pools, reduce the
> > > > code by 9 lines (not counting comments), and no regressions are seen in
> > > > terms of performance.
> > > >
> > > 
> > > This adds an additional acquire + release cycle on the mutex for every
> > > write which preallocates, so I don't see how that's supposed to *not*
> > > slow things down in some capacity.
> > 
> > Thanks a lot for the benchmark and the numbers -- a write-heavy pipe
> > workload with the reader and writer on separate CPUs is exactly the case
> > I wanted to make sure doesn't regress.
> > 
> > I double-checked it again with your test on different setups and page sizes I
> > don't see the regression you are seeing: the bare-metal numbers are
> > flat-to-positive at your size.
> > 
> > I couldn't get my hands on a Sapphire Rapids box easily, so the bare-metal runs
> > are on a Cooper Lake Xeon (the same class I used for the cover letter) and
> > NVIDIA Grace (arm64). Both hosts are completely idle.
> > 
> > 1) X86 test
> > 
> > CPU:  Intel(R) Xeon(R) Platinum 8321HC @ 1.40GHz (Cooper Lake)
> >       1 socket / 26 cores / 2 threads = 52 CPUs
> >       L3 35.8 MiB (1 instance), single NUMA node (0-51)
> >       max freq == base 1.40GHz (no turbo), so the clock is steady
> > 
> > Bare metal, Intel(R) Xeon(R) Platinum 8321HC, your pipen.c
> >    (writer on CPU0, reader on CPU1 -- separate physical cores, same
> >    socket, shared L3), blocking pipe, 12 x 8s per point, median MB/s:
> > 
> >      PIPEN_SIZE   baseline   patched    delta
> >        4096         2658       2531      -4.8%   (noise?)
> >        32678        3517       3527      +0.3%   (your size -- flat)
> >        65536        3072       3358      +9.3%
> > 
> >    4096 is a single-page write that barely touches the pool; the swing
> >    there is inside the run-to-run variance (sd ~100 MB/s on a ~2600
> >    median), so I read it as neutral, not a regression. At 32678/65536
> >    the variance is small (sd 18-58 MB/s), so those deltas are real.
> > 
> > 
> >  
> > 2) Arm64 test:
> > 
> >  Machine / build:
> >     - NVIDIA Grace (Neoverse-V2), 72 cores, 1 socket, no SMT,
> >       single NUMA node, ~256 GB RAM
> >     - Kernel using 64k pages.
> > 
> > Results (baseline vs patched):
> > 
> >   PIPEN_SIZE   pages   baseline   patched   delta    regime
> >      65536       1       17759     17683    -0.4%    want=1 → pool covers it, NO extra lock (no-op)
> >     131072       2       17583     19734   +12.2%    prefill + extra lock taken
> >     262144       4       18781     21017   +11.9%    prefill + extra lock taken
> >     524288       8       19061     20886    +9.6%    = pool max; == Similar to Guzik's 32678 (8 pages)
> >    1048576      16       16842     17110    +1.6%    pool overflows; == Guzik's 65536 (16 pages)
> 
> I verified the extra lock acquires *do* show up on the profile for me
> (with bpftrace -e 'kprobe:osq_lock { @[kstack()] = count(); }'), so this
> has to be leaving perf on the table.
> 
> However, looking at the diff it seems the extra acquires can be
> trivially avoided? See below (untested, consider it an illustration of
> what I mean -- if it works as is I'm fine if it gets folded into your
> patchset without credit).

That is a very nice improvement, thanks for thinking about it and proposing the
patch.

In my quick check on arm64, I haven't seen a big improvement in my Grace arm64
host, These are the numbers I got now:

  3-way comparison — arm64 Grace, 64K pages

    SIZE     pg    base  mypatch  guzik    v2/base   guzik/base   guzik/mypatch
    65536     1   17759   17683   17598     -0.4%       -0.9%        -0.5%
   131072     2   17583   19734   19953    +12.2%      +13.5%        +1.1%
   262144     4   18781   21017   20998    +11.9%      +11.8%        -0.1%
   524288     8   19061   20886   20569     +9.6%       +7.9%        -1.5%
  1048576    16   16842   17110   16920     +1.6%       +0.5%        -1.1%

So, basically just noise compared to this patch in here for that given
CPU/platform.

That said, your approach is better than what I had, and I would like to
integrate it to this patch set. Os it OK if I give a Co-developed-with:
tag?

Thanks for your help here,
--breno

  reply	other threads:[~2026-07-08 15:11 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-07 15:04 [PATCH v2 0/4] fs/pipe: unify the page pools into a single per-pipe pool Breno Leitao
2026-07-07 15:04 ` [PATCH v2 1/4] fs/pipe: move the prealloc pool to per-pipe infrastructure Breno Leitao
2026-07-07 15:04 ` [PATCH v2 2/4] fs/pipe: add per-pipe pool push, prefill and trim helpers Breno Leitao
2026-07-07 15:05 ` [PATCH v2 3/4] fs/pipe: switch the read and write paths to the per-pipe pool Breno Leitao
2026-07-07 15:05 ` [PATCH v2 4/4] fs/pipe: remove the old on-stack prealloc helpers and tmp_page[2] Breno Leitao
2026-07-07 15:29 ` [PATCH v2 0/4] fs/pipe: unify the page pools into a single per-pipe pool Mateusz Guzik
2026-07-08 12:09   ` Breno Leitao
2026-07-08 13:18     ` Mateusz Guzik
2026-07-08 15:11       ` Breno Leitao [this message]
2026-07-08 15:24         ` Mateusz Guzik
2026-07-08 15:54           ` Breno Leitao

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=ak5nXRnSSXfA2B75@gmail.com \
    --to=leitao@debian.org \
    --cc=axboe@kernel.dk \
    --cc=brauner@kernel.org \
    --cc=jack@suse.cz \
    --cc=jlayton@kernel.org \
    --cc=josh@joshtriplett.org \
    --cc=kernel-team@meta.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mjguzik@gmail.com \
    --cc=oleg@redhat.com \
    --cc=shakeel.butt@linux.dev \
    --cc=viro@zeniv.linux.org.uk \
    /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.