From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH 00/15] tracepoint: Avoid double static_branch evaluation at guarded call sites Date: Fri, 13 Mar 2026 14:04:52 +1000 Message-ID: In-Reply-To: <1e3c2830-765e-4271-89f7-0b6784b37597@efficios.com> References: <1e3c2830-765e-4271-89f7-0b6784b37597@efficios.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: Re: [PATCH 00/15] tracepoint: Avoid double static_branch evaluatio= n at guarded call sites Author: Mathieu Desnoyers Patches: 18 Reviewed: 2026-03-13T14:04:52.638511 --- This is a well-motivated, clean optimization series. The core idea =E2=80= =94 avoiding a redundant `static_branch_unlikely()` evaluation at tracepoin= t call sites that are already guarded by `trace_foo_enabled()` =E2=80=94 is= sound and was suggested by both Steven Rostedt and Peter Zijlstra. The imp= lementation is straightforward: a new `trace_invoke_##name()` API that call= s `__do_trace_##name()` directly, bypassing the static branch, while preser= ving the LOCKDEP RCU-watching assertion and `might_fault()` for syscall tra= cepoints. The filtered patches touching DRM/accel/dma-buf subsystems (patches 05, 08,= 10) are all mechanical, correct conversions. The series is low-risk =E2=80= =94 it only changes behavior on hot paths when tracing is already enabled, = and the functional semantics are preserved. **One design concern** with patch 01 (the API patch): the new `trace_invoke= _##name()` does **not** include the LOCKDEP `rcu_is_watching()` assertion t= hat the normal `trace_##name()` path has. Looking at the implementation: ```c static inline void trace_invoke_##name(proto) { __do_trace_##name(args); } ``` The normal `trace_##name()` includes: ```c if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { WARN_ONCE(!rcu_is_watching(), "RCU not watching for tracepoint"); } ``` The commit message for patch 01 says "retains the LOCKDEP RCU-watching asse= rtion", but looking at the actual diff, **the assertion is NOT present** in= `trace_invoke_##name()`. This is misleading. In practice it may be accepta= ble (the caller has already passed through `trace_foo_enabled()` which impl= ies the tracepoint infrastructure is active, and `__do_trace_##name()` uses= `guard(srcu_fast_notrace)` internally), but the commit message should accu= rately describe the behavior. Steven Rostedt should confirm whether omittin= g this assertion is intentional. --- --- Generated by Claude Code Patch Reviewer