Re: [PATCH v2 13/59] dyndbg: add 2 new _DPRINTK_FLAGS_: INCL_LOOKUP, PREFIX_CACHED

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 20/03/2025 à 19:51, Jim Cromie a écrit :
> Add _INCL_LOOKUP condition to separate +mfsl flags from +t, allowing
> (after refactoring) to avoid a needless call-return.
> 
> Add a PREFIX_CACHED bit to remember that a pr-debug callsite is:
> 
> - enabled, with +p
> - wants a dynamic-prefix, with _INCL_LOOKUP
> - was previously called
> - was thus saved in the prefix cache. NOT YET.
  >
> This allows (later) to cache part/all of the dynamic-prefix for each
> pr_debug that gets called.
> 
> NOTES:
> 
> dyndbg's dynamic prefixing can get expensive; each enabled callsite's
> prefix is sprintf'd into stack-mem, every time a pr_debug is called.
> 
> A cache would help, if callsites mark _DPRINTK_FLAGS_PREFIX_CACHED
> after saving the prefix string.  But not yet.
> 
> -t  thread-id. not part of the "callsite" info, derived from current.
>      doesn't belong in the cache. it would be wrong.
>      can be done in outer: dynamic_emit_prefix()
> 
> -mfsl  module, function, source-file, line
>      we cache these, composed into a sub-string.
>      they are "lookups", currently to descriptor fields,.
>      could be accessor macros to "compressed" tables.
> 
> All enabled together, they compose a prefix string like:
> 
>    # outer   -----inner-------------------
>    "[tid] module:function:sourcfile:line: "

s/sourcfile/sourcesfile/

> 
> So this patch extracts _DPRINTK_FLAGS_INCL_LOOKUP macro out of
> _DPRINTK_FLAGS_INCL_ANY macro, then redefs latter.
> 
> Next re-refactor dynamic_emit_prefix inner/outer fns accordingly.

This commit introduces two things:

- introduction of _DPRINTK_FLAGS_INCL_LOOKUP, used in a future patch
- introduction of _DPRINTK_FLAGS_PREFIX_CACHED, not used in this series

I don't think those changes are needed to fix DYNDBG_CLASSMAP, it seems 
to be an (unfinished?) optimization, so it could make sense to move it 
to an independent series. Please tell me if I overlooked something!

> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>   include/linux/dynamic_debug.h | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index c388ab05a6e1..82eabaa6e827 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -38,11 +38,13 @@ struct _ddebug {
>   #define _DPRINTK_FLAGS_INCL_LINENO	(1<<3)
>   #define _DPRINTK_FLAGS_INCL_TID		(1<<4)
>   #define _DPRINTK_FLAGS_INCL_SOURCENAME	(1<<5)
> +#define _DPRINTK_FLAGS_PREFIX_CACHED	(1<<7)

Is there a reason to skip 1 << 6? I don't see any usage of this flag in 
this series, maybe you can skip it for now and introduce it with the 
actual implementation of the cache system?

Also, I think it make sense to add some documentation on this define. 
All the other are controlled by the user, but PREFIX_CACHED is 
controlled by the "dyndbg core", so maybe add something like:

	/**
	 * _DPRINTK_FLAGS_PREFIX_CACHED - Mark a printk prefix as cached
	 * This bit is set by the callsite to avoid regenerating fixed
	 * part of the prefix at each call
	 */

>   
> -#define _DPRINTK_FLAGS_INCL_ANY		\
> -	(_DPRINTK_FLAGS_INCL_MODNAME | _DPRINTK_FLAGS_INCL_FUNCNAME |\
> -	 _DPRINTK_FLAGS_INCL_LINENO  | _DPRINTK_FLAGS_INCL_TID |\
> -	 _DPRINTK_FLAGS_INCL_SOURCENAME)
> +#define _DPRINTK_FLAGS_INCL_LOOKUP					\
> +	(_DPRINTK_FLAGS_INCL_MODNAME | _DPRINTK_FLAGS_INCL_FUNCNAME |	\
> +	 _DPRINTK_FLAGS_INCL_SOURCENAME | _DPRINTK_FLAGS_INCL_LINENO)
> +#define _DPRINTK_FLAGS_INCL_ANY						\
> +	(_DPRINTK_FLAGS_INCL_TID | _DPRINTK_FLAGS_INCL_LOOKUP)
>   
>   #if defined DEBUG
>   #define _DPRINTK_FLAGS_DEFAULT _DPRINTK_FLAGS_PRINT

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