On 2024-09-09 12:53, Andrii Nakryiko wrote:

On Mon, Sep 9, 2024 at 8:11 AM Mathieu Desnoyers

[...]

quoted

quoted

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@mail.gmail.com/ (local)

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

Thanks,

Mathieu


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