Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] The `do_trace_refcount_final_put_cond` macro does not parenthesize its `cond` parameter, which can lead to logical errors and bypass the tracepoint static branch. - [Medium] The refcount_final_put tracepoint is dead code because no callers were added to invoke it. - [Low] Missing `<linux/export.h>` in `lib/refcount_trace.c`. - [Low] Incorrect file name referenced in comment for the trace wrapper implementation. --
--- Patch [1]: [PATCH v5 1/5] tracing: add refcount_final_put tracepoint --- commit 6fe677465a197120927dfba704ab4c4aff5f134a Author: Eugene Mavick <[email protected]> tracing: add refcount_final_put tracepoint Add refcount_final_put tracepoint and related core infrastructure refcount_final_put fires when a reference count reaches zero and the object enters its final release path. > The tracepoint records three fields: > - caller: function that called the refcounting > function(refcount_sub_and_test, percpu_ref_put_many) [Severity: Medium] Are there missing callers for the tracepoint macro? The commit message mentions that the tracepoint records the caller function, such as refcount_sub_and_test or percpu_ref_put_many, but this patch doesn't add any calls to do_trace_refcount_final_put() in the refcount release paths. This leaves the tracepoint inactive. > diff --git a/include/linux/refcount_trace.h b/include/linux/refcount_trace.h > new file mode 100644 > index 0000000000000..6f8d0ba910f05 > --- /dev/null > +++ b/include/linux/refcount_trace.h > @@ -0,0 +1,33 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_REFCOUNT_TRACE_H > +#define _LINUX_REFCOUNT_TRACE_H > + > +#include <linux/tracepoint-defs.h> > +#include <linux/instruction_pointer.h> > + > +#ifdef CONFIG_REFCOUNT_TRACE_FINAL_PUT > +/* Declare the tracepoint so tracepoint_enabled() can be used */ > +DECLARE_TRACEPOINT(refcount_final_put); > + > +/* Wrapper function implemented in lib/ref_trace.c */ [Severity: Low] This isn't a bug, but there's a typo in the filename here. The wrapper is implemented in lib/refcount_trace.c, not lib/ref_trace.c. > +extern void do_refcount_trace_final_put(unsigned long caller, unsigned long > ip, const void *obj); > + > +#define do_trace_refcount_final_put(obj) > \ > + do { > \ > + if (tracepoint_enabled(refcount_final_put)) > \ > + do_refcount_trace_final_put(_RET_IP_, _THIS_IP_, obj); > \ > + } while (0) > + > +#define do_trace_refcount_final_put_cond(cond, obj) > \ > + do { > \ > + if (tracepoint_enabled(refcount_final_put) && cond) > \ [Severity: Medium] Should cond be wrapped in parentheses here? If a caller passes a condition containing a logical OR, such as a || b, operator precedence would cause this to evaluate as: (tracepoint_enabled(...) && a) || b If b is true, the trace wrapper would be invoked unconditionally even when the tracepoint is disabled, bypassing the static key optimization. > + do_refcount_trace_final_put(_RET_IP_, _THIS_IP_, obj); > \ > + } while (0) [ ... ] > diff --git a/lib/refcount_trace.c b/lib/refcount_trace.c > new file mode 100644 > index 0000000000000..2476bb5f2f603 > --- /dev/null > +++ b/lib/refcount_trace.c > @@ -0,0 +1,14 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#define CREATE_TRACE_POINTS > +#include <trace/events/refcount.h> > +#include <linux/refcount_trace.h> > + [Severity: Low] Should this file include <linux/export.h>? It uses EXPORT_SYMBOL_GPL and EXPORT_TRACEPOINT_SYMBOL_GPL below. Relying on implicit inclusions can sometimes cause build failures on different architectures or configurations. > +//Wrapper function for functions defined entirely in header files > +void do_refcount_trace_final_put(unsigned long caller, > + unsigned long ip, > + const void *obj) > +{ > + trace_call__refcount_final_put(caller, ip, obj); > +} > +EXPORT_SYMBOL_GPL(do_refcount_trace_final_put); > +EXPORT_TRACEPOINT_SYMBOL_GPL(refcount_final_put); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
