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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250728121543.7aaa75eb@batman.local.home>
Date: Mon, 28 Jul 2025 12:15:43 -0400
From: Steven Rostedt <rostedt@...nel.org>
To: Jens Remus <jremus@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 bpf@...r.kernel.org, x86@...nel.org, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Josh Poimboeuf <jpoimboe@...nel.org>, Peter Zijlstra
 <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, Jiri Olsa
 <jolsa@...nel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung
 Kim <namhyung@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Andrii
 Nakryiko <andrii@...nel.org>, Indu Bhagat <indu.bhagat@...cle.com>, "Jose
 E. Marchesi" <jemarch@....org>, Beau Belgrave <beaub@...ux.microsoft.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>, Andrew Morton
 <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>, Florian Weimer
 <fweimer@...hat.com>, Sam James <sam@...too.org>
Subject: Re: [PATCH v15 03/10] unwind_user/deferred: Add unwind cache

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

> On 25.07.2025 20:55, Steven Rostedt wrote:
> > From: Josh Poimboeuf <jpoimboe@...nel.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@...dmis.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>  
> 
> Reviewed-by: Jens Remus <jremus@...ux.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