Re: [PATCH v6 3/5] tracing/bpf-trace: Add support for faultable tracepoints

[Mathieu Desnoyers]

On 2024-09-09 12:53, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 8:11 AM Mathieu Desnoyers
[...]
> > > I wonder if it would be better to just do this, instead of that
> > > preempt guard. I think we don't strictly need preemption to be
> > > disabled, we just need to stay on the same CPU, just like we do that
> > > for many other program types.
> >
> > I'm worried about introducing any kind of subtle synchronization
> > change in this series, and moving from preempt-off to migrate-disable
> > definitely falls under that umbrella.
> >
> > I would recommend auditing all uses of this_cpu_*() APIs to make sure
> > accesses to per-cpu data structures are using atomics and not just using
> > operations that expect use of preempt-off to prevent concurrent threads
> > from updating to the per-cpu data concurrently.
> >
> > So what you are suggesting may be a good idea, but I prefer to leave
> > this kind of change to a separate bpf-specific series, and I would
> > leave this work to someone who knows more about ebpf than me.
>
> Yeah, that's ok. migrate_disable() switch is probably going a bit too
> far too fast, but I think we should just add
> preempt_disable/preempt_enable inside __bpf_trace_run() instead of
> leaving it inside those hard to find and follow tracepoint macros. So
> maybe you can just pass a bool into __bpf_trace_run() and do preempt
> guard (or explicit disable/enable) there?

Passing an extra boolean to __bpf_trace_run would impact all tracepoints
calling into ebpf, adding an extra function argument and extra tests for
all of those. The impact may be small, but it is non-zero in both code size
and overhead, so it would not be my preferred approach.

I have modified the macros to add the guard within __bpf_trace_##call
following suggestions from Linus:

  https://lore.kernel.org/lkml/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@xxxxxxxxxxxxxx/

I'll Cc you on that version of the series.

Thanks,

Mathieu


--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com