Linux filesystem development
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox