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

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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

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

Thanks,

Mathieu


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