Re: [PATCH v10 14/30] tracing: Add a trace remote module for testing
On Thu, Jan 29, 2026 at 11:34:52AM -0500, Steven Rostedt wrote:
> On Mon, 26 Jan 2026 10:44:03 +
> Vincent Donnefort wrote:
>
> > diff --git a/kernel/trace/remote_test.c b/kernel/trace/remote_test.c
> > new file mode 100644
> > index ..059127489c99
> > --- /dev/null
> > +++ b/kernel/trace/remote_test.c
> > @@ -0,0 +1,259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2025 - Google LLC
> > + * Author: Vincent Donnefort
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define REMOTE_EVENT_INCLUDE_FILE kernel/trace/remote_test_events.h
> > +#include
> > +
> > +static DEFINE_PER_CPU(struct simple_rb_per_cpu *, simple_rbs);
> > +static struct trace_buffer_desc *remote_test_buffer_desc;
> > +
> > +/*
> > + * The trace_remote lock already serializes accesses from the
> > trace_remote_callbacks.
> > + * However write_event can still race with load/unload.
> > + */
> > +static DEFINE_MUTEX(simple_rbs_lock);
> > +
> > +static int remote_test_load_simple_rb(int cpu, struct ring_buffer_desc
> > *rb_desc)
> > +{
> > + struct simple_rb_per_cpu *cpu_buffer;
> > + struct simple_buffer_page *bpages;
> > + int ret = -ENOMEM;
> > +
> > + cpu_buffer = kmalloc(sizeof(*cpu_buffer), GFP_KERNEL);
> > + if (!cpu_buffer)
> > + return ret;
> > +
> > + bpages = kmalloc_array(rb_desc->nr_page_va, sizeof(*bpages),
> > GFP_KERNEL);
> > + if (!bpages)
> > + goto err_free_cpu_buffer;
> > +
> > + ret = simple_ring_buffer_init(cpu_buffer, bpages, rb_desc);
> > + if (ret)
> > + goto err_free_bpages;
> > +
> > + scoped_guard(mutex, &simple_rbs_lock)
> > + *per_cpu_ptr(&simple_rbs, cpu) = cpu_buffer;
>
> Should there be some kind of check before blindly assigning the cpu_buffer?
> If not, what is the mutex protecting from?
This intends to order accesses with write_event_write(). trace_remote ensures
that load/unload are always balanced, so I don't believe there's anything to
test.
I could add a WARN_ON though. and probably guard(mutex)(&simple_rbs_lock) is
enough.
>
>
> > +
> > + return 0;
> > +
> > +err_free_bpages:
> > + kfree(bpages);
> > +
> > +err_free_cpu_buffer:
> > + kfree(cpu_buffer);
> > +
> > + return ret;
> > +}
> > +
> > +static void remote_test_unload_simple_rb(int cpu)
> > +{
> > + struct simple_rb_per_cpu *cpu_buffer = *per_cpu_ptr(&simple_rbs, cpu);
> > + struct simple_buffer_page *bpages;
> > +
> > + if (!cpu_buffer)
> > + return;
> > +
> > + guard(mutex)(&simple_rbs_lock);
> > +
> > + bpages = cpu_buffer->bpages;
> > + simple_ring_buffer_unload(cpu_buffer);
> > + kfree(bpages);
> > + kfree(cpu_buffer);
> > + *per_cpu_ptr(&simple_rbs, cpu) = NULL;
> > +}
> > +
> > +static struct trace_buffer_desc *remote_test_load(unsigned long size, void
> > *unused)
> > +{
> > + struct ring_buffer_desc *rb_desc;
> > + struct trace_buffer_desc *desc;
> > + size_t desc_size;
> > + int cpu, ret;
> > +
> > + if (WARN_ON(remote_test_buffer_desc))
> > + return ERR_PTR(-EINVAL);
> > +
> > + desc_size = trace_buffer_desc_size(size, num_possible_cpus());
> > + if (desc_size == SIZE_MAX) {
> > + ret = -E2BIG;
> > + goto err_unlock_cpus;
> > + }
> > +
> > + desc = kmalloc(desc_size, GFP_KERNEL);
> > + if (!desc) {
> > + ret = -ENOMEM;
> > + goto err_unlock_cpus;
> > + }
> > +
> > + ret = trace_remote_alloc_buffer(desc, desc_size, size,
> > cpu_possible_mask);
> > + if (ret)
> > + goto err_free_desc;
> > +
> > + for_each_ring_buffer_desc(rb_desc, cpu, desc) {
> > + ret = remote_test_load_simple_rb(rb_desc->cpu, rb_desc);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + remote_test_buffer_desc = desc;
> > +
> > + return remote_test_buffer_desc;
> > +
> > +err:
> > + for_each_ring_buffer_desc(rb_desc, cpu, remote_test_buffer_desc)
> > + remote_test_unload_simple_rb(rb_desc->cpu);
> > + trace_remote_free_buffer(remote_test_buffer_desc);
> > +
> > +err_free_desc:
> > + kfree(desc);
> > +
> > +err_unlock_cpus:
>
> Where was the cpus_read lock taken?
>
> > + cpus_read_unlock();
> > +
> > + return ERR_PTR(ret);
> > +}
> > +
> > +static void remote_test_unload(struct trace_buffer_desc *desc, void
> > *unused)
> > +{
> > + struct ring_buffer_desc *rb_desc;
> > + int cpu;
> > +
> > + if (WARN_ON(desc != remote_test_buffer_desc))
> > + return;
> > +
> > + for_each_ring_buffer_desc(rb_desc, cpu, desc)
> > + remote_test_unload_simple_rb(rb_desc->cpu);
> > +
> > + remote_test_buffer_desc = NULL;
> > + trace_remote_free_buffer(desc);
> > + kfree(desc);
> > +}
> > +
> > +static int remote_test_enable_tracing(bool enable, void *unused)
> > +{
> > + struct ring_buffer_desc *rb_desc;
> > + int cpu;
> > +
> > + if (!remote_test_buffer_desc)
> > + return -ENODEV;
> >
Re: [PATCH v10 14/30] tracing: Add a trace remote module for testing
>
> Where was the cpus_read lock taken?
I am pretty sure that's leftover of when I was reading some CPU mask :-(
>
> > + cpus_read_unlock();
> > +
> > + return ERR_PTR(ret);
> > +}
> > +
[...]
> > +static ssize_t
> > +write_event_write(struct file *filp, const char __user *ubuf, size_t cnt,
> > loff_t *pos)
> > +{
> > + struct remote_event_format_selftest *evt_test;
> > + struct simple_rb_per_cpu *cpu_buffer;
> > + unsigned long val;
> > + int ret;
> > +
> > + ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&simple_rbs_lock);
> > +
> > + if (!remote_event_selftest.enabled)
> > + return -ENODEV;
> > +
>
> You want a guard(preempt)(); here...
>
> > + cpu_buffer = *this_cpu_ptr(&simple_rbs);
>
> Otherwise this triggers:
>
> BUG: using smp_processor_id() in preemptible [] code: bash/1096
> caller is write_event_write+0xe0/0x230 [remote_test]
>
> -- Steve
Ha, of course! And also for a per-CPU ring-buffer, that is way better to make
sure we actually do all the writing from the same CPU even though in that case
the simple_rbs_lock protect us.
>
> > + if (!cpu_buffer)
> > + return -ENODEV;
> > +
> > + evt_test = simple_ring_buffer_reserve(cpu_buffer,
> > + sizeof(struct
> > remote_event_format_selftest),
> > + trace_clock_global());
> > + if (!evt_test)
> > + return -ENODEV;
> > +
> > + evt_test->hdr.id = REMOTE_TEST_EVENT_ID;
> > + evt_test->id = val;
> > +
> > + simple_ring_buffer_commit(cpu_buffer);
> > +
> > + return cnt;
> > +}
> > +
Re: [PATCH v10 14/30] tracing: Add a trace remote module for testing
On Mon, 26 Jan 2026 10:44:03 +
Vincent Donnefort wrote:
> diff --git a/kernel/trace/remote_test.c b/kernel/trace/remote_test.c
> new file mode 100644
> index ..059127489c99
> --- /dev/null
> +++ b/kernel/trace/remote_test.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 - Google LLC
> + * Author: Vincent Donnefort
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define REMOTE_EVENT_INCLUDE_FILE kernel/trace/remote_test_events.h
> +#include
> +
> +static DEFINE_PER_CPU(struct simple_rb_per_cpu *, simple_rbs);
> +static struct trace_buffer_desc *remote_test_buffer_desc;
> +
> +/*
> + * The trace_remote lock already serializes accesses from the
> trace_remote_callbacks.
> + * However write_event can still race with load/unload.
> + */
> +static DEFINE_MUTEX(simple_rbs_lock);
> +
> +static int remote_test_load_simple_rb(int cpu, struct ring_buffer_desc
> *rb_desc)
> +{
> + struct simple_rb_per_cpu *cpu_buffer;
> + struct simple_buffer_page *bpages;
> + int ret = -ENOMEM;
> +
> + cpu_buffer = kmalloc(sizeof(*cpu_buffer), GFP_KERNEL);
> + if (!cpu_buffer)
> + return ret;
> +
> + bpages = kmalloc_array(rb_desc->nr_page_va, sizeof(*bpages),
> GFP_KERNEL);
> + if (!bpages)
> + goto err_free_cpu_buffer;
> +
> + ret = simple_ring_buffer_init(cpu_buffer, bpages, rb_desc);
> + if (ret)
> + goto err_free_bpages;
> +
> + scoped_guard(mutex, &simple_rbs_lock)
> + *per_cpu_ptr(&simple_rbs, cpu) = cpu_buffer;
Should there be some kind of check before blindly assigning the cpu_buffer?
If not, what is the mutex protecting from?
> +
> + return 0;
> +
> +err_free_bpages:
> + kfree(bpages);
> +
> +err_free_cpu_buffer:
> + kfree(cpu_buffer);
> +
> + return ret;
> +}
> +
> +static void remote_test_unload_simple_rb(int cpu)
> +{
> + struct simple_rb_per_cpu *cpu_buffer = *per_cpu_ptr(&simple_rbs, cpu);
> + struct simple_buffer_page *bpages;
> +
> + if (!cpu_buffer)
> + return;
> +
> + guard(mutex)(&simple_rbs_lock);
> +
> + bpages = cpu_buffer->bpages;
> + simple_ring_buffer_unload(cpu_buffer);
> + kfree(bpages);
> + kfree(cpu_buffer);
> + *per_cpu_ptr(&simple_rbs, cpu) = NULL;
> +}
> +
> +static struct trace_buffer_desc *remote_test_load(unsigned long size, void
> *unused)
> +{
> + struct ring_buffer_desc *rb_desc;
> + struct trace_buffer_desc *desc;
> + size_t desc_size;
> + int cpu, ret;
> +
> + if (WARN_ON(remote_test_buffer_desc))
> + return ERR_PTR(-EINVAL);
> +
> + desc_size = trace_buffer_desc_size(size, num_possible_cpus());
> + if (desc_size == SIZE_MAX) {
> + ret = -E2BIG;
> + goto err_unlock_cpus;
> + }
> +
> + desc = kmalloc(desc_size, GFP_KERNEL);
> + if (!desc) {
> + ret = -ENOMEM;
> + goto err_unlock_cpus;
> + }
> +
> + ret = trace_remote_alloc_buffer(desc, desc_size, size,
> cpu_possible_mask);
> + if (ret)
> + goto err_free_desc;
> +
> + for_each_ring_buffer_desc(rb_desc, cpu, desc) {
> + ret = remote_test_load_simple_rb(rb_desc->cpu, rb_desc);
> + if (ret)
> + goto err;
> + }
> +
> + remote_test_buffer_desc = desc;
> +
> + return remote_test_buffer_desc;
> +
> +err:
> + for_each_ring_buffer_desc(rb_desc, cpu, remote_test_buffer_desc)
> + remote_test_unload_simple_rb(rb_desc->cpu);
> + trace_remote_free_buffer(remote_test_buffer_desc);
> +
> +err_free_desc:
> + kfree(desc);
> +
> +err_unlock_cpus:
Where was the cpus_read lock taken?
> + cpus_read_unlock();
> +
> + return ERR_PTR(ret);
> +}
> +
> +static void remote_test_unload(struct trace_buffer_desc *desc, void *unused)
> +{
> + struct ring_buffer_desc *rb_desc;
> + int cpu;
> +
> + if (WARN_ON(desc != remote_test_buffer_desc))
> + return;
> +
> + for_each_ring_buffer_desc(rb_desc, cpu, desc)
> + remote_test_unload_simple_rb(rb_desc->cpu);
> +
> + remote_test_buffer_desc = NULL;
> + trace_remote_free_buffer(desc);
> + kfree(desc);
> +}
> +
> +static int remote_test_enable_tracing(bool enable, void *unused)
> +{
> + struct ring_buffer_desc *rb_desc;
> + int cpu;
> +
> + if (!remote_test_buffer_desc)
> + return -ENODEV;
> +
> + for_each_ring_buffer_desc(rb_desc, cpu, remote_test_buffer_desc)
> +
> WARN_ON(simple_ring_buffer_enable_tracing(*per_cpu_ptr(&simple_rbs,
> rb_desc->cpu),
> + enable));
> + return 0;
> +}
> +
> +static int remote_test_swap_reader_page(unsigned int cpu, void *unused)
> +{
> + struct simple_rb_per_cpu *cpu_buffer;
> +
> + if (cpu >= NR_CPUS)
> + return -EINVAL;
> +
> + cpu_buffer = *per_cpu_ptr(&simple
