Hi, 在 2026/11/7 21:27, [email protected] 写道: > 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 <[email protected]> > > 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 >
