linux-kernel - Re: [GIT PULL] tracing: Updates for v6.13

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241122174136.18f97fb6@gandalf.local.home>
Date: Fri, 22 Nov 2024 17:41:36 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Andrii Nakryiko <andrii@...nel.org>, Colin Ian King
 <colin.i.king@...il.com>, Jeff Xie <jeff.xie@...ux.dev>, Jinjie Ruan
 <ruanjinjie@...wei.com>, Josh Poimboeuf <jpoimboe@...nel.org>, Justin Stitt
 <justinstitt@...gle.com>, Levi Yun <yeoreum.yun@....com>, Li Chen
 <chenl311@...natelecom.cn>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, Ryan Roberts <ryan.roberts@....com>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Tatsuya S
 <tatsuya.s2862@...il.com>, Uros Bizjak <ubizjak@...il.com>, Zheng Yejian
 <zhengyejian@...weicloud.com>, guoweikang <guoweikang.kernel@...il.com>
Subject: Re: [GIT PULL] tracing: Updates for v6.13

On Fri, 22 Nov 2024 14:30:10 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Fri, 22 Nov 2024 at 14:12, Steven Rostedt <rostedt@...dmis.org> 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