Message-ID: <e65d2c81-6d0b-3c1e-582c-56d707c0d1f1@akamai.com>
Date:   Thu, 11 Jun 2020 17:59:18 -0400
From:   Jason Baron <jbaron@...mai.com>
To:     jim.cromie@...il.com, Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Joe Perches <joe@...ches.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-media@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-btrfs@...r.kernel.org,
        linux-acpi@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible



On 6/11/20 5:19 PM, jim.cromie@...il.com wrote:
> 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

Cool. thanks for working on this!

> 
> commit 20298ec88cc2ed64269c8be7b287a24e60a5347e
> Author: Jim Cromie <jim.cromie@...il.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)


I think it might be nice to have this proposal to integrate level too
so we see how it fits in. Maybe level is just about we enable things?
So you still mark callsites with pr_debug_typed(), but we can eanble
them via something like, 'echo module foo level N +p  >
/proc/dynamic_debug/control'. And that and 'p' to all callsites <= N.
So anybody using pr_debug_typed() can use either style to enable/disable.


> 
>     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

So I thought this meant select group twelve. Aren't there 32 groups?

>       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.
> 

what is this for again?

>     - 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 ?

seems like it should group 0 which you're calling untyped. So you can write group
0 call sites as pr_debug(); or pr_debug_typed(0, ...); I'm not sure why we should
treat it differently other than it can be written without the pr_debug_typed().

> 
>     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
> 

Also wondering if the control file should display the type (if non-zero). I think
that would be nice.

Thanks,

-Jason

Powered by blists - more mailing lists