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

[Andrii Nakryiko]

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
>