On Tue, 30 Sep 2025 10:46:45 +0200 Peter Zijlstra <[email protected]> wrote:
> On Tue, Sep 30, 2025 at 09:18:48AM +0100, [email protected] wrote: > > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > > index 842383fbc03b..98b838591edc 100644 > > --- a/kernel/trace/trace_probe.h > > +++ b/kernel/trace/trace_probe.h > > @@ -274,19 +274,19 @@ struct event_file_link { > > static inline bool trace_probe_test_flag(struct trace_probe *tp, > > unsigned int flag) > > { > > - return !!(tp->event->flags & flag); > > + return !!(smp_load_acquire(&tp->event->flags) & flag); > > } > > > > static inline void trace_probe_set_flag(struct trace_probe *tp, > > unsigned int flag) > > { > > - tp->event->flags |= flag; > > + smp_store_release(&tp->event->flags, tp->event->flags | flag); > > } > > > > static inline void trace_probe_clear_flag(struct trace_probe *tp, > > unsigned int flag) > > { > > - tp->event->flags &= ~flag; > > + smp_store_release(&tp->event->flags, tp->event->flags & ~flag); > > } > > > I _think_ the clear one is superfluous. Is there anything that cares > about stores done before the clear when the flag is found not set? > > Also, code like: > > static int fentry_dispatcher(struct fprobe *fp, unsigned long entry_ip, > unsigned long ret_ip, struct ftrace_regs *fregs, > void *entry_data) > { > struct trace_fprobe *tf = container_of(fp, struct trace_fprobe, fp); > int ret = 0; > > if (trace_probe_test_flag(&tf->tp, TP_FLAG_TRACE)) > fentry_trace_func(tf, entry_ip, fregs); > > #ifdef CONFIG_PERF_EVENTS > if (trace_probe_test_flag(&tf->tp, TP_FLAG_PROFILE)) > ret = fentry_perf_func(tf, entry_ip, fregs); > #endif > return ret; > } > > > Will now have two barriers; where one would suffice, eg. > > flags = smp_load_acquire(&tp->event->flags); > > if (flags & TP_FLAG_TRACE) > fentry_trace_func(...); > > if (flags & TP_FLAG_PROFILE) > fentry_perf_func(...); > > Should be just fine afaict. Looks good to me. We should replace trace_probe_test_flag() with trace_probe_load_flag(). Thanks, > > > Is this something anybody cares about? -- Masami Hiramatsu (Google) <[email protected]>
