On 2024-10-28 01:06, Steven Rostedt wrote:
On Sun, 27 Oct 2024 08:30:54 -0400
Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/
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
