On Fri, Jul 22, 2022 at 2:33 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>
>
>
> On 7/20/22 11:32, Jim Cromie wrote:
> > Add kernel_param_ops and callbacks to apply a class-map to a
> > sysfs-node, which then can control classes defined in that class-map.
> > This supports uses like:
> >
> > echo 0x3 > /sys/module/drm/parameters/debug
> >
> > IE add these:
> >
> > - int param_set_dyndbg_classes()
> > - int param_get_dyndbg_classes()
> > - struct kernel_param_ops param_ops_dyndbg_classes
> >
> > Following the model of kernel/params.c STANDARD_PARAM_DEFS, these are
> > non-static and exported. This might be unnecessary here.
> >
> > get/set use an augmented kernel_param; the arg refs a new struct
> > ddebug_classes_bitmap_param, initialized by DYNAMIC_DEBUG_CLASSBITS
> > macro, which contains:
> >
> > BITS: a pointer to the user module's ulong holding the bits/state. By
> > ref'g the client's bit-state _var, we coordinate with existing code
> > (such as drm_debug_enabled) which uses the _var, so it works
> > unchanged, even as the foundation is switched out underneath it..
> > Using a ulong allows use of BIT() etc.
> >
> > FLAGS: dyndbg.flags toggled by changes to bitmap. Usually just "p".
> >
> > MAP: a pointer to struct ddebug_classes_map, which maps those
> > class-names to .class_ids 0..N that the module is using. This
> > class-map is declared & initialized by DEFINE_DYNDBG_CLASSMAP.
> >
> > map-type: add support here for DD_CLASS_DISJOINT, DD_CLASS_VERBOSE.
> >
> > These 2 class-types both expect an integer; _DISJOINT treats input
> > like a bit-vector (ala drm.debug), and sets each bit accordingly.
> >
> > _VERBOSE treats input like a bit-pos:N, then sets bits(0..N)=1, and
> > bits(N+1..max)=0. This applies (bit<N) semantics on top of disjoint
> > bits.
> >
> > cases DD_CLASS_SYMBOLIC, DD_CLASS_LEVELS are included for the complete
> > picture, with commented out call to a following commit.
> >
> > NOTES:
> >
> > this now includes SYMBOLIC/LEVELS support, too tedious to keep
> > separate thru all the tweaking.
> >
> > get-param undoes the bit-pos -> bitmap transform that set-param does
> > on VERBOSE inputs, this gives the read-what-was-written property.
> >
> > _VERBOSE is overlay on _DISJOINT:
> >
> > verbose-maps still need class-names, even though theyre not usable at
> > the sysfs interface (unlike with _SYMBOLIC/_LEVELS).
> >
> > - It must have a "V0" name,
> > something below "V1" to turn "V1" off.
> > __pr_debug_cls(V0,..) is printk, don't do that.
> >
> > - "class names" is required at the >control interface.
> > - relative levels are not enforced at >control
> >
> > IOW this is possible, and maybe confusing:
> >
> > echo class V3 +p > control
> > echo class V1 -p > control
> >
> > IMO thats ok, relative verbosity is an interface property.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
> > ---
> > . drop kp->mod->name as unneeded (build-dependent) <lkp>
> > ---
> > include/linux/dynamic_debug.h | 18 ++++
> > lib/dynamic_debug.c | 193 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 211 insertions(+)
> >
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> > index f57076e02767..b50bdd5c8184 100644
> > --- a/include/linux/dynamic_debug.h
> > +++ b/include/linux/dynamic_debug.h
> > @@ -113,6 +113,12 @@ struct ddebug_class_map {
> > #define NUM_TYPE_ARGS(eltype, ...) \
> > (sizeof((eltype[]) {__VA_ARGS__}) / sizeof(eltype))
> >
> > +struct ddebug_classes_bitmap_param {
> > + unsigned long *bits;
> > + char flags[8];
> > + const struct ddebug_class_map *map;
> > +};
> > +
> > #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
> >
> > int ddebug_add_module(struct _ddebug *tab, unsigned int num_debugs,
> > @@ -274,6 +280,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_classes(const char *instr, const struct kernel_param *kp);
> > +int param_get_dyndbg_classes(char *buffer, const struct kernel_param *kp);
> > +
> > /* for test only, generally expect drm.debug style macro wrappers */
> > #define __pr_debug_cls(cls, fmt, ...) do { \
> > BUILD_BUG_ON_MSG(!__builtin_constant_p(cls), \
> > @@ -322,6 +332,14 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
> > rowsize, groupsize, buf, len, ascii); \
> > } while (0)
> >
> > +struct kernel_param;
> > +static inline int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
> > +{ return 0; }
> > +static inline int param_get_dyndbg_classes(char *buffer, const struct kernel_param *kp)
> > +{ return 0; }
> > +
> > #endif /* !CONFIG_DYNAMIC_DEBUG_CORE */
> >
> > +extern const struct kernel_param_ops param_ops_dyndbg_classes;
> > +
> > #endif
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 4c27bbe5187e..dd27dc514aa3 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -596,6 +596,199 @@ static int ddebug_exec_queries(char *query, const char *modname)
> > return nfound;
> > }
> >
> > +static int ddebug_apply_class_bitmap(const struct ddebug_classes_bitmap_param *dcp,
> > + unsigned long inbits)
> > +{
> > +#define QUERY_SIZE 128
> > + char query[QUERY_SIZE];
> > + const struct ddebug_class_map *map = dcp->map;
> > + int matches = 0;
> > + int bi, ct;
> > +
> > + v2pr_info("in: 0x%lx on: 0x%lx\n", inbits, *dcp->bits);
> > +
> > + for (bi = 0; bi < map->length; bi++) {
> > + if (test_bit(bi, &inbits) == test_bit(bi, dcp->bits))
> > + continue;
> > +
> > + snprintf(query, QUERY_SIZE, "class %s %c%s", map->class_names[bi],
> > + test_bit(bi, &inbits) ? '+' : '-', dcp->flags);
> > +
> > + ct = ddebug_exec_queries(query, NULL);
> > + matches += ct;
> > +
> > + v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
> > + ct, map->class_names[bi], inbits);
> > + }
> > + return matches;
> > +}
> > +
> > +/* support for [+-] symbolic-name boolean list */
> > +static int param_set_dyndbg_class_strings(const char *instr, const struct kernel_param *kp)
> > +{
> > + const struct ddebug_classes_bitmap_param *dcp = kp->arg;
> > + const struct ddebug_class_map *map = dcp->map;
> > + unsigned long inbits;
> > + int idx, totct = 0;
> > + bool wanted;
> > + char *cls, *p;
> > +
> > + cls = kstrdup(instr, GFP_KERNEL);
> > + p = strchr(cls, '\n');
> > + if (p)
> > + *p = '\0';
> > +
> > + vpr_info("\"%s\" > %s\n", cls, kp->name);
> > + inbits = *dcp->bits;
> > +
> > + for (; cls; cls = p) {
> > + p = strchr(cls, ',');
> > + if (p)
> > + *p++ = '\0';
> > +
> > + if (*cls == '-') {
> > + wanted = false;
> > + cls++;
> > + } else {
> > + wanted = true;
> > + if (*cls == '+')
> > + cls++;
> > + }
> > + idx = match_string(map->class_names, map->length, cls);
> > + if (idx < 0) {
> > + pr_err("%s unknown to %s\n", cls, kp->name);
> > + continue;
> > + }
> > +
> > + switch (map->map_type) {
> > + case DD_CLASS_TYPE_SYMBOLIC:
> > + if (test_bit(idx, &inbits) == wanted) {
> > + v3pr_info("no change on %s\n", cls);
> > + continue;
> > + }
> > + inbits ^= BIT(idx);
>
> Didn't test this out but the code here confused me. In this case the bit at idx
> in inbits doesn't match. But you are doing an exclusive OR here. So doesn't that
> always set it? Shouldn't it be cleared if wanted is false?
>
I think the trouble is - in part - inbits varname;
it starts with the old bit vector value -
inbits = *dcp->bits; // i'll add comment here
2nd, this is while parsing the csv of classnames,
and evaluating 1 bit/class at a time here,
and only valid class-names
and wanted therefore only pertains to classes/bits that need toggled,
wanted = 1/0 based on +/- setting for that class.
>
> > + break;
> > + case DD_CLASS_TYPE_LEVELS:
> > + /* bitmask must respect classmap ranges, this does not */
> > + inbits = (1 << (idx + wanted));
>
> This line also confused me - below in DD_CLASS_TYPE_VERBOSE: case you use the
> CLASSMAP_BITMASK() which will set all the 'levels' below. So I was expecting
> that here as well as this is the symbolic level case. I think I'm missing
> something...
So this is a bit inscrutable - and the comment needs refresh.
again, we are taking classnames here, so are working a single class-id,
this time the bits are not independent, as they were above.
idx is the valid class-id pertaining to the classname
- I'll pick a better varname here too
so
inbits = (1<< ( cls_id + wanted ? 1 : 0 ))
forces all bits below cls_id on, and maybe cls_id too.
at bottom of the classnames loop, apply-bitmap applies the bits.
it applies only changes made to inbits, by re-fetching orig dcp->bits
and comparing.
Note also the usage diffs for the 2 _NAMED styles
echo DRM_UT_CORE,-DRM_UT_KMS,+DRM_UT_DRIVER > debug_catnames
echo +NV_TRACE > nv_debug_level_names
while this would be weird / makework / stress-test
echo +V7,-V1,+V7,-V1 > /sys/module/test_dynamic_debug/p_level_names
Ive been reworking the set - including your enum suggestions,
also adding
0010-dyndbg-cleanup-local-vars-in-ddebug_init.patch
0011-dyndbg-create-and-use-struct-_ddebug_info.patch
- this gathers __dyndbg and __dyndbg_classes sections
- maybe infos, or something more subsystem-state-ey
any suggestions ?
I'll change varnames I mentioned, and add comments.
I'll add comments derived from this explanation
(hope it clears things up)
and have a few things to sort and retest, I'll send a new rev shortly.
