diff mbox series

[v4,12/41] dyndbg: add DECLARE_DYNDBG_CLASSMAP

Message ID 20220720153233.144129-13-jim.cromie@gmail.com (mailing list archive)
State New, archived
Headers show
Series DYNDBG: opt-in class'd debug for modules, use in drm. | expand

Commit Message

Jim Cromie July 20, 2022, 3:32 p.m. UTC 
DECLARE_DYNDBG_CLASSMAP lets modules declare a set of classnames, this
opt-in authorizes dyndbg to allow enabling of prdbgs by their class:

   :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control

This is just the setup; following commits deliver.

The macro declares and initializes a static struct ddebug_class_map:

 - maps approved class-names to class_ids used in module,
   by array order. forex: DRM_UT_*
 - class-name vals allow validation of "class FOO" queries
   using macro is opt-in
 - enum class_map_type - determines interface, behavior

Each module has its own .class_id space, and only known class-names
will be authorized for a manipulation.  Only DRM stuff should know this:

  :#> echo class DRM_UT_CORE +p > control	# across all modules

pr_debugs (with default class_id) are still controllable as before.

DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, classes...) is::

 _var: name of the static struct var. user passes to module_param_cb()
       if they want a sysfs node. (ive only done it this way).

 _maptype: this is hard-coded to DD_CLASS_TYPE_DISJOINT for now.

 _base: usually 0, it allows splitting 31 classes into subranges, so
 	that multiple classes / sysfs-nodes can share the module's
 	class-id space.

 classes: list of class_name strings, these are mapped to class-ids
 	  starting at _base.  This class-names list must have a
 	  corresponding ENUM, with SYMBOLS that match the literals,
 	  and 1st enum val = _base.

enum class_map_type has 4 values, on 2 factors::

 - classes are disjoint/independent vs relative/x<y/verbosity.
   disjoint is basis, verbosity by overlay.

 - input NUMBERS vs [+-]CLASS_NAMES
   uints, ideally hex.  vs  +DRM_UT_CORE,-DRM_UT_KMS

DD_CLASS_TYPE_DISJOINT: classes are separate, one per bit.
   expecting hex input. basis for others.

DD_CLASS_TYPE_SYMBOLIC: input is a CSV of [+-]CLASS_NAMES,
   classes are independent, like DISJOINT

DD_CLASS_TYPE_VERBOSE: input is numeric level, 0-N.
   0 implies silence. use printk to break that.
   relative levels applied on bitmaps.

DD_CLASS_TYPE_LEVELS: input is a CSV of [+-]CLASS_NAMES,
   names like: ERR,WARNING,NOTICE,INFO,DEBUG
   avoiding EMERG,ALERT,CRIT,ERR - no point.

NOTES:

The macro places the initialized struct ddebug_class_map into the
__dyndbg_classes section.  That draws a 'orphan' warning which we
handle in next commit.  The struct attributes are necessary:
__aligned(8) stopped null-ptr derefs (why?), __used is needed by drm
drivers, which declare class-maps, but don't also declare a
sysfs-param, and thus dont ref the classmap; __used insures that the
linkage is made, then the class-map is found at load-time.

While its possible to handle both NAMES and NUMBERS in the same
sys-interface, there is ambiguity to avoid, by disallowing them
together.  Later, if ambiguities are resolved, 2 new enums can permit
both inputs, on verbose & independent types separately, and authors
can select the interface they like.

RFC:

My plan is to implement LEVELS in the callbacks, outside of
ddebug_exec_query(), which for simplicity will treat the CLASSES in
the map as disjoint.  This should handle most situations.

The callbacks can see map-type, and apply ++/-- loops (or bitops) to
force the relative meanings across the class-bitmap.

That leaves 2 issues:

1. doing LEVELs in callbacks means that the native >control interface
doesn't enforce the LEVELS relationship, so you could confusingly have
V3 enabled, but V1 disabled.  OTOH, the control iface already allows
infinite variety in the underlying callsites, despite the veneer of
uniformity suggested by the bitmap overlay, and LEVELS over that.

2. All dyndbg >control reduces to a query/command, includes +/-, which
is at-root a kernel patching operation with +/- semantics.  And the
symbolic sys-node handling exposes it to the user:

Consider whether these are/should-be 'exactly' the same:

   # force both 2 "half-duplex" relations
   echo +V3,-V4 > /sys/module/test_dynamic_debug/p_VX

   # should these both do the same ?
   echo +V3 > /sys/module/test_dynamic_debug/p_VX
   echo -V4 > /sys/module/test_dynamic_debug/p_VX

IOW, half relations are suggested by the +/-, and enum control of
individual behaviors leaves some room for this, especially wrt
handling [+-]SYMBOLIC inputs from the user.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h | 55 +++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Jason Baron July 22, 2022, 8:23 p.m. UTC | #1 
On 7/20/22 11:32, Jim Cromie wrote:
> DECLARE_DYNDBG_CLASSMAP lets modules declare a set of classnames, this
> opt-in authorizes dyndbg to allow enabling of prdbgs by their class:
> 
>    :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control
> 
> This is just the setup; following commits deliver.
> 
> The macro declares and initializes a static struct ddebug_class_map:
> 
>  - maps approved class-names to class_ids used in module,
>    by array order. forex: DRM_UT_*
>  - class-name vals allow validation of "class FOO" queries
>    using macro is opt-in
>  - enum class_map_type - determines interface, behavior
> 
> Each module has its own .class_id space, and only known class-names
> will be authorized for a manipulation.  Only DRM stuff should know this:
> 
>   :#> echo class DRM_UT_CORE +p > control	# across all modules
> 
> pr_debugs (with default class_id) are still controllable as before.
> 
> DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, classes...) is::
> 
>  _var: name of the static struct var. user passes to module_param_cb()
>        if they want a sysfs node. (ive only done it this way).
> 
>  _maptype: this is hard-coded to DD_CLASS_TYPE_DISJOINT for now.
> 
>  _base: usually 0, it allows splitting 31 classes into subranges, so
>  	that multiple classes / sysfs-nodes can share the module's
>  	class-id space.
> 
>  classes: list of class_name strings, these are mapped to class-ids
>  	  starting at _base.  This class-names list must have a
>  	  corresponding ENUM, with SYMBOLS that match the literals,
>  	  and 1st enum val = _base.
> 
> enum class_map_type has 4 values, on 2 factors::
> 
>  - classes are disjoint/independent vs relative/x<y/verbosity.
>    disjoint is basis, verbosity by overlay.
> 
>  - input NUMBERS vs [+-]CLASS_NAMES
>    uints, ideally hex.  vs  +DRM_UT_CORE,-DRM_UT_KMS
> 

Could the naming here directly reflect the 2 factors? At least for me
I think it's more readable. So something like:


> DD_CLASS_TYPE_DISJOINT: classes are separate, one per bit.
>    expecting hex input. basis for others.

DD_CLASS_TYPE_DISJOINT_NUM

> 
> DD_CLASS_TYPE_SYMBOLIC: input is a CSV of [+-]CLASS_NAMES,
>    classes are independent, like DISJOINT
> 

DD_CLASS_TYPE_DISJOINT_NAME

> DD_CLASS_TYPE_VERBOSE: input is numeric level, 0-N.
>    0 implies silence. use printk to break that.
>    relative levels applied on bitmaps.
> 

DD_CLASS_TYPE_LEVEL_NUM

> DD_CLASS_TYPE_LEVELS: input is a CSV of [+-]CLASS_NAMES,
>    names like: ERR,WARNING,NOTICE,INFO,DEBUG
>    avoiding EMERG,ALERT,CRIT,ERR - no point.
> 

DD_CLASS_TYPE_LEVEL_NAME

> NOTES:
> 
> The macro places the initialized struct ddebug_class_map into the
> __dyndbg_classes section.  That draws a 'orphan' warning which we
> handle in next commit.  The struct attributes are necessary:
> __aligned(8) stopped null-ptr derefs (why?), __used is needed by drm
> drivers, which declare class-maps, but don't also declare a
> sysfs-param, and thus dont ref the classmap; __used insures that the
> linkage is made, then the class-map is found at load-time.
> 
> While its possible to handle both NAMES and NUMBERS in the same
> sys-interface, there is ambiguity to avoid, by disallowing them
> together.  Later, if ambiguities are resolved, 2 new enums can permit
> both inputs, on verbose & independent types separately, and authors
> can select the interface they like.
> 
> RFC:
> 
> My plan is to implement LEVELS in the callbacks, outside of
> ddebug_exec_query(), which for simplicity will treat the CLASSES in
> the map as disjoint.  This should handle most situations.
> 
> The callbacks can see map-type, and apply ++/-- loops (or bitops) to
> force the relative meanings across the class-bitmap.
> 
> That leaves 2 issues:
> 
> 1. doing LEVELs in callbacks means that the native >control interface
> doesn't enforce the LEVELS relationship, so you could confusingly have
> V3 enabled, but V1 disabled.  OTOH, the control iface already allows
> infinite variety in the underlying callsites, despite the veneer of
> uniformity suggested by the bitmap overlay, and LEVELS over that.
> 
> 2. All dyndbg >control reduces to a query/command, includes +/-, which
> is at-root a kernel patching operation with +/- semantics.  And the
> symbolic sys-node handling exposes it to the user:
> 
> Consider whether these are/should-be 'exactly' the same:
> 
>    # force both 2 "half-duplex" relations
>    echo +V3,-V4 > /sys/module/test_dynamic_debug/p_VX
> 
>    # should these both do the same ?
>    echo +V3 > /sys/module/test_dynamic_debug/p_VX
>    echo -V4 > /sys/module/test_dynamic_debug/p_VX
> 
> IOW, half relations are suggested by the +/-, and enum control of
> individual behaviors leaves some room for this, especially wrt
> handling [+-]SYMBOLIC inputs from the user.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  include/linux/dynamic_debug.h | 55 +++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index 0f7ad6cecf05..84e97cd0e8c4 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -56,7 +56,62 @@ struct _ddebug {
>  #endif
>  } __attribute__((aligned(8)));
>  
> +enum class_map_type {
> +	DD_CLASS_TYPE_DISJOINT,
> +	/**
> +	 * DD_CLASS_TYPE_DISJOINT: classes are independent, one per bit.
> +	 * expecting hex input. basis for others.
> +	 */
> +	DD_CLASS_TYPE_VERBOSE,
> +	/**
> +	 * DD_CLASS_TYPE_VERBOSE: input is numeric level, 0-N.
> +	 * 0 should be silent, use printk to break that.
> +	 * (x<y) relationship on bitpos
> +	 */
> +	DD_CLASS_TYPE_SYMBOLIC,
> +	/**
> +	 * DD_CLASS_TYPE_SYMBOLIC: input is a CSV of [+-]CLASS_NAMES,
> +	 * classes are independent, like DISJOINT
> +	 */
> +	DD_CLASS_TYPE_LEVELS,
> +	/**
> +	 * DD_CLASS_TYPE_LEVELS: input is a CSV of [+-]CLASS_NAMES,
> +	 * intended for names like: ERR,WARNING,NOTICE,INFO,DEBUG
> +	 * avoid ? EMERG,ALERT,CRIT,ERR,WARNING ??
> +	 */
> +};
> +
> +struct ddebug_class_map {
> +	struct list_head link;
> +	struct module *mod;
> +	const char *mod_name;	/* needed for builtins */
> +	const char **class_names;
> +	const int length;
> +	const int base;		/* index of 1st .class_id, allows split/shared space */
> +	enum class_map_type map_type;
> +};
> +
> +/**
> + * DECLARE_DYNDBG_CLASSMAP - declare classnames known by a module
> + * @_var:   a struct ddebug_class_map, passed to module_param_cb
> + * @_type:  enum class_map_type, chooses bits/verbose, numeric/symbolic
> + * @_base:  offset of 1st class-name. splits .class_id space
> + * @classes: class-names used to control class'd prdbgs
> + */
> +#define DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, ...)		\
> +	static const char *_var##_classnames[] = { __VA_ARGS__ };	\
> +	static struct ddebug_class_map __aligned(8) __used		\
> +		__section("__dyndbg_classes") _var = {			\
> +		.mod = THIS_MODULE,					\
> +		.mod_name = KBUILD_MODNAME,				\
> +		.base = _base,						\
> +		.map_type = _maptype,					\
> +		.length = NUM_TYPE_ARGS(char*, __VA_ARGS__),		\
> +		.class_names = _var##_classnames,			\
> +	}
>  
> +#define NUM_TYPE_ARGS(eltype, ...)				\
> +	(sizeof((eltype[]) {__VA_ARGS__}) / sizeof(eltype))
>  
>  #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
>
Jim Cromie July 22, 2022, 11:23 p.m. UTC | #2 
On Fri, Jul 22, 2022 at 2:23 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 7/20/22 11:32, Jim Cromie wrote:
> > DECLARE_DYNDBG_CLASSMAP lets modules declare a set of classnames, this
> > opt-in authorizes dyndbg to allow enabling of prdbgs by their class:
> >
> >    :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control
> >
> > This is just the setup; following commits deliver.
> >
> > The macro declares and initializes a static struct ddebug_class_map:
> >
> >  - maps approved class-names to class_ids used in module,
> >    by array order. forex: DRM_UT_*
> >  - class-name vals allow validation of "class FOO" queries
> >    using macro is opt-in
> >  - enum class_map_type - determines interface, behavior
> >
> > Each module has its own .class_id space, and only known class-names
> > will be authorized for a manipulation.  Only DRM stuff should know this:
> >
> >   :#> echo class DRM_UT_CORE +p > control     # across all modules
> >
> > pr_debugs (with default class_id) are still controllable as before.
> >
> > DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, classes...) is::
> >
> >  _var: name of the static struct var. user passes to module_param_cb()
> >        if they want a sysfs node. (ive only done it this way).
> >
> >  _maptype: this is hard-coded to DD_CLASS_TYPE_DISJOINT for now.
> >
> >  _base: usually 0, it allows splitting 31 classes into subranges, so
> >       that multiple classes / sysfs-nodes can share the module's
> >       class-id space.
> >
> >  classes: list of class_name strings, these are mapped to class-ids
> >         starting at _base.  This class-names list must have a
> >         corresponding ENUM, with SYMBOLS that match the literals,
> >         and 1st enum val = _base.
> >
> > enum class_map_type has 4 values, on 2 factors::
> >
> >  - classes are disjoint/independent vs relative/x<y/verbosity.
> >    disjoint is basis, verbosity by overlay.
> >
> >  - input NUMBERS vs [+-]CLASS_NAMES
> >    uints, ideally hex.  vs  +DRM_UT_CORE,-DRM_UT_KMS
> >
>
> Could the naming here directly reflect the 2 factors? At least for me
> I think it's more readable. So something like:
>

yeah, those were 1st-pass names.
your names are more regular

> DD_CLASS_TYPE_DISJOINT_NUM
> DD_CLASS_TYPE_DISJOINT_NAME
> DD_CLASS_TYPE_LEVEL_NUM
> DD_CLASS_TYPE_LEVEL_NAME

s/_NAME/_NAMES/  - the plural suggests list,comma-separated

s/(DISJOINT)_NUM/$1_BITS/  - strongly signals expected input form

LEVEL - while VERBOSE spoke to me, LEVEL has legacy: include/linux/kern_levels.h
LEVELS - pluralize ?
NUM - Im not crazy about, but it stands well in opposition to _BITS
diff mbox series

Patch

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 0f7ad6cecf05..84e97cd0e8c4 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -56,7 +56,62 @@  struct _ddebug {
 #endif
 } __attribute__((aligned(8)));
 
+enum class_map_type {
+	DD_CLASS_TYPE_DISJOINT,
+	/**
+	 * DD_CLASS_TYPE_DISJOINT: classes are independent, one per bit.
+	 * expecting hex input. basis for others.
+	 */
+	DD_CLASS_TYPE_VERBOSE,
+	/**
+	 * DD_CLASS_TYPE_VERBOSE: input is numeric level, 0-N.
+	 * 0 should be silent, use printk to break that.
+	 * (x<y) relationship on bitpos
+	 */
+	DD_CLASS_TYPE_SYMBOLIC,
+	/**
+	 * DD_CLASS_TYPE_SYMBOLIC: input is a CSV of [+-]CLASS_NAMES,
+	 * classes are independent, like DISJOINT
+	 */
+	DD_CLASS_TYPE_LEVELS,
+	/**
+	 * DD_CLASS_TYPE_LEVELS: input is a CSV of [+-]CLASS_NAMES,
+	 * intended for names like: ERR,WARNING,NOTICE,INFO,DEBUG
+	 * avoid ? EMERG,ALERT,CRIT,ERR,WARNING ??
+	 */
+};
+
+struct ddebug_class_map {
+	struct list_head link;
+	struct module *mod;
+	const char *mod_name;	/* needed for builtins */
+	const char **class_names;
+	const int length;
+	const int base;		/* index of 1st .class_id, allows split/shared space */
+	enum class_map_type map_type;
+};
+
+/**
+ * DECLARE_DYNDBG_CLASSMAP - declare classnames known by a module
+ * @_var:   a struct ddebug_class_map, passed to module_param_cb
+ * @_type:  enum class_map_type, chooses bits/verbose, numeric/symbolic
+ * @_base:  offset of 1st class-name. splits .class_id space
+ * @classes: class-names used to control class'd prdbgs
+ */
+#define DECLARE_DYNDBG_CLASSMAP(_var, _maptype, _base, ...)		\
+	static const char *_var##_classnames[] = { __VA_ARGS__ };	\
+	static struct ddebug_class_map __aligned(8) __used		\
+		__section("__dyndbg_classes") _var = {			\
+		.mod = THIS_MODULE,					\
+		.mod_name = KBUILD_MODNAME,				\
+		.base = _base,						\
+		.map_type = _maptype,					\
+		.length = NUM_TYPE_ARGS(char*, __VA_ARGS__),		\
+		.class_names = _var##_classnames,			\
+	}
 
+#define NUM_TYPE_ARGS(eltype, ...)				\
+	(sizeof((eltype[]) {__VA_ARGS__}) / sizeof(eltype))
 
 #if defined(CONFIG_DYNAMIC_DEBUG_CORE)