Re: [PATCH 1/2] fs/pipe: bulk pre-allocate pages outside pipe->mutex in anon_pipe_write

[Breno Leitao <leitao@debian.org>]

On Thu, May 21, 2026 at 07:26:48PM +0200, Mateusz Guzik wrote:
> On Thu, May 21, 2026 at 5:38 PM Breno Leitao <leitao@debian.org> wrote:
>
> I do suspect the best way forward in the long run is repopulate the
> tmp_page array if you have pages to do so.

Ack!

> > +       struct anon_pipe_prealloc prealloc = {};
> >
> 
> I forgot this bit is going to be a problem with gcc. I verified it
> emits rep stosq in place, which is going to completely unnecessarily
> slow things down especially on older uarchs. This is a known bug with
> gcc doing terrible job optimizing this.
> 
> The problem will be avoided by merely initializing the count to 0.
> which looks kind of ugly if done here, but see below.

Ack, I can do it inside anon_pipe_get_page_prealloc()

	static void anon_pipe_get_page_prealloc(struct anon_pipe_prealloc *prealloc,
						size_t total_len)
	{
		prealloc->count = 0;
		if (total_len <= PAGE_SIZE)
			return;
			...
	}

> > +       /*
> > +        * Bulk pre-allocate pages outside pipe->mutex for writes that span more
> > +        * than one full page. alloc_page() with GFP_HIGHUSER may sleep doing
> > +        * reclaim and runs memcg charging, so doing it under the mutex
> > +        * extends the critical section and stalls the reader. The merge path
> > +        * handles sub-PAGE_SIZE writes without a fresh page; single-page and
> > +        * >PIPE_PREALLOC_MAX-page writes fall back to alloc_page() under the
> > +        * mutex for the remainder.
> > +        */
> > +       if (total_len > PAGE_SIZE)
> > +               anon_pipe_get_page_prealloc(&prealloc, total_len);
> > +
> 
> I don't think this comment belongs here, it should move above the
> prealloc routine.
> 
> How about this: anon_pipe_get_page_prealloc gets called
> unconditionally and expects an uninitialized prealloc struct. For
> total_len > PAGE_SIZE you roll with the current code. Otherwise you
> just set ->count to 0, which prevents the ->pages array from being
> looked at. You can even pre-set to 0 on entry, just don't memset the
> entire obj.

Thanks, let me respin a v2.
--breno