Re: [PATCH v15 03/10] unwind_user/deferred: Add unwind cache

[Steven Rostedt <rostedt@kernel.org>]

On Mon, 28 Jul 2025 17:46:42 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

> On 25.07.2025 20:55, Steven Rostedt wrote:
> > From: Josh Poimboeuf <jpoimboe@kernel.org>
> > 
> > Cache the results of the unwind to ensure the unwind is only performed
> > once, even when called by multiple tracers.
> > 
> > The cache nr_entries gets cleared every time the task exits the kernel.
> > When a stacktrace is requested, nr_entries gets set to the number of
> > entries in the stacktrace. If another stacktrace is requested, if
> > nr_entries is not zero, then it contains the same stacktrace that would be
> > retrieved so it is not processed again and the entries is given to the
> > caller.
> > 
> > Co-developed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  
> 
> Reviewed-by: Jens Remus <jremus@linux.ibm.com>

Thanks.

> 
> > diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c  
> 
> > +	cache = info->cache;
> > +	trace->entries = cache->entries;
> > +
> > +	if (cache->nr_entries) {
> > +		/*
> > +		 * The user stack has already been previously unwound in this
> > +		 * entry context.  Skip the unwind and use the cache.
> > +		 */
> > +		trace->nr = cache->nr_entries;
> > +		return 0;
> > +	}
> > +
> >  	trace->nr = 0;
> > -	trace->entries = info->entries;
> >  	unwind_user(trace, UNWIND_MAX_ENTRIES);
> >  
> > +	cache->nr_entries = trace->nr;
> > +  
> 
> Would the following alternative to above excerpt be easier to read?

Not to me ;-)

I looked at this and read it a couple of times, but had to go back to
see what it was replacing before I understood it.

I prefer the original. It's logic is, "if this was already done, just
return the cache", where as the below logic is "Assign everything, if
it hasn't been done, do it now".

Maybe it's just my own preference, but I'm more comfortable with the
"if it's already been done, exit out early" than the "set everything
up, and do it if it hasn't been done" approach.

-- Steve


> 
> 	/* Use the cache, if the user stack has already been previously
> 	 * unwound in this entry context.  If not this will initialize
> 	 * trace->nr to zero to trigger the unwind now.
> 	 */
> 	cache = info->cache;
> 	trace->nr = cache->nr_entries;
> 	trace->entries = cache->entries;
> 
> 	if (!trace->nr) {
> 		unwind_user(trace, UNWIND_MAX_ENTRIES);
> 		cache->nr_entries = trace->nr;
> 	}
> 
> >  	return 0;
> >  }  
> 
> Regards,
> Jens