Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220311044756.425777-1-jim.cromie@gmail.com> <20220311044756.425777-3-jim.cromie@gmail.com>
 <823e51e6-2af4-7854-9428-697a2af12488@akamai.com> <CAJfuBxxVti_pa1YPmas=Ub28yWUFFGeR13wxveLvPCYS61NxuA@mail.gmail.com>
 <0d00c529-3bac-f09f-e07c-584194251a06@akamai.com>
In-Reply-To: <0d00c529-3bac-f09f-e07c-584194251a06@akamai.com>
From:   jim.cromie@gmail.com
Date:   Mon, 28 Mar 2022 13:07:27 -0600
Message-ID: <CAJfuBxwxA-9EwBaEow2UTBXD5-iER03+f5=D1zCUjTUut_bQaw@mail.gmail.com>
Subject: Re: [PATCH 2/5] dyndbg: add class_id field and query support
To:     Jason Baron <jbaron@akamai.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Sean Paul <seanpaul@chromium.org>, robdclark@gmail.com,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Joe Perches <joe@perches.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        amd-gfx mailing list <amd-gfx@lists.freedesktop.org>,
        intel-gvt-dev@lists.freedesktop.org,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Mar 14, 2022 at 3:30 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 3/11/22 20:06, jim.cromie@gmail.com wrote:
> > On Fri, Mar 11, 2022 at 12:06 PM Jason Baron <jbaron@akamai.com> wrote:
> >>
> >>
> >>
> >> On 3/10/22 23:47, Jim Cromie wrote:
> >>>
> >>> With the patch, one can enable current/unclassed callsites by:
> >>>
> >>>   #> echo drm class 15 +p > /proc/dynamic_debug/control
> >>>
> >>
> >> To me, this is hard to read, what the heck is '15'? I have to go look it
> >> up in the control file and it's not descriptive. I think that using
> >> classes/categories makes sense but I'm wondering if it can be a bit more
> >> user friendly? Perhaps, we can pass an array of strings that is indexed
> >> by the class id to each pr_debug() site that wants to use class. So
> >> something like:

hi Jason,
Im now in basically full agreement with you

1.   .class_id  is a "global" space, in that all callsites have one.
2.    0-15 is an exceedingly small range for a global space

Fix that by
3. make it private (by removing "class N" parsing)
   now nobody can do
   echo module * class N +p >control

4. add private/per-module "string" -> class_id map
    each module will have to declare the class-strings they use/accept
    opt-in - want coordinated / shared names for DRM_UT_KMS etc.

5. validate input using the known class_string -> class_id

then, this is knowably right or wrong, depending on DRM_FOO:
     echo module drm class DRM_FOO +p > control

it also means that
     echo class DRM_UT_KMS +p >control
is both wellformed and minimal;
any module that has DRM_UT_KMS defined will know which class_id *they*
have mapped it to.
theres no global "DRM_UT_KMS" to be abused.

So Ive been working towards that..
Heres my current biggest roadblock

DEFINE_DYNAMIC_DEBUG_CLASSBITS
creates the class-strings[] array declaratively, at compile-time.
This array is attached to the kernel-param.args,
so it can be used by the callbacks (to construct the command)

But its not obviously available from outside the sysfs knob
that its attached to, as is needed to apply command >control generally.

If I can attach the class-strings[]  to struct ddebug_table,
then ddebug_change() can use it to validate >control input.

So, how to attach ?
its got to work for both builtin & loadable modules.
(which precludes something in struct module ?)

I looked for a kernel_param_register callback, came up empty.
Trying to add it feels invasive / imposing.


> >
> > If that works, its cuz DEFINE_DYNAMIC_DEBUG_CLASSBITS()
> > plucks class symbols out of its __VA_ARGS__, and #stringifes them.
> > So that macro could then build the 1-per-module static constant string array
> > and (only) the callbacks would be able to use it.
> >

So Ive been tinkering hard on this macro, since its pretty central to
the interface defn.
heres some choices:

this is what Ive been working towards.
using enum symbols directly like this associates them by grep,
in contrast, class-strings are mealy-mouthed, milquetoast.

DEFINE_DYNAMIC_DEBUG_CLASSBITS(debug, __drm_debug, "p",
        "enable drm.debug categories - 1 bit per category",
        DRM_UT_CORE,
        DRM_UT_DRIVER,
        DRM_UT_KMS,
        DRM_UT_PRIME,
        DRM_UT_ATOMIC,
        DRM_UT_VBL,
        DRM_UT_STATE,
        DRM_UT_LEASE,
        DRM_UT_DP,
        DRM_UT_DRMRES);

 I found a slick MAP ( ) macro to do this:

#define DEFINE_DYNAMIC_DEBUG_CLASSBITS(fsname, _var, _flgs, desc, ...) \
  MODULE_PARM_DESC(fsname, desc); \
  static struct dyndbg_classbits_param ddcats_##_var = { \
    .bits = &(_var), \
    .flags = _flgs, \
    .classes = { __VA_ARGS__, _DPRINTK_CLASS_DFLT }, \
    .class_names = { mgMAP(__stringify, sCOMMA, \
                                                __VA_ARGS__,
_DPRINTK_CLASS_DFLT) } \
}; \
module_param_cb(fsname, &param_ops_dyndbg_classbits, \
&ddcats_##_var, 0644)

 __VA_ARGS__   is used 2x
.class_names is available for validating command >control

As much as I like the above, the MAP macro has a longer, more risky
path to the kernel

so a more modest alternative: module user defines class-strings in interface,
but they *must* align manually with the enum values they correspond to;
the order determines the bit-pos in the sysfs node,
since the interface no longer provides the enum values themselves.

DEFINE_DYNAMIC_DEBUG_CLASS_STRINGS(debug, __drm_debug, "p",
        "enable drm.debug categories - 1 bit per category",
        "DRM_UT_CORE",
        "DRM_UT_DRIVER",
        "DRM_UT_KMS",

different name allows CLASSBITs or similar in future, if MAP works out.
class-strings are completely defined by users, drm can drop UT naming

TLDR: FWIW

iSTM that  the same macro will support the coordinated use of class-strings
across multiple modules.

drm_print.c - natural home for exposed sysfs node

amdgpu/, drm_helper/ i915/  nouveau/  will all need a DEFINE_DD added,
so that ddebug_change() can allow those .class_ids to be controlled.
sysfs perm inits can disable their nodes, since theyre coordinated.

> >
>
> Ok, yeah so I guess I'm thinking about the 'class' more as global namespace,
> so that for example, it could span modules, or be specific to certain modules.
> I'm also thinking of it as a 'string' which is maybe hierarchical, so that it's
> clear what sub-system, or sub-sub-system it belongs to. So for DRM for example,
> it could be a string like "DRM:CORE". The index num I think is still helpful for
> implementation so we don't have to store a pointer size, but I don't think it's
> really exposed (except perhaps in module params b/c drm is doing that already?).
>

So what Ive got here is as described above,
I just need a few bright ideas,
then I can bring it together.
got a spare tuit?

Jim