mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Eugene Mavick <m@mavick.dev>
Cc: Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Boqun Feng <boqun@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>, Gary Guo <gary@garyguo.net>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dennis Zhou <dennis@kernel.org>, Tejun Heo <tj@kernel.org>,
	Christoph Lameter <cl@gentwo.org>,
	linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [PATCH v3 1/5] tracing: add ref_trace_final_put tracepoint
Date: Mon, 27 Jul 2026 16:21:22 -0400	[thread overview]
Message-ID: <20260727162122.39f292fe@gandalf.local.home> (raw)
In-Reply-To: <20260711-refcount-final-put-trace-v3-1-674c8f03d9f9@mavick.dev>

On Sat, 11 Jul 2026 12:40:19 +0800
Eugene Mavick <m@mavick.dev> wrote:

> Add ref_trace_final_put tracepoint and related core infrastructure
> 
> ref_trace_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)
> - fn: refcounting function(eg refcount_sub_and_test)

Why save the string just to figure out which function is used. It's quite
expensive in both recording the string and saving the name all over the
place.

> - obj: refcount object(struct percpu_ref, refcount_t)
> 
> Signed-off-by: Eugene Mavick <m@mavick.dev>
> ---
>  include/linux/ref_trace.h        | 26 +++++++++++++++++++++++
>  include/trace/events/ref_trace.h | 46 ++++++++++++++++++++++++++++++++++++++++
>  lib/Makefile                     |  2 ++
>  lib/ref_trace.c                  | 13 ++++++++++++
>  4 files changed, 87 insertions(+)
> 
> diff --git a/include/linux/ref_trace.h b/include/linux/ref_trace.h
> new file mode 100644
> index 000000000000..6c6600938a47
> --- /dev/null
> +++ b/include/linux/ref_trace.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_REF_TRACE_H
> +#define _LINUX_REF_TRACE_H
> +
> +#include <linux/tracepoint-defs.h>
> +#include <linux/instruction_pointer.h>
> +
> +/* Declare the tracepoint so tracepoint_enabled() can be used */
> +DECLARE_TRACEPOINT(ref_trace_final_put);
> +
> +#ifdef CONFIG_TRACEPOINTS
> +/* Wrapper function implemented in lib/ref_trace.c */
> +extern void do_ref_trace_final_put(unsigned long caller, const char *fn, const void *obj);
> +
> +#define do_trace_ref_final_put(obj)						\
> +	do {									\
> +		if (tracepoint_enabled(ref_trace_final_put))			\
> +			do_ref_trace_final_put(_RET_IP_, __func__, obj);	\

Why save __func__ and and not just __THIS_IP__?

The name is quite expensive to save and even to store in memory, as it will
*always* be there.

> +	} while (0)
> +
> +#else /* !CONFIG_TRACEPOINTS */
> +static inline void do_ref_trace_final_put(unsigned long caller, const char *fn, const void *obj) { }
> +#define do_trace_ref_final_put(obj) do { } while (0)
> +#endif
> +
> +#endif /* _LINUX_REF_TRACE_H */
> diff --git a/include/trace/events/ref_trace.h b/include/trace/events/ref_trace.h
> new file mode 100644
> index 000000000000..3bf1b1d390b8
> --- /dev/null
> +++ b/include/trace/events/ref_trace.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM ref_trace
> +
> +#if !defined(_TRACE_REF_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_REF_TRACE_H
> +
> +#include <linux/tracepoint.h>
> +
> +/**
> + * ref_trace_final_put - trace when a reference count reaches zero
> + * @caller: return address of refcount
> + * function(refcount_sub_and_test, percpu_ref_put_many)
> + * @fn: refcounting function(eg refcount_sub_and_test)
> + * @obj: refcount object(struct percpu_ref, refcount_t)
> + *
> + * Tracepoint instrumentation can be added using the do_ref_trace_final_put
> + * macro defined in include/linux/ref_trace.h
> + * which uses _RET_IP_ and __func__ for caller and fn arguments respectively,
> + * thus only requiring obj arg to be supplied
> + */
> +TRACE_EVENT(ref_trace_final_put,
> +
> +	TP_PROTO(unsigned long caller, const char *fn, const void *obj),
> +
> +	TP_ARGS(caller, fn, obj),
> +
> +	TP_STRUCT__entry(
> +	__field(unsigned long, caller)
> +	__string(fn, fn)
> +	__field(const void *, obj)

The TP_STRUCT__entry is to be in structure format:

	TP_STRUCT__entry(
		__field(	unsigned long,		caller	)
		__field(	unsigned long,		ip	)
		__field(	const void *,		obj	)
> +	),

See how much better that looks and easier to read. Just like writing:

struct entry {
	unsigned long	caller;
	unsigned long	ip;
	const void	*obj;
};

	
> +
> +	TP_fast_assign(
> +	__entry->caller = caller;
> +	__assign_str(fn);
> +	__entry->obj = obj;

The code should be indented:

	TP_fast_assign(
		__entry->caller = caller;
		__entry->ip = ip;
		__entry->obj = obj;
> +	),

> +
> +	TP_printk("caller=%pS fn=%s obj=%p", (void *)__entry->caller, __get_str(fn), __entry->obj)

	TP_printk("caller=%pS fn=%pS obj=%p", ...

-- Steve

> +);
> +
> +#endif /* _TRACE_REF_TRACE_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> diff --git a/lib/Makefile b/lib/Makefile
> index f33a24bf1c19..41737090a95d 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -335,3 +335,5 @@ CONTEXT_ANALYSIS_test_context-analysis.o := y
>  obj-$(CONFIG_CONTEXT_ANALYSIS_TEST) += test_context-analysis.o
>  
>  subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
> +
> +obj-$(CONFIG_TRACEPOINTS) += ref_trace.o
> diff --git a/lib/ref_trace.c b/lib/ref_trace.c
> new file mode 100644
> index 000000000000..466647772ce1
> --- /dev/null
> +++ b/lib/ref_trace.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/ref_trace.h>
> +#include <linux/ref_trace.h>
> +
> +//Wrapper function for functions defined entirely in header files
> +void do_ref_trace_final_put(unsigned long caller, const char *fn, const void *obj)
> +{
> +	trace_call__ref_trace_final_put(caller, fn, obj);
> +}
> +EXPORT_SYMBOL_GPL(do_ref_trace_final_put);
> +
> +EXPORT_TRACEPOINT_SYMBOL_GPL(ref_trace_final_put);
> 


  reply	other threads:[~2026-07-27 20:21 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-11  4:40 [PATCH v3 0/5] tracing: add ref_trace_final_put tracing Eugene Mavick
2026-07-11  4:40 ` [PATCH v3 1/5] tracing: add ref_trace_final_put tracepoint Eugene Mavick
2026-07-27 20:21   ` Steven Rostedt [this message]
2026-07-11  4:40 ` [PATCH v3 2/5] refcount: " Eugene Mavick
2026-07-27 20:27   ` Steven Rostedt
2026-07-11  4:40 ` [PATCH v3 3/5] percpu-refcount: add ref_trace_final_put trace Eugene Mavick
2026-07-11  4:40 ` [PATCH v3 4/5] kunit: add test for ref_trace_final_put Eugene Mavick
2026-07-11  4:40 ` [PATCH v3 5/5] MAINTAINERS: add entries " Eugene Mavick

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260727162122.39f292fe@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=akpm@linux-foundation.org \
    --cc=boqun@kernel.org \
    --cc=cl@gentwo.org \
    --cc=dennis@kernel.org \
    --cc=gary@garyguo.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=m@mavick.dev \
    --cc=mark.rutland@arm.com \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tj@kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®