| Message ID | 20240828143719.828968-3-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cleanup.h: Introduce DEFINE_INACTIVE_GUARD()/activate_guard() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
On Wed, Aug 28, 2024 at 10:37:19AM -0400, Mathieu Desnoyers wrote: > To cover scenarios where the scope of the guard differs from the scope > of its activation, introduce DEFINE_INACTIVE_GUARD() and activate_guard(). > > Here is an example use for a conditionally activated guard variable: > > void func(bool a) > { > DEFINE_INACTIVE_GUARD(preempt_notrace, myguard); > > [...] > if (a) { > might_sleep(); > activate_guard(preempt_notrace, myguard)(); > } > [ protected code ] > } So... I more or less proposed this much earlier: https://lore.kernel.org/all/20230919131038.GC39346@noisy.programming.kicks-ass.net/T/#mb7b84212619ac743dfe4d2cc81decce451586b27 and Linus took objection to similar patterns. But perhaps my naming wasn't right.
On 2024-09-02 11:43, Peter Zijlstra wrote: > On Wed, Aug 28, 2024 at 10:37:19AM -0400, Mathieu Desnoyers wrote: >> To cover scenarios where the scope of the guard differs from the scope >> of its activation, introduce DEFINE_INACTIVE_GUARD() and activate_guard(). >> >> Here is an example use for a conditionally activated guard variable: >> >> void func(bool a) >> { >> DEFINE_INACTIVE_GUARD(preempt_notrace, myguard); >> >> [...] >> if (a) { >> might_sleep(); >> activate_guard(preempt_notrace, myguard)(); >> } >> [ protected code ] >> } > > So... I more or less proposed this much earlier: > > https://lore.kernel.org/all/20230919131038.GC39346@noisy.programming.kicks-ass.net/T/#mb7b84212619ac743dfe4d2cc81decce451586b27 > > and Linus took objection to similar patterns. But perhaps my naming > wasn't right. Then you suggested something like a "guard_if()": https://lore.kernel.org/lkml/20231120221524.GD8262@noisy.programming.kicks-ass.net/ which I find really odd because it requires to evaluate the same condition twice within the function if it is used as guard_if expression and needed as expression within the rest of the function flow. I find the original patch with labels and gotos less ugly than the guard_if(). Hence my proposal to optionally separate the definition from the activation, which nicely integrates with the existing code flow. If Linus' objections were mainly about naming, perhaps what I am suggestion here may be more to his liking ? Thanks, Mathieu
On Mon, 2 Sept 2024 at 08:43, Peter Zijlstra <peterz@infradead.org> wrote: > > and Linus took objection to similar patterns. But perhaps my naming > wasn't right. Well, more of a "this stuff is new, let's start very limited and very clear". I'm not loving the inactive guard, but I did try to think of a better model for it, and I can't. I absolutely hate the *example*, though: void func(bool a) { DEFINE_INACTIVE_GUARD(preempt_notrace, myguard); [...] if (a) { might_sleep(); activate_guard(preempt_notrace, myguard)(); } [ protected code ] } because that "protected code" obviously is *NOT* protected code. It's conditionally protected only in one situation. Honestly, I still think the guard macros are new enough that we should strive to avoid them in complicated cases like this. And this *is* complicated. It *looks* simple, but when even the example that was given was pure and utter garbage, it's clearly not *actually* simple. Once some code is sometimes protected, and sometimes isn't, and you have magic compiler stuff that *hides* it, I'm not sure we should use the magic compiler stuff. Linus
On 2024-09-02 14:10, Linus Torvalds wrote: > On Mon, 2 Sept 2024 at 08:43, Peter Zijlstra <peterz@infradead.org> wrote: >> >> and Linus took objection to similar patterns. But perhaps my naming >> wasn't right. > > Well, more of a "this stuff is new, let's start very limited and very clear". > > I'm not loving the inactive guard, but I did try to think of a better > model for it, and I can't. I absolutely hate the *example*, though: > > void func(bool a) > { > DEFINE_INACTIVE_GUARD(preempt_notrace, myguard); > > [...] > if (a) { > might_sleep(); > activate_guard(preempt_notrace, myguard)(); > } > [ protected code ] Fair. I should have written something more like [ conditionally protected code ] > } > > because that "protected code" obviously is *NOT* protected code. It's > conditionally protected only in one situation. > > Honestly, I still think the guard macros are new enough that we should > strive to avoid them in complicated cases like this. And this *is* > complicated. It *looks* simple, but when even the example that was > given was pure and utter garbage, it's clearly not *actually* simple. > > Once some code is sometimes protected, and sometimes isn't, and you > have magic compiler stuff that *hides* it, I'm not sure we should use > the magic compiler stuff. I've tried my best to come up with a scheme which would be cleaner than the "guard_if()" proposed by Peter, because I really hate it. I'm perfectly fine going back to goto/labels for that function if we cannot agree on a clean way to express what is needed there. Thanks, Mathieu
On Mon, 2 Sept 2024 at 11:08, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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
On 2024-09-02 14:46, Linus Torvalds wrote: [...] > 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. If we look at perf_trace_##call(), with the conditional guard, it looks like the following. It is not clear to me that code duplication would be acceptable here. I agree with you that the conditional guard is perhaps not something we want at this stage, but in this specific case perhaps we should go back to goto and labels ? One alternative is to add yet another level of macros to handle the code duplication. #define _DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print, tp_flags) \ static notrace void \ perf_trace_##call(void *__data, proto) \ { \ struct trace_event_call *event_call = __data; \ struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\ struct trace_event_raw_##call *entry; \ struct pt_regs *__regs; \ u64 __count = 1; \ struct task_struct *__task = NULL; \ struct hlist_head *head; \ int __entry_size; \ int __data_size; \ int rctx; \ \ DEFINE_INACTIVE_GUARD(preempt_notrace, trace_event_guard); \ \ if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \ might_fault(); \ activate_guard(preempt_notrace, trace_event_guard)(); \ } \ \ __data_size = trace_event_get_offsets_##call(&__data_offsets, args); \ \ head = this_cpu_ptr(event_call->perf_events); \ if (!bpf_prog_array_valid(event_call) && \ __builtin_constant_p(!__task) && !__task && \ hlist_empty(head)) \ return; \ \ __entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\ sizeof(u64)); \ __entry_size -= sizeof(u32); \ \ entry = perf_trace_buf_alloc(__entry_size, &__regs, &rctx); \ if (!entry) \ return; \ \ perf_fetch_caller_regs(__regs); \ \ tstruct \ \ { assign; } \ \ perf_trace_run_bpf_submit(entry, __entry_size, rctx, \ event_call, __count, __regs, \ head, __task); \ } Thanks, Mathieu
On Tue, 3 Sept 2024 at 06:42, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > If we look at perf_trace_##call(), with the conditional guard, it looks > like the following. It is not clear to me that code duplication would > be acceptable here. Sure it is. Admittedly, I think it would look a *lot* better off with that macro creating a helper function for the common part, and then just call that helper function in two places. In fact, I think the existing "perf_trace_##call()" function *is* that helper function already, and all it needs is (a) add the traditional double underscore (or "do_") to that function (b) create the wrapper function. IOW, I think the diff would look something like this: --- a/include/trace/perf.h +++ b/include/trace/perf.h @@ -15,7 +15,7 @@ #undef DECLARE_EVENT_CLASS #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \ static notrace void \ -perf_trace_##call(void *__data, proto) \ +__perf_trace_##call(void *__data, proto) \ { \ struct trace_event_call *event_call = __data; \ struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\ @@ -53,6 +53,17 @@ perf_trace_##call(void *__data, proto) \ perf_trace_run_bpf_submit(entry, __entry_size, rctx, \ event_call, __count, __regs, \ head, __task); \ +} \ +static notrace void \ +perf_trace_##call(void *__data, proto) \ +{ \ + if ((tp_flags) & TRACEPOINT_MAY_FAULT) { \ + might_fault(); \ + guard(preempt_notrace)(); \ + __perf_trace_##call(__data, proto); \ + return; \ + } \ + __perf_trace_##call(__data, proto); \ } /* and notice how there is no actual duplicated code on a source level now (although the compiler may obviously do the inlining and duplication - although I think that the tp_flags are always constant, so what actually hopefully happens is that the compiler doesn't duplicate anything at all, and all the conditionals just go away.). NOTE! The above patch is garbage. I did it as a "like this" patch on my current tree, and it doesn't even have the "tp_class" thing at all, so the patch is obviously not functional. It's just meant as a "look, wrapper function". In fact, I think the wrapping might even be done at a higher level (ie maybe the whole "may_fault" shouldn't be an argument at all, but a static part of the define, in case some event classes can take faults and others cannot?) I dunno. I actually think the whole "TRACEPOINT_MAY_FAULT" thing is an incredibly ugly hack, and SHOULD NOT EXIST. In other words - why make these incredibly ugly macros EVEN MORE UGLY - when there is a single use-case and we sure as hell don't want to add any more? IOW - why not get rid of the stupid TRACE_EVENT_FN_MAY_FAULT thing entirely, and do this trivial wrapping in the *one* place that wants it, instead of making it look like some generic thing with an allegedly generic test, when it is anything but generic, and is in fact just a single special case for system call. Yes, I know. Computer Science classes say that "generic programming is good". Those CS classes are garbage. You want to make your code as specialized as humanly possible, and *not* make some complicated "this handles all cases" code that is complicated and fragile. Linus
On 2024-09-03 15:00, Linus Torvalds wrote: [...] > > IOW - why not get rid of the stupid TRACE_EVENT_FN_MAY_FAULT thing > entirely, and do this trivial wrapping in the *one* place that wants > it, instead of making it look like some generic thing with an > allegedly generic test, when it is anything but generic, and is in > fact just a single special case for system call. > > Yes, I know. Computer Science classes say that "generic programming is > good". Those CS classes are garbage. You want to make your code as > specialized as humanly possible, and *not* make some complicated "this > handles all cases" code that is complicated and fragile. Thank you for looking into this. I'm redoing the series based on your feedback, and it is indeed much cleaner. Let me know if you are interested to be CC'd on the next version of the series changing syscall tracepoints. Mathieu
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h index 1247e67a6161..5f031cce97ca 100644 --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h @@ -149,12 +149,20 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ * similar to scoped_guard(), except it does fail when the lock * acquire fails. * + * DEFINE_INACTIVE_GUARD(name, var): + * define an inactive guard variable in a given scope, initialized to NULL. + * + * activate_guard(name, var)(args...): + * activate a guard variable with its constructor, if it is not already + * activated. */ #define DECLARE_GUARD(_name, _type, _lock, _unlock) \ DECLARE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \ static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \ - { return *_T; } + { return *_T; } \ + static inline class_##_name##_t class_##_name##_null(void) \ + { return NULL; } #define DECLARE_GUARD_COND(_name, _ext, _condlock) \ EXTEND_CLASS(_name, _ext, \ @@ -178,6 +186,14 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ if (!__guard_ptr(_name)(&scope)) _fail; \ else +#define DEFINE_INACTIVE_GUARD(_name, _var) \ + class_##_name##_t _var __cleanup(class_##_name##_destructor) = \ + class_##_name##_null() + +#define activate_guard(_name, _var) \ + if (!class_##_name##_lock_ptr(&(_var))) \ + _var = class_##_name##_constructor + /* * Additional helper macros for generating lock guards with types, either for * locks that don't have a native type (eg. RCU, preempt) or those that need a @@ -212,6 +228,11 @@ static inline void class_##_name##_destructor(class_##_name##_t *_T) \ static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \ { \ return _T->lock; \ +} \ +static inline class_##_name##_t class_##_name##_null(void) \ +{ \ + class_##_name##_t _t = { .lock = NULL }; \ + return _t; \ }
To cover scenarios where the scope of the guard differs from the scope of its activation, introduce DEFINE_INACTIVE_GUARD() and activate_guard(). Here is an example use for a conditionally activated guard variable: void func(bool a) { DEFINE_INACTIVE_GUARD(preempt_notrace, myguard); [...] if (a) { might_sleep(); activate_guard(preempt_notrace, myguard)(); } [ protected code ] } Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Kees Cook <keescook@chromium.org> Cc: Greg KH <gregkh@linuxfoundation.org> Cc: Sean Christopherson <seanjc@google.com> --- include/linux/cleanup.h | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)