Re: [PATCH 4/7] tracing: account ring buffer pages and trace_array metadata to memcg

[Tao Cui <cui.tao@linux.dev>]

Hi,

在 2026/11/7 21:27, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [Medium] tracing: alloc_cpu_data() mischarges global_trace and
>   read-page swaps
Correct.  Charging alloc_cpu_data() unconditionally pins the global
trace buffers to whoever first triggers the lazy expansion, and the
read-page swap can move charges between the reader's memcg and the
ring buffer.
> - [Medium] tracing: alloc_cpu_data() memcg limit triggers
>   trace_rb_cpu_prepare() WARN
Also possible.  With __GFP_ACCOUNT, allocation failures from memcg
limits can now reach this path as well, making the existing WARN()
reachable from a constrained memcg.  I'll avoid charging allocations
performed from the hotplug prepare path in the respin.
> - [Low] tracing: inaccurate snapshot size claim and inconsistent
>   GFP_KERNEL_ACCOUNT use
The snapshot size description in the changelog will be corrected in the
respin.
> It also noticed 1 related pre-existing issue this patch does not
> introduce:
> - [Medium] tracing: memory leak in trace_array_create_systems() [1]
That one predates this series as well.  I'll investigate it separately
and send a dedicated fix if confirmed.

The v2 respin narrows the accounting.  Only buffers belonging to
non-root trace instances will be allocated with __GFP_ACCOUNT (via a
new RB_FL_ACCOUNT flag).  The global trace, read-page pool and CPU
hotplug allocations will remain unaccounted, and the changelog will be
updated accordingly.

Thank
> --
> 
> commit a11da3138f9fda850c1b05bae82b0be7e738b994
> Author: Tao Cui <cuitao@kylinos.cn>
> 
> tracing: account ring buffer pages and trace_array metadata to memcg
> 
> This commit adds __GFP_ACCOUNT to ring buffer subbuffer allocations in
> alloc_cpu_data() and switches trace_array allocations in
> trace_array_create_systems() to GFP_KERNEL_ACCOUNT.
> 
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index e760d2fe386fb..f925936378639 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -441,7 +441,13 @@ static struct buffer_data_page *alloc_cpu_data(int cpu, int order)
>>  	 * gracefully without invoking oom-killer and the system is not
>>  	 * destabilized.
>>  	 */
>> -	mflags = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_COMP | __GFP_ZERO;
>> +	/*
>> +	 * The subbuffers are charged to the memcg of whoever caused the
>> +	 * allocation: the creator of a trace instance, or root at boot for
>> +	 * the global trace.
>> +	 */
>> +	mflags = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_COMP | __GFP_ZERO |
>> +		  __GFP_ACCOUNT;
>>  
>>  	page = alloc_pages_node(cpu_to_node(cpu), mflags, order);
>>  	if (!page)
> 
> [Severity: Medium]
> Does charging every alloc_cpu_data() call to current's memcg misattribute
> global_trace subbuffers and swapped read pages?
> 
> At boot, tracer_alloc_buffers() keeps global_trace at its minimum size
> (ring_buf_size = 1) when ring_buffer_expanded is false.  global_trace is
> only expanded to trace_buf_size at runtime in tracing_update_buffers() or
> tracing_set_tracer() when a task first enables an event, sets a tracer, or
> loads a module using trace_printk().  With __GFP_ACCOUNT in
> alloc_cpu_data(), those global_trace pages are charged to whichever cgroup
> triggers the lazy expansion and stay pinned to that memcg because
> global_trace is never freed.
> 
> In addition, ring_buffer_alloc_read_page() calls alloc_cpu_data() when
> reading or splicing from trace_pipe_raw, and ring_buffer_read_page() swaps
> that reader-allocated page directly into reader->page while handing the old
> ring buffer page back to the reader to be freed:
> 
> kernel/trace/ring_buffer.c:ring_buffer_read_page() {
>     ...
> 		dpage = reader->page;
> 		reader->page = data_page->data;
> 		local_set(&reader->write, 0);
> 		local_set(&reader->entries, 0);
> 		reader->read = 0;
> 		data_page->data = dpage;
>     ...
> }
> 
> A host process reading trace_pipe_raw from a container's instance will
> replace the container's charged pages with uncharged pages and uncharge the
> container as the old pages are freed, while a cgroup task reading
> global_trace will swap pages charged to its own memcg into global_trace.
> Similarly, trace_rb_cpu_prepare() during runtime CPU hotplug allocates
> per-CPU buffers for all existing instances in the context of the task
> onlining the CPU.
> 
> [Severity: Medium]
> Can hitting the memcg limit in alloc_cpu_data() during instance creation
> trigger the WARN(1) in trace_rb_cpu_prepare()?
> 
> When alloc_buffer() creates a ring buffer, it allocates the first CPU's
> buffer directly and then calls
> cpuhp_state_add_instance(CPUHP_TRACE_RB_PREPARE, &buffer->node), which runs
> trace_rb_cpu_prepare() for each remaining present CPU:
> 
> kernel/trace/ring_buffer.c:trace_rb_cpu_prepare() {
>     ...
> 	buffer->buffers[cpu] =
> 		rb_allocate_cpu_buffer(buffer, nr_pages, cpu);
> 	if (!buffer->buffers[cpu]) {
> 		WARN(1, "failed to allocate ring buffer on CPU %u\n",
> 		     cpu);
> 		return -ENOMEM;
> 	}
>     ...
> }
> 
> Because __rb_allocate_pages() only checks si_mem_available() against global
> memory, a cgroup with enough headroom for the first CPU (~1.375 MB) that
> hits its memcg limit on any subsequent CPU in trace_rb_cpu_prepare() will
> trigger this WARN(1) and crash the host if panic_on_warn is set.
> 
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 08614af8fad1c..fd1cfd1f66fbe 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -8677,11 +8677,11 @@ trace_array_create_systems(const char *name, const char *systems,
>>  	int ret;
>>  
>>  	ret = -ENOMEM;
>> -	tr = kzalloc_obj(*tr);
>> +	tr = kzalloc_obj(*tr, GFP_KERNEL_ACCOUNT);
>>  	if (!tr)
>>  		return ERR_PTR(ret);
>>  
>> -	tr->name = kstrdup(name, GFP_KERNEL);
>> +	tr->name = kstrdup(name, GFP_KERNEL_ACCOUNT);
>>  	if (!tr->name)
>>  		goto out_free_tr;
>>  
>> @@ -8692,7 +8692,7 @@ trace_array_create_systems(const char *name, const char *systems,
>>  		goto out_free_tr;
>>  
>>  	if (systems) {
>> -		tr->system_names = kstrdup_const(systems, GFP_KERNEL);
>> +		tr->system_names = kstrdup_const(systems, GFP_KERNEL_ACCOUNT);
>>  		if (!tr->system_names)
>>  			goto out_free_tr;
>>  	}
> 
> [Severity: Low]
> Should the alloc_cpumask_var() and zalloc_cpumask_var() calls right between
> tr->name and tr->system_names, as well as alloc_cpu_page() and
> alloc_cpu_buffer() in ring_buffer.c, also use GFP_KERNEL_ACCOUNT?
> 
> Also, the commit message says creating an instance immediately allocates a
> full ~1.375 MB per-CPU snapshot buffer, but trace_allocate_snapshot() passes
> size = 1 when allocate_snapshot is false (which is always the case after
> boot), so snapshot_buffer only allocates the minimum pages until a snapshot
> is armed.
> 
> [Severity: Medium]
> This problem wasn't introduced by this patch, but in
> trace_array_create_systems() [1], does the out_free_tr error path leak
> tr->tracers when trace_array_create_dir() fails?
> 
> trace_array_create_dir() calls __update_tracer() -> add_tracer(), which
> allocates struct tracers and struct tracer_flags entries on tr->tracers.  If
> a later step in trace_array_create_dir() fails and jumps to out_free_tr,
> nothing calls free_tracers(tr), leaking the entries already added to
> tr->tracers.
> 
> [1] https://sashiko.dev/#/bug/linux-9449d18e-1233-4181-9199-09eaead7b576
>