linux-kernel - Re: [PATCH v2 14/59] dyndbg: split _emit_lookup() out of dynamic_emit

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <04d92399-72bc-45f6-870c-1dc5c758686f@bootlin.com>
Date: Mon, 24 Mar 2025 16:14:31 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Jim Cromie <jim.cromie@...il.com>, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, amd-gfx@...ts.freedesktop.org,
 intel-gvt-dev@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
 intel-gfx-trybot@...ts.freedesktop.org
Cc: jbaron@...mai.com, gregkh@...uxfoundation.org, ukaszb@...omium.org,
 daniel.vetter@...ll.ch, tvrtko.ursulin@...ux.intel.com,
 jani.nikula@...el.com, ville.syrjala@...ux.intel.com
Subject: Re: [PATCH v2 14/59] dyndbg: split _emit_lookup() out of
 dynamic_emit_prefix()



Le 20/03/2025 à 19:51, Jim Cromie a écrit :
> Split dynamic_emit_prefix() to separate out _INCL_LOOKUPs:
> 
> 1. keep dynamic_emit_prefix() static inline
>     check _INCL_ANY flags before calling 2
> 
> 2. __dynamic_emit_prefix()
>     prints [TID] or <intr> and trailing space if +t flag
>     check _INCL_LOOKUP flags before calling 3
> 
> 3. __dynamic_emit_lookup()
>     prints ONLY module, function, src, line, and trailing space
>     TID isn't "callsite" specific info.
>     result is "cacheable"
> 
> Notes:
> 
> 2,3 are gated, only called when theyve something to emit, so they just
> add trailing space.  This obsoletes the pos_after_tid var and logic.
> 
> __dynamic_emit_lookup() adds line too, so the result is "whole".
> While this would enlarge a naive cache vs add-line-after-caching, we
> dont even have a naive one yet.
> 
> And some clever indexing on store() might be able to fold the flags
> setting in, such that the prefix stored with +mf flags only (-l),
> could be returned for all pr_debugs in that function which also had
> +mf flags. While still supporting +mfsl prefixes (with cache
> expansion) as they're used.

Like the previous patch: I think this should be a separate series.

> Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> ---
>   lib/dynamic_debug.c | 40 +++++++++++++++++++++++++---------------
>   1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 663c125006d0..f7ec2365ab40 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -850,19 +850,8 @@ static int remaining(int wrote)
>   	return 0;
>   }
>   
> -static char *__dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
> +static int __dynamic_emit_lookup(const struct _ddebug *desc, char *buf, int pos)
>   {
> -	int pos_after_tid;
> -	int pos = 0;
> -
> -	if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
> -		if (in_interrupt())
> -			pos += snprintf(buf + pos, remaining(pos), "<intr> ");
> -		else
> -			pos += snprintf(buf + pos, remaining(pos), "[%d] ",
> -					task_pid_vnr(current));
> -	}
> -	pos_after_tid = pos;
>   	if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
>   		pos += snprintf(buf + pos, remaining(pos), "%s:",
>   				desc->modname);
> @@ -875,8 +864,29 @@ static char *__dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
>   	if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
>   		pos += snprintf(buf + pos, remaining(pos), "%d:",
>   				desc->lineno);
> -	if (pos - pos_after_tid)
> -		pos += snprintf(buf + pos, remaining(pos), " ");
> +
> +	/* cuz LOOKUP, we've emitted, so add trailing space if room */

s/cuz/because/

> +	if (remaining(pos))
> +		buf[pos++] = ' ';
> +
> +	return pos;
> +}
> +
> +static char *__dynamic_emit_prefix(struct _ddebug *desc, char *buf)
> +{
> +	int pos = 0;
> +
> +	if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
> +		if (in_interrupt())
> +			pos += snprintf(buf + pos, remaining(pos), "<intr> ");
> +		else
> +			pos += snprintf(buf + pos, remaining(pos), "[%d] ",
> +					task_pid_vnr(current));
> +	}
> +
> +	if (unlikely(desc->flags & _DPRINTK_FLAGS_INCL_LOOKUP))
> +		pos += __dynamic_emit_lookup(desc, buf, pos);
> +
>   	if (pos >= PREFIX_SIZE)
>   		buf[PREFIX_SIZE - 1] = '\0';
>   
> @@ -885,7 +895,7 @@ static char *__dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
>   
>   static inline char *dynamic_emit_prefix(struct _ddebug *desc, char *buf)
>   {
> -	if (unlikely(desc->flags & _DPRINTK_FLAGS_INCL_ANY))
> +	if (desc->flags & _DPRINTK_FLAGS_INCL_ANY)

Why do you remove the unlikely here?

>   		return __dynamic_emit_prefix(desc, buf);
>   	return buf;
>   }

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com