Re: [PATCH v2 0/4] fs/pipe: unify the page pools into a single per-pipe pool

[Mateusz Guzik]

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@xxxxxxxxxx> 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)
> 
> 
> Both tests with:
> 	Kernel: linux-next 20260623 base, production .config
> 		(no KASAN / LOCKDEP / DEBUG_* / nothing useless), baseline vs the
> 		full series, both built with clang, coexisting in grub
> 	Test:   your will-it-scale test (pipen.c), PIPEN_SIZE=32678, -t 2
> 		(1 writer + 1 reader on separate cores);
> 		performance governor, writer and reader pinned to two cores
> 
> 
> You're right that there is an extra lock/unlock in the prefill path, so
> I don't want to wave your result away -- may it be specific to SPR's
> topology/cache? 
> 
> I'll also keep trying to grab a Sapphire Rapids machine so I can run your
> will-it-scale case directly on the same uarch, and check if I can reproduce
> it..
> 


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).

diff --git a/fs/pipe.c b/fs/pipe.c
index c163b05ef970..cc4be58aeb5f 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -136,7 +136,7 @@ static bool anon_pipe_prealloc_push(struct anon_pipe_prealloc *prealloc,
  * shortfall outside the lock, then briefly take the lock to push the pages in.
  * anon_pipe_get_page() then drains the pool instead of allocating under the lock.
  */
-static void anon_pipe_prefill(struct pipe_inode_info *pipe, size_t total_len)
+static void anon_pipe_prefill_and_lock(struct pipe_inode_info *pipe, size_t total_len)
 {
 	struct page *pages[PIPE_PREALLOC_MAX];
 	unsigned int want, have, need, n = 0;
@@ -147,8 +147,10 @@ static void anon_pipe_prefill(struct pipe_inode_info *pipe, size_t total_len)
 	have = min_t(unsigned int, READ_ONCE(pipe->prealloc.count), want);
 	need = want - have;
 
-	if (!need)
+	if (!need) {
+		mutex_lock(&pipe->mutex);
 		return;
+	}
 
 	while (n < need) {
 		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_ACCOUNT);
@@ -157,14 +159,10 @@ static void anon_pipe_prefill(struct pipe_inode_info *pipe, size_t total_len)
 			break;
 		pages[n++] = page;
 	}
-	if (!n)
-		return;
 
 	mutex_lock(&pipe->mutex);
 	while (n && anon_pipe_prealloc_push(&pipe->prealloc, pages[n - 1]))
 		n--;
-	mutex_unlock(&pipe->mutex);
-
 	/*
 	 * Just flush any extra page that got affected by the TOCTOU
 	 * effect
@@ -174,14 +172,16 @@ static void anon_pipe_prefill(struct pipe_inode_info *pipe, size_t total_len)
 }
 
 /* Trim the pool down to PIPE_PREALLOC_KEEP, freeing the excess unlocked. */
-static void anon_pipe_trim_pool(struct pipe_inode_info *pipe)
+static void anon_pipe_trim_pool_and_unlock(struct pipe_inode_info *pipe)
 {
 	struct page *excess[PIPE_PREALLOC_MAX];
 	unsigned int nexcess = 0;
 
 	/* Unlocked fast path; the count is re-checked under the lock below. */
-	if (READ_ONCE(pipe->prealloc.count) <= PIPE_PREALLOC_KEEP)
+	if (READ_ONCE(pipe->prealloc.count) <= PIPE_PREALLOC_KEEP) {
+		mutex_unlock(&pipe->mutex);
 		return;
+	}
 
 	mutex_lock(&pipe->mutex);
 	while (pipe->prealloc.count > PIPE_PREALLOC_KEEP)
@@ -469,9 +469,7 @@ anon_pipe_read(struct kiocb *iocb, struct iov_iter *to)
 	}
 	if (pipe_is_empty(pipe))
 		wake_next_reader = false;
-	mutex_unlock(&pipe->mutex);
-	/* Consumed buffers may have refilled the pool; trim it back. */
-	anon_pipe_trim_pool(pipe);
+	anon_pipe_trim_pool_and_unlock(pipe);
 
 	if (wake_writer)
 		wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM);
@@ -533,8 +531,7 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
 	if (unlikely(total_len == 0))
 		return 0;
 
-	anon_pipe_prefill(pipe, total_len);
-	mutex_lock(&pipe->mutex);
+	anon_pipe_prefill_and_lock(pipe, total_len);
 
 	if (!pipe->readers) {
 		if ((iocb->ki_flags & IOCB_NOSIGNAL) == 0)
@@ -657,8 +654,7 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
 out:
 	if (pipe_is_full(pipe))
 		wake_next_writer = false;
-	mutex_unlock(&pipe->mutex);
-	anon_pipe_trim_pool(pipe);
+	anon_pipe_trim_pool_and_unlock(pipe);
 
 	/*
 	 * If we do do a wakeup event, we do a 'sync' wakeup, because we