Re: [PATCH v3] ring-buffer: Do not check si_mem_available() before deferred init completes

[Amit Machhiwal <amachhiw@linux.ibm.com>]

Hi Vincent,

Thanks for reviewing this version.  Please find my response inline below.

On 2026/10/23 09:20 AM, Vincent Donnefort wrote:
> On Fri, Oct 23, 2026 at 09:18:10AM +0100, Vincent Donnefort wrote:
> > On Thu, Oct 22, 2026 at 11:48:17PM +0530, Amit Machhiwal wrote:
> > > During early boot (early_trace_init() called from start_kernel()),
> > > tracing allocates initial ring buffers (temp_buffer, array_buffer, and
> > > snapshot_buffer) for CPU 0.  Later, as secondary CPUs come online during
> > > smp_init(), each CPU's ring buffer is allocated via trace_rb_cpu_prepare().
> > > 
> > > Before attempting page allocation, __rb_allocate_pages() performs a
> > > heuristic check using si_mem_available() to return early with -ENOMEM if
> > > memory appears insufficient.
> > > 
> > > However, on kernels built with CONFIG_DEFERRED_STRUCT_PAGE_INIT=y,
> > > NR_FREE_PAGES only reflects the initial non-deferred pool across the entire
> > > period from early_trace_init() through smp_init().  defer_init() leaves
> > > only a single section per node (e.g.  16 MiB with 64 KB pages) initialized
> > > up-front, and on kernels with large static binary footprints (debug
> > > configurations enabling PAGE_OWNER, DEBUG_PAGEALLOC, KFENCE, or SLUB_DEBUG),
> > > early core initialisations (SLUB caches, vmalloc, static ftrace records)
> > > consume virtually all managed pages in this pool.
> > > 
> > > si_mem_available() therefore returns a false negative (often 0-1 pages)
> > > across both SYSTEM_BOOTING (for the initial CPU 0 buffers) and
> > > SYSTEM_SCHEDULING (for secondary CPU buffers via trace_rb_cpu_prepare()),
> > > causing __rb_allocate_pages() to prematurely abort with -ENOMEM.
> > > 
> > > This failure is false: the buddy allocator is designed to replenish
> > > ZONE_NORMAL on demand via deferred_grow_zone() when an actual allocation
> > > is attempted.  Checking si_mem_available() before calling alloc_pages_node()
> > > short-circuits this on-demand growth entirely.
> > > 
> > > deferred_grow_zone() is only permanently disabled after page_alloc_init_late()
> > > completes (it spawns per-node kthreads and blocks until done), which happens
> > > inside kernel_init_freeable() — before system_state is set to
> > > SYSTEM_FREEING_INITMEM.  Skip the si_mem_available() check until
> > > SYSTEM_FREEING_INITMEM, at which point all deferred pages are initialized
> > > and si_mem_available() is accurate.
> > > 
> > > Wrap the guard in #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT so that on kernels
> > > without deferred init the original check is preserved unchanged.
> > > 
> > > Fixes: 2a872fa4e9c8 ("ring-buffer: Check if memory is available before allocation")
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Michal Suchánek <msuchanek@suse.de>
> > > Closes: https://lore.kernel.org/all/arYskzbiaNzBR9MD@kunlun.suse.cz/
> > > Link: https://lore.kernel.org/all/arYskzbiaNzBR9MD@kunlun.suse.cz/
> > > Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> > 
> > It is usually good practice to not use in-reply-to to link the series between
> > each other.

Noted, thanks.  Since the patch 1 from this series was already picked, I thought
keeping the new versions of patch 2 may make sense.  But will send v4 as a
standalone thread.

> > 
> > > ---
> > > Changes since v2:
> > >  - Rebase on top of upstream ring_buffer.c changes from Vincent Donnefort;
> > >    update threshold to i < (nr_pages << subbuf_order). (Michal Suchánek)
> > > 
> > > Changes since v1:
> > >  - Extend the guard to cover SYSTEM_SCHEDULING as well: smp_init() calls
> > >    trace_rb_cpu_prepare() -> __rb_allocate_pages() before page_alloc_init_late()
> > >    completes, so the false negative window spans both SYSTEM_BOOTING and
> > >    SYSTEM_SCHEDULING. Use system_state >= SYSTEM_FREEING_INITMEM as the correct
> > >    boundary.  (Sashiko, Vincent Donnefort)
> > >  - Wrap in #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT to preserve the original
> > >    check unchanged on non-deferred kernels. (Vincent Donnefort)
> > >  - Drop Tested-by: guard condition changed, requesting fresh test.
> > > 
> > >  kernel/trace/ring_buffer.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > index 9c4e40847e43..8e5a59a7b69b 100644
> > > --- a/kernel/trace/ring_buffer.c
> > > +++ b/kernel/trace/ring_buffer.c
> > > @@ -2463,10 +2463,27 @@ static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
> > >  		 * memory. It may not be accurate. But we don't care, we just want
> > >  		 * to prevent doing any allocation when it is obvious that it is
> > >  		 * not going to succeed.
> > > +		 *
> > > +		 * With CONFIG_DEFERRED_STRUCT_PAGE_INIT, NR_FREE_PAGES only
> > > +		 * reflects the initial non-deferred pool during early boot and
> > > +		 * throughout SMP bring-up (smp_init() runs before
> > > +		 * page_alloc_init_late() completes deferred initialization).
> > > +		 * si_mem_available() returns a false negative across this
> > > +		 * entire window while actual allocations succeed by growing the
> > > +		 * zone on demand via deferred_grow_zone(). Only re-enable the
> > > +		 * check from SYSTEM_FREEING_INITMEM onward, after
> > > +		 * page_alloc_init_late() has finished and si_mem_available() is
> > > +		 * accurate.
> > 
> > This is quite a massive comment for something simple: When
> > CONFIG_DEFERRED_STRUCT_PAGE_INIT, si_mem_available() will reflect the actual
> > available memory only after SYSTEM_FREEING_INITMEM.

Agreed, will trim it down to that one sentence.

> > 
> > >  		 */
> > >  		i = si_mem_available();
> > > +#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> > > +		if (system_state >= SYSTEM_FREEING_INITMEM &&
> > > +		    i < (nr_pages << subbuf_order))
> > > +			return -ENOMEM;
> > > +#else
> > >  		if (i < (nr_pages << subbuf_order))
> > >  			return -ENOMEM;
> > > +#endif
> > 
> > Could we avoid the double i < (nr_pages << subbuf_order) with something like
> > 
> > if (IS_ENABLED(CONFIG_DEFERRED_STRUCT_PAGE_INIT) &&
> >     system_state < SYSTEM_FREEING_INITMEM)
> >        i = LONG_MAX;
> > 
> 
> or even i = totalram_pages()?

The IS_ENABLED() pattern is cleaner, will adopt it.  However, totalram_pages()
is not safe as the sentinel here.

_totalram_pages is set by a single totalram_pages_add() call inside
memblock_free_all(), which accumulates the total non-deferred page count from
free_low_memory_core_early() and adds it in one shot.  The deferred pages freed
later by deferred_free_pages() -> __free_pages_core(MEMINIT_EARLY) take the else
branch in __free_pages_core() which only updates zone->managed_pages directly —
totalram_pages_add() is never called for them (the kernel comment there even
says "memblock adjusts totalram_pages() manually").  As a result,
totalram_pages() returns the same small initial-section value (~16 MiB on
ppc64le) throughout the entire window, and setting i = totalram_pages() would
still trigger the i < (nr_pages << subbuf_order) check falsely.

LONG_MAX is unambiguous — it unconditionally passes the threshold check
regardless of nr_pages or subbuf_order, which is exactly the intent.  Will use
that in v4.

Thanks,
Amit