Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

jim.cromie
2020-06-11T21:19:58Z

trimmed..

> > > Currently I think there not enough "levels" to map something like
> > > drm.debug to the new dyn dbg feature. I don't think it is intrinsic
> > > but I couldn't find the bit of the code where the 5-bit level in struct
> > > _ddebug is converted from a mask to a bit number and vice-versa.
> >
> > Here [1] is Joe's initial suggestion. But I decided that bitmask is a
> > good start for the discussion.
> >
> > I guess we can add new member uint "level" in struct _ddebug so that we
> > can cover more "levels" (types, groups).
>
> I don't think it is allocating only 5 bits that is the problem!
>
> The problem is that those 5 bits need not be encoded as a bitmask by
> dyndbg, that can simply be the category code for the message. They only
> need be converted into a mask when we compare them to the mask provided
> by the user.
>
>
> Daniel.

heres what I have in mind. whats described here is working.
I'll send it out soon

commit 20298ec88cc2ed64269c8be7b287a24e60a5347e
Author: Jim Cromie <jim.cromie@gmail.com>
Date: Wed Jun 10 12:55:08 2020 -0600

dyndbg: WIP towards module->debugflags based callsite controls

There are *lots* of ad-hoc debug printing solutions in kernel,
this is a 1st attempt at providing a common mechanism for many of them.

Basically, there are 2 styles of debug printing:
- levels, with increasing verbosity, 1-10 forex
- bits/flags, independently controlling separate groups of dprints

This patch does bits/flags (with no distinction made yet between 2)

API:

- change pr_debug(...) --> pr_debug_typed(type_id=0, ...)
- all existing uses have type_id=0
- developer creates exclusive types of log messages with type_id>0
1, 2, 3 are disjoint groups, for example: hi, mid, low

- !!type_id is just an additional callsite selection criterion

Qfoo() { echo module foo $* >/proc/dynamic_debug/control }
Qfoo +p # all groups, including default 0
Qfoo mflags 1 +p # only group 1
Qfoo mflags 12 +p # TBD[1]: groups 1 or 2
Qfoo mflags 0 +p # ignored atm TBD[2]
Qfoo mflags af +p # TBD[3]: groups a or f (10 or 15)

so patch does:

- add u32 debugflags to struct module. Each bit is a separate print-class.

- add unsigned int mflags into struct ddebug_query
mflags matched in ddebug_change

- add unsigned int type_id:5 to struct _ddebug
picks a single debugflag bit. No subclass or multitype nonsense.
nice and dense, packs with other members.
we will have a lot of struct _ddebugs.

- in ddebug_change()
filter on !! module->debugflags,
IFF query->module is given, and matches dt->mod_name
and query->mflags is given, and bitmatches module->debugflags

- in parse_query()
accept new query term: mflags $arg
populate query->mflags
arg-type needs some attention, but basic plumbing is there

WIP: not included:

- pr_debug_typed( bitpos=0, ....)
aka: pr_debug_class() or pr_debug_id()
the bitpos is 1<<shift, allowing a single type. no ISA relations.
this covers OP's high,mid,low case, many others

- no way to exersize new code in ddebug_change
need pr_debug_typed() to make a (not-null) typed callsite.
also no way to set module->debugflags

Im relying on:
cdf6d00696 dynamic_debug: don't duplicate modname in ddebug_add_module

which copies the ptr-val from module->name to dt->mod_name, which
allowed == tests instead of strcmp.

That equivalence and a (void*) cast in use of container_of() seem to
do the trick to get the module, then module->debugflags.

Notes:

1- A query ANDs all its query terms together, so Qfoo() above
requires both "module foo" AND all additional query terms given in $*

But since callsite type_id creates disjoint groups, "mflags 12" is
nonsense if it means groups 1 AND 2. Here, 1 OR 2 is meaningful, if
its not judged to be too confusing.

2- im not sure what this does atm, or should do
Qfoo mflags 0 +p # select only untyped ? or no flags check at all ?

3- since modflags has 32 bits, [1-9a-v] could select any single type_id
it is succinct, but arcane.
its a crude alphanumeric map for developers to make flags mnemonic
maybe query keyword should be mbits