On Mon, Aug 23, 2021 at 1:21 AM Jim Cromie jim.cromie@gmail.com wrote:
DEFINE_DYNAMIC_DEBUG_CATEGORIES(name, var, bitmap_desc, @bit_descs) allows users to define a drm.debug style (bitmap) sysfs interface, and to specify the desired mapping from bits[0-N] to the format-prefix'd pr_debug()s to be controlled.
DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, "i915/gvt bitmap desc", /** * search-prefixes, passed to dd-exec_queries * defines bits 0-N in order. * leading ^ is tacitly inserted (by callback currently) * trailing space used here excludes subcats. * helper macro needs more work * macro to autogen ++$i, 0x%x$i ? */ _DD_cat_("gvt:cmd: "), _DD_cat_("gvt:core: "), _DD_cat_("gvt:dpy: "), _DD_cat_("gvt:el: "), _DD_cat_("gvt:irq: "), _DD_cat_("gvt:mm: "), _DD_cat_("gvt:mmio: "), _DD_cat_("gvt:render: "), _DD_cat_("gvt:sched: "));
dynamic_debug.c: add 3 new elements:
- int param_set_dyndbg()
- int param_get_dyndbg()
- struct kernel_param_ops param_ops_dyndbg
Following the model of kernel/params.c STANDARD_PARAM_DEFS, All 3 are non-static and exported.
dynamic_debug.h:
Add DEFINE_DYNAMIC_DEBUG_CATEGORIES() described above, and a do-nothing stub.
Note that it also calls MODULE_PARM_DESC for the user, but expects the user to catenate all the bit-descriptions together (as is done in drm.debug), and in the following uses in amdgpu, i915.
This in the hope that someone can offer an auto-incrementing label-generating macro, producing "\tbit-4 0x10\t" etc, and can show how to apply it to __VA_ARGS__.
Also extern the struct kernel_param param_ops_dyndbg symbol, as is done in moduleparams.h for all the STANDARD params.
USAGE NOTES:
Using dyndbg to query on "format ^$prefix" requires that the prefix be present in the compiled-in format string; where run-time prefixing is used, that format would be "%s...", which is not usefully selectable.
Adding structural query terms (func,file,lineno) could help (module is already done), but DEFINE_DYNAMIC_DEBUG_CATEGORIES can't do that now, adding it needs a better reason imo.
Dyndbg is completely agnostic wrt the categorization scheme used, to play well with any prefix convention already in use. Ad-hoc categories and sub-categories are implicitly allowed, author discipline and review is expected.
Here are some examples:
"1","2","3" 2 doesn't imply 1. otherwize, sorta like printk levels "1:","2:","3:" are better, avoiding [1-9]\d+ ambiguity "hi:","mid:","low:" are reasonable, and imply independence "todo:","rfc:","fixme:" might be handy "A:".."Z:" uhm, yeah
Hierarchical classes/categories are natural:
"drm:<CAT>:" is used in later commit "drm:<CAT>:<SUB>:" is a natural extension. "drm:atomic:fail:" has been proposed, sounds directly useful
Some properties of a hierarchical category deserve explication:
Trailing spaces matter !
With 1..3-space ("drm: ", "drm:atomic: ", "drm:atomic:fail: "), the ":" doesn't terminate the search-space, the trailing space does. So a "drm:" search spec will match all DRM categories & subcategories, and will not be useful in an interface where all categories are already controlled together. That said, "drm:atomic:" & "drm:atomic: " are different, and both are useful in cases.
Ad-Hoc sub-categories:
These have a caveat wrt wrapper macros adding prefixes like "drm:atomic: "; the trailing space in the prefix means that drm_dbg_atomic("fail: ...") pastes as "drm:atomic: fail: ", which obviously isn't ideal wrt clear and simple bitmaps.
A possible solution is to have a FOO_() version of every FOO() macro which (anti-mnemonically) elides the trailing space, which is normally inserted by a modified FOO(). Doing this would enforce a policy decision that "debug categories will be space terminated", with an pressure-relief valve.
Summarizing:
- "drm:kms: " & "drm:kms:" are different
- "drm:kms" also different - includes drm:kms2:
- "drm:kms:\t" also different
- "drm:kms:*" doesn't work, no wildcard on format atm.
Order matters in DEFINE_DYNAMIC_DEBUG_CATEGORIES(... @bit_descs)
@bit_descs (array) position determines the bit mapping to the prefix, so to keep a stable map, new categories or 3rd level categories must be added to the end.
Since bits are/will-stay applied 0-N, the later bits can countermand the earlier ones, but its tricky - consider;
it's
DD_CATs(... "drm:atomic:", "drm:atomic:fail:" ) // misleadingThe 1st search-term is misleading, because it includes (modifies) subcategories, but then 2nd overrides it. So don't do that.
For "drm:atomic:fail:" in particular, its best not to add it into an existing bitmap, because the current setting would be lost at every (unrelated) write, and a separate bitmap is much more stable.
There is still plenty of bikeshedding to do.
...
@@ -51,8 +51,6 @@ struct _ddebug { #endif } __attribute__((aligned(8)));
stray change.
...
+static inline int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
{ return 0; }+static inline int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
{ return 0; }
Not sure if it's aligned with the style in this file, but usually we put { at the very beginning of the line.
...
-/* handle multiple queries in query string, continue on error, return
- last error or number of matching callsites. Module name is either
- in param (for boot arg) or perhaps in query string.
-*/ +/*
- handle multiple queries in query string, continue on error, return
- last error or number of matching callsites. Module name is either
- in param (for boot arg) or perhaps in query string.
- */
Doesn't belong to the patch, split it separately.
...
vpr_info("query %d: \"%s\" %s\n", i, query, (modname) ? modname : "");
too many parentheses. Also may use
modname ?: ""
form (but not all maintainers are happy with it).
...
if (!bitmap) {pr_err("set_dyndbg: no bits=>queries map\n");return -EINVAL;}rc = kstrtoul(instr, 0, &inbits);if (rc) {pr_err("set_dyndbg: failed\n");return rc;}vpr_info("set_dyndbg: input 0x%lx\n", inbits);for (i = 0; bitmap->prefix; i++, bitmap++) {snprintf(query, FMT_QUERY_SIZE, "format '^%s' %cp", bitmap->prefix,test_bit(i, &inbits) ? '+' : '-');matches = ddebug_exec_queries(query, KP_MOD_NAME);v2pr_info("bit-%d: %d matches on '%s'\n", i, matches, query);totct += matches;}
I'm wondering if there is a room to parse a bitmap as a bitmap.
...
+int param_get_dyndbg(char *buffer, const struct kernel_param *kp) +{
return scnprintf(buffer, PAGE_SIZE, "%u\n",*((unsigned int *)kp->arg));
One line?
+}