Message-ID: <405391f8-eb8f-0b49-0d28-eb3ecf5d554d@akamai.com>
Date:   Wed, 25 Aug 2021 13:17:33 -0400
From:   Jason Baron <jbaron@...mai.com>
To:     Jim Cromie <jim.cromie@...il.com>, gregkh@...uxfoundation.org,
        seanpaul@...omium.org, jeyu@...nel.org,
        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
Subject: Re: [PATCH v6 02/11] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and
 callbacks



On 8/22/21 6:20 PM, Jim Cromie wrote:
> DEFINE_DYNAMIC_DEBUG_CATEGORIES(name, var, bitmap_desc, @bit_descs)
> allows users to define a drm.debug style (bitmap) sysfs interface, and
> to specify the desired mapping from bits[0-N] to the format-prefix'd
> pr_debug()s to be controlled.
> 
> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
> 	"i915/gvt bitmap desc",
> 	/**
> 	 * search-prefixes, passed to dd-exec_queries
> 	 * defines bits 0-N in order.
> 	 * leading ^ is tacitly inserted (by callback currently)
> 	 * trailing space used here excludes subcats.
> 	 * helper macro needs more work
> 	 * macro to autogen ++$i, 0x%x$i ?
> 	 */
> 	_DD_cat_("gvt:cmd: "),
> 	_DD_cat_("gvt:core: "),
> 	_DD_cat_("gvt:dpy: "),
> 	_DD_cat_("gvt:el: "),
> 	_DD_cat_("gvt:irq: "),
> 	_DD_cat_("gvt:mm: "),
> 	_DD_cat_("gvt:mmio: "),
> 	_DD_cat_("gvt:render: "),
> 	_DD_cat_("gvt:sched: "));
> 
> dynamic_debug.c: add 3 new elements:
> 
>  - int param_set_dyndbg()
>  - int param_get_dyndbg()
>  - struct kernel_param_ops param_ops_dyndbg
> 
> Following the model of kernel/params.c STANDARD_PARAM_DEFS, All 3 are
> non-static and exported.
> 
> dynamic_debug.h:
> 
> Add DEFINE_DYNAMIC_DEBUG_CATEGORIES() described above, and a do-nothing stub.
> 
> Note that it also calls MODULE_PARM_DESC for the user, but expects the
> user to catenate all the bit-descriptions together (as is done in
> drm.debug), and in the following uses in amdgpu, i915.
> 
> This in the hope that someone can offer an auto-incrementing
> label-generating macro, producing "\tbit-4 0x10\t" etc, and can show
> how to apply it to __VA_ARGS__.
> 
> Also extern the struct kernel_param param_ops_dyndbg symbol, as is
> done in moduleparams.h for all the STANDARD params.
> 
> USAGE NOTES:
> 
> Using dyndbg to query on "format ^$prefix" requires that the prefix be
> present in the compiled-in format string; where run-time prefixing is
> used, that format would be "%s...", which is not usefully selectable.
> 
> Adding structural query terms (func,file,lineno) could help (module is
> already done), but DEFINE_DYNAMIC_DEBUG_CATEGORIES can't do that now,
> adding it needs a better reason imo.
> 
> Dyndbg is completely agnostic wrt the categorization scheme used, to
> play well with any prefix convention already in use.  Ad-hoc
> categories and sub-categories are implicitly allowed, author
> discipline and review is expected.
> 
> Here are some examples:
> 
> "1","2","3"		2 doesn't imply 1.
>    			otherwize, sorta like printk levels
> "1:","2:","3:"		are better, avoiding [1-9]\d+ ambiguity
> "hi:","mid:","low:"	are reasonable, and imply independence
> "todo:","rfc:","fixme:" might be handy
> "A:".."Z:"		uhm, yeah
> 
> Hierarchical classes/categories are natural:
> 
> "drm:<CAT>:"		is used in later commit
> "drm:<CAT>:<SUB>:"	is a natural extension.
> "drm:atomic:fail:"	has been proposed, sounds directly useful
> 
> Some properties of a hierarchical category deserve explication:
> 
> Trailing spaces matter !
> 
> With 1..3-space ("drm: ", "drm:atomic: ", "drm:atomic:fail: "), the
> ":" doesn't terminate the search-space, the trailing space does.  So a
> "drm:" search spec will match all DRM categories & subcategories, and
> will not be useful in an interface where all categories are already
> controlled together.  That said, "drm:atomic:" & "drm:atomic: " are
> different, and both are useful in cases.
> 
> Ad-Hoc sub-categories:
> 
> These have a caveat wrt wrapper macros adding prefixes like
> "drm:atomic: "; the trailing space in the prefix means that
> drm_dbg_atomic("fail: ...") pastes as "drm:atomic: fail: ", which
> obviously isn't ideal wrt clear and simple bitmaps.
> 
> A possible solution is to have a FOO_() version of every FOO() macro
> which (anti-mnemonically) elides the trailing space, which is normally
> inserted by a modified FOO().  Doing this would enforce a policy
> decision that "debug categories will be space terminated", with an
> pressure-relief valve.
> 
> Summarizing:
> 
>  - "drm:kms: " & "drm:kms:" are different
>  - "drm:kms"		also different - includes drm:kms2:
>  - "drm:kms:\t"		also different
>  - "drm:kms:*"		doesn't work, no wildcard on format atm.
> 
> Order matters in DEFINE_DYNAMIC_DEBUG_CATEGORIES(... @bit_descs)
> 
> @bit_descs (array) position determines the bit mapping to the prefix,
> so to keep a stable map, new categories or 3rd level categories must
> be added to the end.
> 
> Since bits are/will-stay applied 0-N, the later bits can countermand
> the earlier ones, but its tricky - consider;
> 
>     DD_CATs(... "drm:atomic:", "drm:atomic:fail:" ) // misleading
> 
> The 1st search-term is misleading, because it includes (modifies)
> subcategories, but then 2nd overrides it.  So don't do that.
> 
> For "drm:atomic:fail:" in particular, its best not to add it into an
> existing bitmap, because the current setting would be lost at every
> (unrelated) write, and a separate bitmap is much more stable.
> 
> There is still plenty of bikeshedding to do.
> 
> Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> ---
> v5:
> . rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
> . in set_dyndbg, replace hardcoded "i915" w kp->mod->name
> . static inline the stubs
> . const *str in structs, const array. - Emil
> . dyndbg: add do-nothing DEFINE_DYNAMIC_DEBUG_CATEGORIES if !DD_CORE
> . call MOD_PARM_DESC(name, "$desc") for users
> . simplify callback, remove bit-change detection
> . config errs reported by <lkp@...el.com>
> 
> v6:
> . return rc, bitmap->, snprintf, ws - Andy Shevchenko
> . s/chgct/matches/ - old varname is misleading
> . move code off file bottom to a "better" place
> . change ##fsname to ##var for safer varname construct
> . workaround for !CONFIG_MODULES
> . move forward decl down to where its needed
> ---
>  include/linux/dynamic_debug.h | 52 +++++++++++++++++++++++-
>  lib/dynamic_debug.c           | 76 ++++++++++++++++++++++++++++++++---
>  2 files changed, 121 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index dce631e678dd..51b7254daee0 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -51,8 +51,6 @@ struct _ddebug {
>  #endif
>  } __attribute__((aligned(8)));
>  
> -
> -
>  #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
>  
>  /* exported for module authors to exercise >control */
> @@ -181,6 +179,10 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
>  				   KERN_DEBUG, prefix_str, prefix_type,	\
>  				   rowsize, groupsize, buf, len, ascii)
>  
> +struct kernel_param;
> +int param_set_dyndbg(const char *instr, const struct kernel_param *kp);
> +int param_get_dyndbg(char *buffer, const struct kernel_param *kp);
> +
>  #else /* !CONFIG_DYNAMIC_DEBUG_CORE */
>  
>  #include <linux/string.h>
> @@ -227,6 +229,52 @@ static inline int dynamic_debug_exec_queries(const char *query, const char *modn
>  	return 0;
>  }
>  
> +static inline int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> +	{ return 0; }
> +static inline int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
> +	{ return 0; }
> +
>  #endif /* !CONFIG_DYNAMIC_DEBUG_CORE */
>  
> +struct dyndbg_bitdesc {
> +	/* bitpos is inferred from index in containing array */
> +	const char *prefix;
> +	const char *help;
> +};
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> +	(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
> +/**
> + * DEFINE_DYNAMIC_DEBUG_CATEGORIES() - define debug categories, bitmap, sysfs-knob
> + * @fsname: parameter basename under /sys
> + * @var:    C-identifier holding state
> + * @_desc:  string summarizing the controls provided
> + * @...:    list of struct dyndbg_bitdesc initializations
> + *
> + * Defines /sys/modules/$modname/parameters/@...ame, and @bit_descs,
> + * which maps bits 0-N to categories of pr_debugs to be controlled.
> + * This is effectively write only, because controlled callsites can be
> + * further modified via >control.
> + *
> + * Also calls MODULE_PARM_DESC(fsname, _desc), with the intent to
> + * generate the bit_legend and apply it to the given bit_descs
> + */
> +#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, var, _desc, ...)	\
> +	MODULE_PARM_DESC(fsname, _desc);				\
> +	static const struct dyndbg_bitdesc dyndbg_cats_##var[] =	\
> +		{ __VA_ARGS__, { NULL, NULL } };			\
> +	module_param_cb_data(fsname, &param_ops_dyndbg, &var, 0644,	\
> +			     &dyndbg_cats_##var)
> +
> +#define _DD_cat_(pfx)		{ .prefix = pfx, .help = "help for " pfx }
> +#define _DD_cat_help_(pfx)	"\t   " pfx "\t- help for " pfx "\n"
> +
> +extern const struct kernel_param_ops param_ops_dyndbg;
> +#else
> +#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, var, bitmap_desc, ...) \
> +	MODULE_PARM_DESC(fsname, "auto: " bitmap_desc)
> +#define _DD_cat_(pfx)
> +#define _DD_cat_help_(pfx)
> +#endif
> +
>  #endif
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index cb5abb42c16a..a43427c67c3f 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -511,10 +511,11 @@ static int ddebug_exec_query(char *query_string, const char *modname)
>  	return nfound;
>  }
>  
> -/* handle multiple queries in query string, continue on error, return
> -   last error or number of matching callsites.  Module name is either
> -   in param (for boot arg) or perhaps in query string.
> -*/
> +/*
> + * handle multiple queries in query string, continue on error, return
> + * last error or number of matching callsites.  Module name is either
> + * in param (for boot arg) or perhaps in query string.
> + */
>  static int ddebug_exec_queries(char *query, const char *modname)
>  {
>  	char *split;
> @@ -529,7 +530,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
>  		if (!query || !*query || *query == '#')
>  			continue;
>  
> -		vpr_info("query %d: \"%s\"\n", i, query);
> +		vpr_info("query %d: \"%s\" %s\n", i, query, (modname) ? modname : "");
>  
>  		rc = ddebug_exec_query(query, modname);
>  		if (rc < 0) {
> @@ -577,6 +578,71 @@ int dynamic_debug_exec_queries(const char *query, const char *modname)
>  }
>  EXPORT_SYMBOL_GPL(dynamic_debug_exec_queries);
>  
> +#ifdef MODULES
> +#define KP_MOD_NAME kp->mod->name
> +#else
> +#define KP_MOD_NAME NULL /* wildcard */
> +#endif
> +#define FMT_QUERY_SIZE 128 /* typically need <40 */
> +/**
> + * param_set_dyndbg() - drm.debug style bits=>categories setter
> + * @instr: string echo>d to sysfs
> + * @kp:    struct kernel_param* ->data has bitmap
> + * Exported to support DEFINE_DYNAMIC_DEBUG_CATEGORIES
> + */
> +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> +{
> +	unsigned long inbits;
> +	int rc, i, matches = 0, totct = 0;
> +	char query[FMT_QUERY_SIZE];
> +	const struct dyndbg_bitdesc *bitmap = kp->data;
> +
> +	if (!bitmap) {
> +		pr_err("set_dyndbg: no bits=>queries map\n");
> +		return -EINVAL;
> +	}
> +	rc = kstrtoul(instr, 0, &inbits);
> +	if (rc) {
> +		pr_err("set_dyndbg: failed\n");
> +		return rc;
> +	}
> +	vpr_info("set_dyndbg: input 0x%lx\n", inbits);
> +
> +	for (i = 0; bitmap->prefix; i++, bitmap++) {
> +		snprintf(query, FMT_QUERY_SIZE, "format '^%s' %cp", bitmap->prefix,
> +			 test_bit(i, &inbits) ? '+' : '-');
> +
> +		matches = ddebug_exec_queries(query, KP_MOD_NAME);
> +
> +		v2pr_info("bit-%d: %d matches on '%s'\n", i, matches, query);
> +		totct += matches;
> +	}
> +	vpr_info("total matches: %d\n", totct);
> +	return 0;
> +}
> +EXPORT_SYMBOL(param_set_dyndbg);
> +
> +/**
> + * param_get_dyndbg() - drm.debug style bitmap to format-prefix categories
> + * @buffer: string returned to user via sysfs
> + * @kp:     struct kernel_param*
> + * Exported to provide required .get interface, not useful.
> + * pr_debugs may be altered after .set via `echo $foo >control`
> + */
> +int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
> +{
> +	return scnprintf(buffer, PAGE_SIZE, "%u\n",
> +			 *((unsigned int *)kp->arg));


If kp->arg is read here, don't we need to set too somewhere? I'm wondering
if the 'get' can use one of the generic ones like param_get_int too? Instead
of a special case here.

Thanks,

-Jason

> +}
> +EXPORT_SYMBOL(param_get_dyndbg);
> +
> +const struct kernel_param_ops param_ops_dyndbg = {
> +	.set = param_set_dyndbg,
> +	.get = param_get_dyndbg,
> +};
> +/* support DEFINE_DYNAMIC_DEBUG_CATEGORIES users */
> +EXPORT_SYMBOL(param_ops_dyndbg);
> +
>  #define PREFIX_SIZE 64
>  
>  static int remaining(int wrote)
> 

Powered by blists - more mailing lists