On Mon, 15 Jan 2024 15:37:31 +0000 Vincent Donnefort <vdonnef...@google.com> wrote:
> > > @@ -5418,6 +5446,11 @@ int ring_buffer_swap_cpu(struct trace_buffer > > > *buffer_a, > > > cpu_buffer_a = buffer_a->buffers[cpu]; > > > cpu_buffer_b = buffer_b->buffers[cpu]; > > > > > > + if (READ_ONCE(cpu_buffer_a->mapped) || READ_ONCE(cpu_buffer_b->mapped)) > > > { > > > + ret = -EBUSY; > > > + goto out; > > > + } > > > > Ah, this is not enough to stop snapshot. > > update_max_tr()@kernel/trace/trace.c > > is used for swapping all CPU buffer and it does not use this function. > > (but that should use this instead of accessing buffer directly...) > > > > Maybe we need ring_buffer_swap() and it checks all cpu_buffer does not set > > mapping bits. > > > > Thank you, > > How about instead of having ring_buffer_swap_cpu() returning -EBUSY when > mapped > we have two functions to block any mapping that trace.c could call? > No. The ring buffer logic should not care if the user of it is swapping the entire ring buffer or not. It only cares if parts of the ring buffer is being swapped or not. That's not the level of scope it should care about. If we do not want a swap to happen in update_max_tr() that's not ring_buffer.c's problem. The code to prevent that from happening should be 100% in trace.c. The trace.c code knows what's being mapped or not. The accounting to keep a mapped buffer from being swapped should stay in trace.c, and not add unnecessary implementation coupling between that and ring_buffer.c. -- Steve