Re: [PATCH v5 1/5] tracing: add refcount_final_put tracepoint

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