Re: [PATCH v11 07/30] tracing: Add non-consuming read to trace remotes

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Feb 04, 2026 at 06:52:08PM -0500, Steven Rostedt wrote:
> On Sat, 31 Jan 2026 13:28:25 +0000
> Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> > -static struct trace_remote_iterator *trace_remote_iter(struct trace_remote *remote, int cpu)
> > +static void __free_ring_buffer_iter(struct trace_remote_iterator *iter, int cpu)
> > +{
> > +	if (!iter->rb_iter)
> > +		return;
> 
> Hmm, can't iter->rb_iter be NULL when iter->rb_iters[] is used?

Arg yes, I missed that when I removed the union. And actually I don't think this
can be called with iter->rb_iter or iter->rb_iter NULL anymore.

> 
> > +
> > +	if (cpu != RING_BUFFER_ALL_CPUS) {
> > +		ring_buffer_read_finish(iter->rb_iter);
> > +		return;
> > +	}
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		if (iter->rb_iters[cpu])
> > +			ring_buffer_read_finish(iter->rb_iters[cpu]);
> > +	}
> > +
> > +	kfree(iter->rb_iters);
> > +}
> > +
> > +static int __alloc_ring_buffer_iter(struct trace_remote_iterator *iter, int cpu)
> > +{
> > +	if (cpu != RING_BUFFER_ALL_CPUS) {
> > +		iter->rb_iter = ring_buffer_read_start(iter->remote->trace_buffer, cpu, GFP_KERNEL);
> > +
> > +		return iter->rb_iter ? 0 : -ENOMEM;
> > +	}
> > +
> > +	iter->rb_iters = kcalloc(nr_cpu_ids, sizeof(*iter->rb_iters), GFP_KERNEL);
> > +	if (!iter->rb_iters)
> > +		return -ENOMEM;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		iter->rb_iters[cpu] = ring_buffer_read_start(iter->remote->trace_buffer, cpu,
> > +							     GFP_KERNEL);
> > +		if (!iter->rb_iters[cpu]) {
> > +			__free_ring_buffer_iter(iter, RING_BUFFER_ALL_CPUS);
> 
> For instance, we call __free_ring_buffer_iter() here, but I don't see
> iter->rb_iter being set.
> 
> -- Steve
> 
> 
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +

[...]

> > +static void *trace_next(struct seq_file *m, void *v, loff_t *pos)
> > +{
> > +	struct trace_remote_iterator *iter = m->private;
> > +
> > +	++*pos;
> > +
> > +	if (!iter || !trace_remote_iter_read_event(iter))
> > +		return NULL;
> > +
> > +	trace_remote_iter_move(iter);
> > +	iter->pos++;
> > +
> > +	return iter;
> > +}
> > +
> > +static void *trace_start(struct seq_file *m, loff_t *pos)
> > +{
> > +	struct trace_remote_iterator *iter = m->private;
> > +	loff_t i;
> > +
> 
> FYI, this is where you take locks for iteration of files.
> 
> > +	if (!iter)
> > +		return NULL;
> > +
> > +	if (!*pos) {
> > +		iter->pos = -1;
> > +		return trace_next(m, NULL, &i);
> > +	}
> > +
> > +	i = iter->pos;
> > +	while (i < *pos) {
> > +		iter = trace_next(m, NULL, &i);
> > +		if (!iter)
> > +			return NULL;
> > +	}
> > +
> > +	return iter;
> > +}
> > +
> > +static int trace_show(struct seq_file *m, void *v)
> > +{
> > +	struct trace_remote_iterator *iter = v;
> > +
> > +	trace_seq_init(&iter->seq);
> > +
> > +	if (trace_remote_iter_print_event(iter)) {
> > +		seq_printf(m, "[EVENT %d PRINT TOO BIG]\n", iter->evt->id);
> > +		return 0;
> > +	}
> > +
> > +	return trace_print_seq(m, &iter->seq);
> > +}
> > +
> > +static void trace_stop(struct seq_file *s, void *v) { }
> 
> And stop is where you release the locks.
> 
> > +
> > +static const struct seq_operations trace_sops = {
> > +	.start		= trace_start,
> > +	.next		= trace_next,
> > +	.show		= trace_show,
> > +	.stop		= trace_stop,
> > +};
> > +
> > +static int trace_open(struct inode *inode, struct file *filp)
> > +{
> > +	struct trace_remote *remote = inode->i_private;
> > +	struct trace_remote_iterator *iter = NULL;
> > +	int cpu = tracing_get_cpu(inode);
> > +	int ret;
> > +
> > +	if (!(filp->f_mode & FMODE_READ))
> > +		return 0;
> > +
> > +	guard(mutex)(&remote->lock);
> > +
> > +	iter = trace_remote_iter(remote, cpu, TRI_NONCONSUMING);
> > +	if (IS_ERR(iter))
> > +		return PTR_ERR(iter);
> 
> So if iter is bad we exit out here.
> 
> > +
> > +	ret = seq_open(filp, &trace_sops);
> > +	if (ret) {
> > +		trace_remote_iter_free(iter);
> > +		return ret;
> > +	}
> > +
> > +	if (iter)
> 
> Why test if iter exists here?

We only test IS_ERR. iter will be NULL if the buffer isn't loaded and the
userspace output would be empty. But anyway if I move the locking into
start/stop this line will go away!

> 
> > +		trace_remote_iter_read_start(iter);
> 
> But still, the above grabs locks in the open, where it can return to user
> space while still holding the locks? That's a no-no.
> 
> You can use the seq file start and stop for locking.
> 
> -- Steve
>
[...]