Re: [GIT PULL] tracing: Updates for v6.13
From: Steven Rostedt
Date: Fri Nov 22 2024 - 17:41:03 EST
On Fri, 22 Nov 2024 14:30:10 -0800
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 22 Nov 2024 at 14:12, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >
> > Hmm, if we make a __DO_TRACE_SYSCALL(), I don't think it needs to even have
> > that condition parameter.
>
> That was my point. The whole conditional - and the parameter - seems
> to be completely pointless as far as I can tell.
>
> That said, I think you can actually simplify things even further: if
> you move the TO_CONDITION() checking into the caller, you could move
> the locking there too.
>
> IOW, instead of this pattern:
>
> if (static_branch_unlikely(&__tracepoint_##name.key)) \
> __DO_TRACE(name, \
> TP_ARGS(args), \
> TP_CONDITION(cond), 0); \
>
> you could make it be something like this instead:
>
> if (static_branch_unlikely(&__tracepoint_##name.key)) \
> if (TP_CONDITION(cond)) \
> scoped_guard(preempt_notrace) \
> __DO_TRACE(name, TP_ARGS(args)); \
Hmm, I wonder why I didn't do that in the first place :-/
But doing a little git forensics, I added that with:
287050d390264 ("tracing: Add TRACE_EVENT_CONDITIONAL()")
Which goes back to December of 2010!
I have no idea what I was thinking back then :-p
>
> where __DO_TRACE() would get neither the "cond" argument _nor_ that
> locking argument, because both are just done by the two users (the
> other one would use "scoped_guard(rcu_read_trace)" of course.
IOW, remove __DO_TRACE() and just call __DO_TRACE_CALL() directly.
>
> And look, this is another reason why unconditional locking is a good
> thing: now you can use the "guard()" model for the lock, and don't
> need an explicit unlock, simplifying the code more.
>
> Of course, you want "guard(rcu_read_trace)" (for system call events)
> and "guard(preempt_notrace)" (for the regular trace event case), and
> we don't have the "notrace" versions of those guard classes yet.
>
> But adding those would literally be trivial, ie something like
>
> DEFINE_LOCK_GUARD_0(rcu_notrace,
> rcu_read_lock_notrace(), rcu_read_unlock_notrace())
>
> wouldn't that make it all look really nice?
>
> NOTE NOTE NOTE! I didn't actually try any of the above in real life,
> so I might be missing some important detail. I'm just pointing out
> that making this all unconditional and not based on random flags has
> the potential for even more cleanups.
>
> And I might have gotten the different lock names confused too.
Thanks for the analysis.
-- Steve