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, ¶m_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