On 3/11/22 20:06, jim.cromie@xxxxxxxxx wrote:
> On Fri, Mar 11, 2022 at 12:06 PM Jason Baron <jbaron@xxxxxxxxxx> wrote:
>>
>>
>>
>> On 3/10/22 23:47, Jim Cromie wrote:
>>> DRM defines/uses 10 enum drm_debug_category's to create exclusive
>>> classes of debug messages. To support this directly in dynamic-debug,
>>> add the following:
>>>
>>> - struct _ddebug.class_id:4 - 4 bits is enough
>>> - define _DPRINTK_SITE_UNCLASSED 15 - see below
>>>
>>> and the query support:
>>> - struct _ddebug_query.class_id
>>> - ddebug_parse_query - looks for "class" keyword, then calls..
>>> - parse_class - accepts uint: 0-15, sets query.class_id and marker
>>> - vpr_info_dq - displays new field
>>> - ddebug_proc_show - append column with "cls:%d" if !UNCLASSED
>>>
>>> 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:
>>
>
> Im not at all averse to nice names, but as something added on.
>
> 1st, the interface to make friendlier is DRM's
>
> echo 0x04 > /sys/module/drm/parameters/debug # which category ?
>
> parm: debug:Enable debug output, where each bit enables a
> debug category.
> Bit 0 (0x01) will enable CORE messages (drm core code)
> Bit 1 (0x02) will enable DRIVER messages (drm controller code)
> Bit 2 (0x04) will enable KMS messages (modesetting code)
>
> echo DRM_UT_DRIVER,DRM_UT_KMS > /sys/module/drm/parameters/debug #
> now its pretty clear
>
> 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.
>
> from there, it shouldnt be hard to ref that from the module's ddebug_table,
> so parse_query could validate class args against the module given (or
> fail if none given)
>
> Speaking strictly, theres a gap:
> echo module * class DRM_UT_KMS +p > control
> is nonsense for * other than drm + drivers,
> but its fair/safe to just disallow wildcards on modname for this purpose.
>
> it does however imply that module foo must exist for FOO_CAT_1 to be usable.
> thats not currently the case:
> bash-5.1# echo module foo +p > /proc/dynamic_debug/control
> [ 15.403749] dyndbg: read 14 bytes from userspace
> [ 15.405413] dyndbg: query 0: "module foo +p" mod:*
> [ 15.406486] dyndbg: split into words: "module" "foo" "+p"
> [ 15.407070] dyndbg: op='+'
> [ 15.407388] dyndbg: flags=0x1
> [ 15.407809] dyndbg: *flagsp=0x1 *maskp=0xffffffff
> [ 15.408300] dyndbg: parsed: func="" file="" module="foo" format=""
> lineno=0-0 class=15
> [ 15.409151] dyndbg: no matches for query
> [ 15.409591] dyndbg: no-match: func="" file="" module="foo"
> format="" lineno=0-0 class=15
> [ 15.410524] dyndbg: processed 1 queries, with 0 matches, 0 errs
> bash-5.1#
>
> ISTM we can keep that "0 errs" response for that case, but still reject this:
>
> echo module foo class FOO_NOT_HERE +p > /proc/dynamic_debug/control
>
>
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?).
>> enum levels {
>> LOW,
>> MEDIUM,
>> HIGH
>> };
>
> I want to steer clear of "level" anything,
> since 2>1 implies non independence of the categories
>
>
Agreed, that was a bad example on my part.
I've put together a rough patch on top of your series, to make my thinking
hopefully clear. I also think that the module param callback thing could be
implemented in this 'global' space with the 0-14 low numbers, by adding
some sort of offset into the table. IE you would add the low number +
the offset to get the 'string' to add to the ddebug query. I commented it
out in my patch below b/c I didn't implement that part.
Thoughts?
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 664bb83778d2..b0bc1b536d54 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -8,6 +8,14 @@
#include <linux/build_bug.h>
+enum ddebug_categories {
+ FOO_BAR = 0,
+ DRM_A = 1,
+ DRM_B = 2,
+ DRM_C = 3,
+ //_DPRINTK_SITE_DEFAULT 63 is max
+};
+
/*
* An instance of this structure is created in a special
* ELF section at every dynamic debug callsite. At runtime,
@@ -23,9 +31,9 @@ struct _ddebug {
const char *filename;
const char *format;
unsigned int lineno:18;
-#define CLS_BITS 4
+#define CLS_BITS 6
unsigned int class_id:CLS_BITS;
-#define _DPRINTK_SITE_UNCLASSED ((1 << CLS_BITS) - 1)
+#define _DPRINTK_SITE_DEFAULT ((1 << CLS_BITS) - 1)
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -101,11 +109,11 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
.class_id = cls, \
_DPRINTK_KEY_INIT \
}; \
- BUILD_BUG_ON_MSG(cls > _DPRINTK_SITE_UNCLASSED, \
+ BUILD_BUG_ON_MSG(cls > _DPRINTK_SITE_DEFAULT, \
"classid value overflow")
#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
- DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, _DPRINTK_SITE_UNCLASSED, fmt)
+ DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, _DPRINTK_SITE_DEFAULT, fmt)
#ifdef CONFIG_JUMP_LABEL
@@ -149,11 +157,11 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
} while (0)
#define __dynamic_func_call(id, fmt, func, ...) \
- __dynamic_func_call_cls(id, _DPRINTK_SITE_UNCLASSED, \
+ __dynamic_func_call_cls(id, _DPRINTK_SITE_DEFAULT, \
fmt, func, ##__VA_ARGS__)
#define __dynamic_func_call_no_desc(id, fmt, func, ...) \
- __dynamic_func_call_no_desc_cls(id, _DPRINTK_SITE_UNCLASSED, \
+ __dynamic_func_call_no_desc_cls(id, _DPRINTK_SITE_DEFAULT, \
fmt, func, ##__VA_ARGS__)
/*
@@ -167,7 +175,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
#define _dynamic_func_call_cls(cls, fmt, func, ...) \
__dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
#define _dynamic_func_call(fmt, func, ...) \
- _dynamic_func_call_cls(_DPRINTK_SITE_UNCLASSED, fmt, func, ##__VA_ARGS__)
+ _dynamic_func_call_cls(_DPRINTK_SITE_DEFAULT, fmt, func, ##__VA_ARGS__)
/*
* A variant that does the same, except that the descriptor is not
@@ -180,7 +188,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
#define _dynamic_func_call_no_desc(fmt, func, ...) \
__dynamic_func_call_no_desc_cls(__UNIQUE_ID(ddebug), \
- _DPRINTK_SITE_UNCLASSED, \
+ _DPRINTK_SITE_DEFAULT, \
fmt, func, ##__VA_ARGS__)
#define dynamic_pr_debug(fmt, ...) \
@@ -284,7 +292,7 @@ struct dyndbg_classbits_param {
static struct dyndbg_classbits_param ddcats_##_var = { \
.bits = &(_var), \
.flags = _flgs, \
- .classes = { __VA_ARGS__, _DPRINTK_SITE_UNCLASSED } \
+ .classes = { __VA_ARGS__, _DPRINTK_SITE_DEFAULT } \
}; \
module_param_cb(fsname, ¶m_ops_dyndbg_classbits, \
&ddcats_##_var, 0644)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 11c8d0771cd2..0f390638e46c 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -39,6 +39,14 @@
#include <rdma/ib_verbs.h>
+static const char * const cat_to_strings[] = {
+ [FOO_BAR] = "foo:bar",
+ [DRM_A] = "drm:A",
+ [DRM_B] = "drm:B",
+ [DRM_C] = "drm:C",
+ [_DPRINTK_SITE_DEFAULT] = "default",
+};
+
extern struct _ddebug __start___dyndbg[];
extern struct _ddebug __stop___dyndbg[];
@@ -55,8 +63,7 @@ struct ddebug_query {
const char *function;
const char *format;
unsigned int first_lineno, last_lineno;
- unsigned int class_id;
- unsigned int class_marked:1;
+ const char *class_string;
};
struct ddebug_iter {
@@ -136,13 +143,13 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
fmtlen--;
}
- v3pr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u class=%u\n",
+ v3pr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u class=%s\n",
msg,
query->function ?: "",
query->filename ?: "",
query->module ?: "",
fmtlen, query->format ?: "",
- query->first_lineno, query->last_lineno, query->class_id);
+ query->first_lineno, query->last_lineno, query->class_string);
}
/*
@@ -158,7 +165,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct ddebug_table *dt;
unsigned int newflags;
unsigned int nfound = 0;
- struct flagsbuf fbuf;
+ struct flagsbuf fbuf, nbuf;
/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
@@ -172,8 +179,8 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = &dt->ddebugs[i];
- /* match against the class_id, either given or default */
- if (query->class_id != dp->class_id)
+ /* if class is not set fall through, ot check if it matches */
+ if (query->class_string && strcmp(query->class_string, cat_to_strings[dp->class_id]))
continue;
/* match against the source filename */
@@ -223,11 +230,12 @@ static int ddebug_change(const struct ddebug_query *query,
static_branch_enable(&dp->key.dd_key_true);
}
#endif
+ v4pr_info("changed %s:%d [%s]%s %s -> %s\n",
+ trim_prefix(dp->filename), dp->lineno,
+ dt->mod_name, dp->function,
+ ddebug_describe_flags(dp->flags, &fbuf),
+ ddebug_describe_flags(newflags, &nbuf));
dp->flags = newflags;
- v4pr_info("changed %s:%d [%s]%s =%s\n",
- trim_prefix(dp->filename), dp->lineno,
- dt->mod_name, dp->function,
- ddebug_describe_flags(dp->flags, &fbuf));
}
}
mutex_unlock(&ddebug_lock);
@@ -314,21 +322,6 @@ static inline int parse_lineno(const char *str, unsigned int *val)
return 0;
}
-static inline int parse_class(struct ddebug_query *query, const char *str)
-{
- int rc;
- unsigned int val;
-
- rc = kstrtouint(str, 10, &val);
- if (rc < 0 || val > _DPRINTK_SITE_UNCLASSED) {
- pr_err("expecting class:[0-%d], not %s\n", _DPRINTK_SITE_UNCLASSED, str);
- return -EINVAL;
- }
- query->class_id = val;
- query->class_marked = 1;
- return 0;
-}
-
static int parse_linerange(struct ddebug_query *query, const char *first)
{
char *last = strchr(first, '-');
@@ -443,8 +436,7 @@ static int ddebug_parse_query(char *words[], int nwords,
if (parse_linerange(query, arg))
return -EINVAL;
} else if (!strcmp(keyword, "class")) {
- if (parse_class(query, arg))
- return -EINVAL;
+ rc = check_set(&query->class_string, arg, "class");
} else {
pr_err("unknown keyword \"%s\"\n", keyword);
return -EINVAL;
@@ -452,9 +444,6 @@ static int ddebug_parse_query(char *words[], int nwords,
if (rc)
return rc;
}
- /* post-validate the query, set default */
- if (!query->class_marked)
- query->class_id = _DPRINTK_SITE_UNCLASSED;
vpr_info_dq(query, "parsed");
return 0;
@@ -616,6 +605,8 @@ int param_set_dyndbg_classbits(const char *instr, const struct kernel_param *kp)
}
vpr_info("set_dyndbg_classbits: new 0x%lx old 0x%lx\n", inbits, *dcp->bits);
+
+/*
for (i = 0; (i < _DPRINTK_SITE_UNCLASSED &&
dcp->classes[i] < _DPRINTK_SITE_UNCLASSED); i++) {
@@ -630,6 +621,7 @@ int param_set_dyndbg_classbits(const char *instr, const struct kernel_param *kp)
matches, dcp->classes[i]);
totct += matches;
}
+*/
*dcp->bits = inbits;
vpr_info("total matches: %d\n", totct);
return 0;
@@ -978,8 +970,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
seq_escape(m, dp->format, "\t\r\n\"");
seq_puts(m, "\"");
- if (dp->class_id != _DPRINTK_SITE_UNCLASSED)
- seq_printf(m, " cls:%u", dp->class_id);
+ seq_printf(m, " cat:%s", cat_to_strings[dp->class_id]);
seq_puts(m, "\n");
return 0;
