Re: [RFC PATCH v3 2/3] tracing: Introduce tracepoint_is_syscall()

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

On 2024-10-28 01:06, Steven Rostedt wrote:
> On Sun, 27 Oct 2024 08:30:54 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>>
>>> I wonder if we should call it "sleepable" instead? For this patch set
>>> do we really care if it's a system call or not? It's really if the
>>> tracepoint is sleepable or not that's the issue. System calls are just
>>> one user of it, there may be more in the future, and the changes to BPF
>>> will still be needed.
>>
>> Remember that syscall tracepoint probes are allowed to handle page
>> faults, but should not generally block, otherwise it would postpone the
>> grace periods of all RCU tasks trace users.
>>
>> So naming this "sleepable" would be misleading, because probes are
>> not allowed general blocking, just to handle page faults.
> 
> I'm fine with "faultable" too.
> 
>>
>> If we look at the history of this tracepoint feature, we went with
>> the following naming over the various versions of the patch series:
>>
>> 1) Sleepable tracepoints: until we understood that we just want to
>>      allow page fault, not general sleeping, so we needed to change
>>      the name,
>>
>> 2) Faultable tracepoints: until Linus requested that we aim for
>>      something that is specific to system calls, rather than a generic
>>      thing.
>>
>>      https://lore.kernel.org/lkml/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com/
> 
> Reading that thread again, I believe that Linus was talking more about
> all the infrastructure going around to make a special "faultable"
> tracepoint (I could be wrong, and Linus may correct me here). When in
> fact, the only user is system calls. But from the BPF POV, it doesn't
> care if it's a system call, it cares that it is faultable, and the
> check should be on that. Having BPF check if it's a system call is
> requiring that BPF knows the implementation details of system call
> tracepoints. But if it knows it is faultable, then it needs to do
> something special.

It might just be that, indeed. Considering the overwhelming preference
for something a little more general (sleepable/faultable vs syscall),
I am tempted to go for "tracepoint_is_faultable()".

> 
>>
>> 3) Syscall tracepoints: This is what we currently have.
>>
>>> Other than that, I think this could work.
>>
>> Calling this field "sleepable" would be misleading. Calling it "faultable"
>> would be a better fit, but based on Linus' request, I'm tempted to stick
>> with "syscall" for now.
>>
>> Your concern is to name this in a way that is general and future-proof.
>> Linus' point was to make it syscall-specific rather than general. My
>> position is that we should wait until we face other use-cases (if we
>> even do) before consider changing the naming from "syscall" to something
>> more generic.
> 
> Yes, but that was for the infrastructure itself. It really doesnt' make
> sense that BPF needs to know which type of tracepoint can fault. That's
> telling BPF, you need to know the implementation of this type of
> tracepoint.

OK, I'll use tracepoint_is_faultable() and a "faultable" field name, and
see how people react. I really prefer "faultable" to "sleepable" here,
because I envision that in the future we may introduce tracepoints
which are really able to sleep (general blocking), for instance though
use of hazard pointers to protect a list iteration. (if there is ever
a need for it)

Thanks,

Mathieu

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