On Mon, Sep 9, 2024 at 10:22 AM Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote: > > 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. > Ok, sounds good to me, we can always change that after your patch set makes it into upstream. > 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! > > Thanks, > > Mathieu > > > -- > Mathieu Desnoyers > EfficiOS Inc. > https://www.efficios.com >
