On Mon, 2 Sept 2024 at 11:08, Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> If Linus' objections were mainly about naming, perhaps what I am
> suggestion here may be more to his liking ?
Most of my objections were about having subtle features that then had
very subtle syntax and weren't that clear.
And yes, a small part of it was naming (I absolutely hated the initial
"everything is a guard" thing, when one of the main uses were about
automatic freeing of variables).
But a much larger part was about making the new use greppable and have
really simple syntax.
And the conditional case was never that simple syntax, and I feel it
really violates the whole "this scope is protected".
And no, I do not like Peter's "if_guard()" either.
Honestly, I get the feeling that this is all wrong.
For example, I searched for a few of the places where you wanted to
use this, and see code like this:
#define __BPF_DECLARE_TRACE(call, proto, args, tp_flags) \
static notrace void \
__bpf_trace_##call(void *__data, proto) \
{ \
DEFINE_INACTIVE_GUARD(preempt_notrace, bpf_trace_guard); \
\
if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
might_fault(); \
activate_guard(preempt_notrace, bpf_trace_guard)(); \
} \
\
CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data,
CAST_TO_U64(args)); \
}
and it makes me just go "that's just *WRONG*".
That code should never EVER use a conditional guard.
I find *two* uses of this in your patches, and both of them look
com,pletely wrong to me, because you could have written the code
*without* that conditional activation, and it would have been *better*
that way.
IOW, that code should just have been something like this:
#define __BPF_DECLARE_TRACE(call, proto, args, tp_flags) \
static notrace void \
__bpf_trace_##call(void *__data, proto) \
{ \
\
if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \
might_fault(); \
guard(preempt_notrace)(); \
CONCATENATE(bpf_trace_run, ... \
return; \
} \
CONCATENATE(bpf_trace_run, ... \
}
instead.
Sure, it duplicates the code inside the guard, but what's the
downside? In both of the cases I saw, the "duplicated" code was
trivial.
And the *upside* is that you don't have any conditional locking or
guarding, and you don't have to make up ridiculous and meaningless
temporary names for the guard etc. So you get to use the *LEGIBLE*
code.
And you don't have a patch that just renames all our existing uses.
Which was also wrong.
So no, I don't like Peter's "if_guard()", but I find your conditional
activation to be absolutely wrogn and horrible too.
Linus
