Re: [PATCH v5 3/4] tracing: add per-CPU buffer memory usage statistics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] tracing: duplicate helpers and fops in trace_buffers_memory_cpu()
--

commit eebda64960929af5cb03f9b9f28c818ce9a75fce
Author: Xiang Gao <gxxa03070307@gmail.com>

tracing: add per-CPU buffer memory usage statistics

This commit adds per-CPU ring-buffer memory usage files under
trace_stats/per_cpu/cpuN/memory_usage_kb using the same categorized output
format as the aggregate trace_stats/memory_usage_kb file.

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6036e368fe102..e7e23ff38105c 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -5898,6 +5898,60 @@ static const struct file_operations trace_mem_fops = {
>  	.release	= single_release,
>  };
>  
> +static struct trace_mem_stats trace_buffers_memory_cpu(int cpu)
> +{
> +	struct trace_mem_stats stats = {};
> +	struct trace_array *tr;
> +
> +	guard(mutex)(&trace_types_lock);
> +
> +	list_for_each_entry(tr, &ftrace_trace_arrays, list)
> +		trace_array_buffer_memory(tr, cpu, &stats);
> +
> +	/* Account for the bootstrapping temp_buffer as well. */
> +	if (temp_buffer)
> +		stats.global += ring_buffer_memory_size(temp_buffer, cpu);
> +
> +	stats.total_system = stats.global + stats.instances +
> +			     stats.snapshots + stats.remotes;
> +	stats.total_carveout = stats.persistents;
> +
> +	return stats;
> +}
> +
> +static int trace_mem_per_cpu_show(struct seq_file *m, void *v)
> +{
> +	struct trace_mem_stats stats = trace_buffers_memory_cpu((long)m->private);
> +
> +	seq_printf(m, "global: %lu\n", stats.global >> 10);
> +	seq_printf(m, "instances: %lu\n", stats.instances >> 10);
> +	seq_printf(m, "snapshots: %lu\n", stats.snapshots >> 10);
> +	seq_printf(m, "persistents: %lu\n", stats.persistents >> 10);
> +	seq_printf(m, "remotes: %lu\n", stats.remotes >> 10);
> +	seq_printf(m, "total_system: %lu\n", stats.total_system >> 10);
> +	seq_printf(m, "total_carveout: %lu\n", stats.total_carveout >> 10);
> +
> +	return 0;
> +}
> +
> +static int trace_mem_per_cpu_open(struct inode *inode, struct file *file)
> +{
> +	int ret;
> +
> +	ret = tracing_check_open_get_tr(NULL);
> +	if (ret)
> +		return ret;
> +
> +	return single_open(file, trace_mem_per_cpu_show, inode->i_private);
> +}
> +
> +static const struct file_operations trace_mem_per_cpu_fops = {
> +	.open		= trace_mem_per_cpu_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};

[Severity: Low]
As reported by Masami Hiramatsu (Google) on v4 [1], could this reuse
trace_buffers_memory(), trace_mem_show(), trace_mem_open(), and
trace_mem_fops instead of duplicating them?

In kernel/trace/trace.c, the existing aggregate implementation already
forwards inode->i_private to single_open() in trace_mem_open() and formats
the exact same fields in trace_mem_show():

kernel/trace/trace.c:trace_mem_show() {
	struct trace_mem_stats stats = trace_buffers_memory();

	seq_printf(m, "global: %lu\n", stats.global >> 10);
	...
}

kernel/trace/trace.c:trace_mem_open() {
	int ret;

	ret = tracing_check_open_get_tr(NULL);
	if (ret)
		return ret;

	return single_open(file, trace_mem_show, inode->i_private);
}

If init_trace_stats_tracefs() passes (void *)(long)RING_BUFFER_ALL_CPUS for
the aggregate memory_usage_kb file and (void *)(long)cpu for each per-CPU
file, and trace_buffers_memory(int cpu) handles both RING_BUFFER_ALL_CPUS and
a specific CPU, all four duplicated definitions can be avoided.

Also, the comment in trace_buffers_memory_cpu() describes temp_buffer as a
bootstrapping buffer, whereas trace_buffers_memory() notes that temp_buffer
is allocated in tracer_alloc_buffers() to hold temporary event data for
triggers when tracing is off.

[1] https://lore.kernel.org/r/20260924103621.734c895420a97a2d6f36332e@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105140221.44614-1-gaoxiang17@xiaomi.com?part=3