Patchset starts with 9 "cleanups";
- change section name from vague "__verbose" to "__dyndbg"
- cleaner docs, drop obsolete comment & useless debug prints,
refine verbosity, fix a BUG_ON, ram reporting miscounts.
It adds a few query parsing conveniences;
accept combined file:line & file:func forms
file inode.c:100-200 # file & line-range
file inode.c:start_* # file & function
Then it expands flags:
Adds 'u' user flag, allowing user to compose an arbitrary set of
callsites by marking them with 'u', without altering current
print-modifying flags.
Adds 'PFMLTU' flags, which negate their lower-case counterparts.
Adds filter-flags, which select callsites for modification based upon
their current flags. This lets user activate the set of callsites
marked with 'u' in a batch.
echo 'u+p' > control
By using negating-flags in your filter, you can match on an exact
flagstate; not just required flags, but prohibited ones.
So if youre using dyndbg to track 2 separate problems, you can use
different patterns of [fmltu] flags to keep the 2 sets of pr-debug
activations separate.
By also using negating-flags in your modflags (after the [+-=] op) you
can fully control the new flagstate, or modify any part of it (unlike
if =p is used).
Also, a late addition:
parse args like: file=afile:afunc module:kvm*
I tried to drag this up to cleanups, but has a conflict so I punted.
WIP print-classes (new from previous)
cc: <[email protected]>
A patchset from Stanimir tried to create mutually exclusive HI, MID,
LO print-classes for his module, which drove some discussion. These
patches implement a working scheme:
The whole macro stack in dynamic_debug.h is adapted to *_cl()
versions, and old names are redefined to use the new *_cl names, with
a default print-class=0.
pr_debug_n(pr_class>0) provides a way to "create" HI MID LO classes,
by just claiming an N>0. print-classes are meant to be module-scoped;
so manual allocation of them in module #defines is practical.
"mflags <print-classes>" the query term is implemented,
crudely but functional.
To partly enforce print-class locality to a module, "module foo" is
required if "mflags flags" is used; but this can be defeated by
"module *". Its easy to error out if '*' is used in query, but maybe
"module kvm*" should work, as kvm* developers could probably reach
agreement on print-class constants
Use of above is demonstrated in a patch to samples/kobject/kset-example
I hope this WIP doesnt distract from the non-WIP stuff.
Previous submission (in this release cycle)
https://lkml.org/lkml/2020/6/5/766
changes since:
WIP print-classes
rebase over a recent conflict in master
2 more "cleanups": Docs ~= s/arch_init/early_init/, maybe macro
else coding style fix. thx JB
commit message corrections, improvements based on comments
Jim Cromie (24):
cleanups:
dyndbg-docs: eschew file /full/path query in docs
dyndbg-docs: initialization is done early, not arch
dyndbg: drop obsolete comment on ddebug_proc_open
dyndbg: refine debug verbosity; 1 is basic, 2 more chatty
dyndbg: rename __verbose section to __dyndbg
dyndbg: fix overcounting of ram used by dyndbg
dyndbg: fix a BUG_ON in ddebug_describe_flags
dyndbg: make ddebug_tables list LIFO for add/remove_module
dyndbg: add maybe(str,"") macro to reduce code
file:(func|lines)
dyndbg: refactor parse_linerange out of ddebug_parse_query
dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'
internal rework
dyndbg: refactor ddebug_read_flags out of ddebug_parse_flags
dyndbg: combine flags & mask into a struct, use that
dyndbg: add filter parameter to ddebug_parse_flags
dyndbg: extend ddebug_parse_flags to accept optional filter-flags
dyndbg: prefer declarative init in caller, to memset in callee
expose features
dyndbg: add user-flag, negating-flags, and filtering on flags
dyndbg: allow negating flag-chars in modflags
recent, not trivially backport
dyndbg: accept query terms like module:foo and file=bar
WIP
dyndbg: WIP towards debug-print-class based callsite controls
dyndbg: adapt header macros to pass print-class
dyndbg: add print-class as trailing number to control output
kset-example: add pr_debug()s for easy visibility of its operation
kset-example: use pr_debug_n to create example print-classes
.../admin-guide/dynamic-debug-howto.rst | 79 ++--
include/asm-generic/vmlinux.lds.h | 6 +-
include/linux/dynamic_debug.h | 75 ++--
include/linux/printk.h | 5 +-
kernel/module.c | 2 +-
lib/dynamic_debug.c | 369 +++++++++++-------
samples/kobject/kset-example.c | 32 ++
7 files changed, 374 insertions(+), 194 deletions(-)
--
2.26.2
since cf964976484 in 2012, initialization is done with early_initcall,
update the Docs, which still say arch_initcall.
---
Documentation/admin-guide/dynamic-debug-howto.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 57108f64afc8..1423af580bed 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -250,8 +250,8 @@ the syntax described above, but must not exceed 1023 characters. Your
bootloader may impose lower limits.
These ``dyndbg`` params are processed just after the ddebug tables are
-processed, as part of the arch_initcall. Thus you can enable debug
-messages in all code run after this arch_initcall via this boot
+processed, as part of the early_initcall. Thus you can enable debug
+messages in all code run after this early_initcall via this boot
parameter.
On an x86 system for example ACPI enablement is a subsys_initcall and::
--
2.26.2
dyndbg populates its callsite info into __verbose section, change that
to a more specific and descriptive name, __dyndbg.
Also, per checkpatch:
simplify __attribute(..) to __section(__dyndbg) declaration.
and 1 spelling fix, decriptor
---
include/asm-generic/vmlinux.lds.h | 6 +++---
include/linux/dynamic_debug.h | 4 ++--
kernel/module.c | 2 +-
lib/dynamic_debug.c | 12 ++++++------
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index db600ef218d7..05af5cef1ad6 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -320,9 +320,9 @@
*(__tracepoints) \
/* implement dynamic printk debug */ \
. = ALIGN(8); \
- __start___verbose = .; \
- KEEP(*(__verbose)) \
- __stop___verbose = .; \
+ __start___dyndbg = .; \
+ KEEP(*(__dyndbg)) \
+ __stop___dyndbg = .; \
LIKELY_PROFILE() \
BRANCH_PROFILE() \
TRACE_PRINTKS() \
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index abcd5fde30eb..aa9ff9e1c0b3 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -80,7 +80,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
static struct _ddebug __aligned(8) \
- __attribute__((section("__verbose"))) name = { \
+ __section(__dyndbg) name = { \
.modname = KBUILD_MODNAME, \
.function = __func__, \
.filename = __FILE__, \
@@ -133,7 +133,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
/*
* "Factory macro" for generating a call to func, guarded by a
- * DYNAMIC_DEBUG_BRANCH. The dynamic debug decriptor will be
+ * DYNAMIC_DEBUG_BRANCH. The dynamic debug descriptor will be
* initialized using the fmt argument. The function will be called with
* the address of the descriptor as first argument, followed by all
* the varargs. Note that fmt is repeated in invocations of this
diff --git a/kernel/module.c b/kernel/module.c
index e8a198588f26..1fb493167b9c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3232,7 +3232,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
if (section_addr(info, "__obsparm"))
pr_warn("%s: Ignoring obsolete parameters\n", mod->name);
- info->debug = section_objs(info, "__verbose",
+ info->debug = section_objs(info, "__dyndbg",
sizeof(*info->debug), &info->num_debug);
return 0;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c97872cffc8e..66c0bdf06ce7 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -39,8 +39,8 @@
#include <rdma/ib_verbs.h>
-extern struct _ddebug __start___verbose[];
-extern struct _ddebug __stop___verbose[];
+extern struct _ddebug __start___dyndbg[];
+extern struct _ddebug __stop___dyndbg[];
struct ddebug_table {
struct list_head link;
@@ -1019,7 +1019,7 @@ static int __init dynamic_debug_init(void)
int n = 0, entries = 0, modct = 0;
int verbose_bytes = 0;
- if (&__start___verbose == &__stop___verbose) {
+ if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
return 1;
@@ -1028,10 +1028,10 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
return 0;
}
- iter = __start___verbose;
+ iter = __start___dyndbg;
modname = iter->modname;
iter_start = iter;
- for (; iter < __stop___verbose; iter++) {
+ for (; iter < __stop___dyndbg; iter++) {
entries++;
verbose_bytes += strlen(iter->modname) + strlen(iter->function)
+ strlen(iter->filename) + strlen(iter->format);
@@ -1054,7 +1054,7 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
vpr_info("%d modules, %d entries and %d bytes in ddebug tables, %d bytes in (readonly) verbose section\n",
modct, entries, (int)(modct * sizeof(struct ddebug_table)),
- verbose_bytes + (int)(__stop___verbose - __start___verbose));
+ verbose_bytes + (int)(__stop___dyndbg - __start___dyndbg));
/* apply ddebug_query boot param, dont unload tables on err */
if (ddebug_setup_string[0] != '\0') {
--
2.26.2
The verbose/debug logging done for `cat $MNT/dynamic_debug/control` is
voluminous (2 per control file entry + 2 per PAGE). Moreover, it just
prints pointer and sequence, which is not useful to a dyndbg user.
So just drop them.
Also require verbose>=2 for several other debug printks that are a bit
too chatty for typical needs;
ddebug_change() prints changes, once per modified callsite. Since
queries like "+p" will enable ~2300 callsites in a typical laptop, a
user probably doesnt need to see them often. ddebug_exec_queries()
still summarizes with verbose=1.
ddebug_(add|remove)_module() also print 1 line per action on a module,
not needed by typical modprobe user.
This leaves verbose=1 better focussed on the >control parsing process.
---
lib/dynamic_debug.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2989a590ce9a..c97872cffc8e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -105,12 +105,15 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
return buf;
}
-#define vpr_info(fmt, ...) \
+#define vnpr_info(lvl, fmt, ...) \
do { \
- if (verbose) \
+ if (verbose >= lvl) \
pr_info(fmt, ##__VA_ARGS__); \
} while (0)
+#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
+#define v2pr_info(fmt, ...) vnpr_info(2, fmt, ##__VA_ARGS__)
+
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
/* trim any trailing newlines */
@@ -198,7 +201,7 @@ static int ddebug_change(const struct ddebug_query *query,
static_branch_enable(&dp->key.dd_key_true);
#endif
dp->flags = newflags;
- vpr_info("changed %s:%d [%s]%s =%s\n",
+ v2pr_info("changed %s:%d [%s]%s =%s\n",
trim_prefix(dp->filename), dp->lineno,
dt->mod_name, dp->function,
ddebug_describe_flags(dp, flagbuf,
@@ -771,8 +774,6 @@ static void *ddebug_proc_start(struct seq_file *m, loff_t *pos)
struct _ddebug *dp;
int n = *pos;
- vpr_info("called m=%p *pos=%lld\n", m, (unsigned long long)*pos);
-
mutex_lock(&ddebug_lock);
if (!n)
@@ -795,9 +796,6 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
struct ddebug_iter *iter = m->private;
struct _ddebug *dp;
- vpr_info("called m=%p p=%p *pos=%lld\n",
- m, p, (unsigned long long)*pos);
-
if (p == SEQ_START_TOKEN)
dp = ddebug_iter_first(iter);
else
@@ -818,8 +816,6 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
struct _ddebug *dp = p;
char flagsbuf[10];
- vpr_info("called m=%p p=%p\n", m, p);
-
if (p == SEQ_START_TOKEN) {
seq_puts(m,
"# filename:lineno [module]function flags format\n");
@@ -842,7 +838,6 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
*/
static void ddebug_proc_stop(struct seq_file *m, void *p)
{
- vpr_info("called m=%p p=%p\n", m, p);
mutex_unlock(&ddebug_lock);
}
@@ -905,7 +900,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
list_add_tail(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
- vpr_info("%u debug prints in module %s\n", n, dt->mod_name);
+ v2pr_info("%u debug prints in module %s\n", n, dt->mod_name);
return 0;
}
@@ -964,7 +959,7 @@ int ddebug_remove_module(const char *mod_name)
struct ddebug_table *dt, *nextdt;
int ret = -ENOENT;
- vpr_info("removing module \"%s\"\n", mod_name);
+ v2pr_info("removing module \"%s\"\n", mod_name);
mutex_lock(&ddebug_lock);
list_for_each_entry_safe(dt, nextdt, &ddebug_tables, link) {
--
2.26.2
during dyndbg init, verbose logging prints its ram overhead. It
counted strlens of struct _ddebug's 4 string members, in all callsite
entries, which would be approximately correct if each had been
mallocd. But they are pointers into shared .rodata; for example, all
10 kobject callsites have identical filename, module values.
Its best not to count that memory at all, since we cannot know they
were linked in because of CONFIG_DYNAMIC_DEBUG=y, and we want to
report a number that reflects what ram is saved by deconfiguring it.
Also fix wording and size under-reporting of the __dyndbg section.
Heres my overhead, on a virtme-run VM on a fedora-31 laptop:
dynamic_debug:dynamic_debug_init: 260 modules, 2479 entries \
and 10400 bytes in ddebug tables, 138824 bytes in __dyndbg section
---
lib/dynamic_debug.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 66c0bdf06ce7..9b2445507988 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1017,7 +1017,6 @@ static int __init dynamic_debug_init(void)
char *cmdline;
int ret = 0;
int n = 0, entries = 0, modct = 0;
- int verbose_bytes = 0;
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1033,9 +1032,6 @@ static int __init dynamic_debug_init(void)
iter_start = iter;
for (; iter < __stop___dyndbg; iter++) {
entries++;
- verbose_bytes += strlen(iter->modname) + strlen(iter->function)
- + strlen(iter->filename) + strlen(iter->format);
-
if (strcmp(modname, iter->modname)) {
modct++;
ret = ddebug_add_module(iter_start, n, modname);
@@ -1052,9 +1048,9 @@ static int __init dynamic_debug_init(void)
goto out_err;
ddebug_init_success = 1;
- vpr_info("%d modules, %d entries and %d bytes in ddebug tables, %d bytes in (readonly) verbose section\n",
+ vpr_info("%d modules, %d entries and %d bytes in ddebug tables, %d bytes in __dyndbg section\n",
modct, entries, (int)(modct * sizeof(struct ddebug_table)),
- verbose_bytes + (int)(__stop___dyndbg - __start___dyndbg));
+ (int)(entries * sizeof(struct _ddebug)));
/* apply ddebug_query boot param, dont unload tables on err */
if (ddebug_setup_string[0] != '\0') {
--
2.26.2
make the code-block reusable to later handle "file foo.c:101-200" etc.
This should be a 90%+ code-move, with minimal adaptations; reindent,
and scafolding.
---
lib/dynamic_debug.c | 61 +++++++++++++++++++++++++--------------------
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 20b712652ee4..f87a7bef4204 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -292,6 +292,39 @@ static inline int parse_lineno(const char *str, unsigned int *val)
return 0;
}
+static int parse_linerange(struct ddebug_query *query, const char *first)
+{
+ char *last = strchr(first, '-');
+
+ if (query->first_lineno || query->last_lineno) {
+ pr_err("match-spec: line used 2x\n");
+ return -EINVAL;
+ }
+ if (last)
+ *last++ = '\0';
+ if (parse_lineno(first, &query->first_lineno) < 0)
+ return -EINVAL;
+ if (last) {
+ /* range <first>-<last> */
+ if (parse_lineno(last, &query->last_lineno) < 0)
+ return -EINVAL;
+
+ /* special case for last lineno not specified */
+ if (query->last_lineno == 0)
+ query->last_lineno = UINT_MAX;
+
+ if (query->last_lineno < query->first_lineno) {
+ pr_err("last-line:%d < 1st-line:%d\n",
+ query->last_lineno,
+ query->first_lineno);
+ return -EINVAL;
+ }
+ } else {
+ query->last_lineno = query->first_lineno;
+ }
+ return 0;
+}
+
static int check_set(const char **dest, char *src, char *name)
{
int rc = 0;
@@ -350,34 +383,8 @@ static int ddebug_parse_query(char *words[], int nwords,
UNESCAPE_SPECIAL);
rc = check_set(&query->format, words[i+1], "format");
} else if (!strcmp(words[i], "line")) {
- char *first = words[i+1];
- char *last = strchr(first, '-');
- if (query->first_lineno || query->last_lineno) {
- pr_err("match-spec: line used 2x\n");
- return -EINVAL;
- }
- if (last)
- *last++ = '\0';
- if (parse_lineno(first, &query->first_lineno) < 0)
+ if (parse_linerange(query, words[i+1]))
return -EINVAL;
- if (last) {
- /* range <first>-<last> */
- if (parse_lineno(last, &query->last_lineno) < 0)
- return -EINVAL;
-
- /* special case for last lineno not specified */
- if (query->last_lineno == 0)
- query->last_lineno = UINT_MAX;
-
- if (query->last_lineno < query->first_lineno) {
- pr_err("last-line:%d < 1st-line:%d\n",
- query->last_lineno,
- query->first_lineno);
- return -EINVAL;
- }
- } else {
- query->last_lineno = query->first_lineno;
- }
} else {
pr_err("unknown keyword \"%s\"\n", words[i]);
return -EINVAL;
--
2.26.2
reduce word count via macro, no actual object change.
OTOH, maybe() could be scrubbed if printk's default printing (iirc) of
"(null)" pointers is deemed appropriate for the log-msg.
---
lib/dynamic_debug.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 31d3be30776e..20b712652ee4 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -114,6 +114,7 @@ do { \
#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
#define v2pr_info(fmt, ...) vnpr_info(2, fmt, ##__VA_ARGS__)
+#define maybe(str, empty) ( str ? str : empty )
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
/* trim any trailing newlines */
@@ -127,10 +128,10 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
vpr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u\n",
msg,
- query->function ? query->function : "",
- query->filename ? query->filename : "",
- query->module ? query->module : "",
- fmtlen, query->format ? query->format : "",
+ maybe(query->function, ""),
+ maybe(query->filename, ""),
+ maybe(query->module, ""),
+ fmtlen, maybe(query->format, ""),
query->first_lineno, query->last_lineno);
}
--
2.26.2
combine flags & mask into a struct, and replace those 2 parameters in
3 functions: ddebug_change, ddebug_parse_flags, ddebug_read_flags,
altering the derefs in them accordingly.
This simplifies the 3 function sigs, preparing for more changes.
We dont yet need mask from ddebug_read_flags, but will soon.
---
lib/dynamic_debug.c | 46 +++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 93c627e9c094..8dc073a6e8a4 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -62,6 +62,11 @@ struct ddebug_iter {
unsigned int idx;
};
+struct flagsettings {
+ unsigned int flags;
+ unsigned int mask;
+};
+
static DEFINE_MUTEX(ddebug_lock);
static LIST_HEAD(ddebug_tables);
static int verbose;
@@ -142,7 +147,7 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
* logs the changes. Takes ddebug_lock.
*/
static int ddebug_change(const struct ddebug_query *query,
- unsigned int pflags, unsigned int mask)
+ struct flagsettings *mods)
{
int i;
struct ddebug_table *dt;
@@ -191,14 +196,14 @@ static int ddebug_change(const struct ddebug_query *query,
nfound++;
- newflags = (dp->flags & mask) | pflags;
+ newflags = (dp->flags & mods->mask) | mods->flags;
if (newflags == dp->flags)
continue;
#ifdef CONFIG_JUMP_LABEL
if (dp->flags & _DPRINTK_FLAGS_PRINT) {
- if (!(pflags & _DPRINTK_FLAGS_PRINT))
+ if (!(mods->flags & _DPRINTK_FLAGS_PRINT))
static_branch_disable(&dp->key.dd_key_true);
- } else if (pflags & _DPRINTK_FLAGS_PRINT)
+ } else if (mods->flags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(&dp->key.dd_key_true);
#endif
dp->flags = newflags;
@@ -414,14 +419,14 @@ static int ddebug_parse_query(char *words[], int nwords,
return 0;
}
-static int ddebug_read_flags(const char *str, unsigned int *flags)
+static int ddebug_read_flags(const char *str, struct flagsettings *f)
{
int i;
for (; *str ; ++str) {
for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
if (*str == opt_array[i].opt_char) {
- *flags |= opt_array[i].flag;
+ f->flags |= opt_array[i].flag;
break;
}
}
@@ -430,7 +435,7 @@ static int ddebug_read_flags(const char *str, unsigned int *flags)
return -EINVAL;
}
}
- vpr_info("flags=0x%x\n", *flags);
+ vpr_info("flags=0x%x mask=0x%x\n", f->flags, f->mask);
return 0;
}
@@ -440,10 +445,8 @@ static int ddebug_read_flags(const char *str, unsigned int *flags)
* flags fields of matched _ddebug's. Returns 0 on success
* or <0 on error.
*/
-static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
- unsigned int *maskp)
+static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
{
- unsigned flags = 0;
int op;
switch (*str) {
@@ -458,31 +461,30 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
}
vpr_info("op='%c'\n", op);
- if (ddebug_read_flags(str, &flags))
+ if (ddebug_read_flags(str, mods))
return -EINVAL;
- /* calculate final *flagsp, *maskp according to mask and op */
+ /* calculate final flags, mask based upon op */
switch (op) {
case '=':
- *maskp = 0;
- *flagsp = flags;
+ mods->mask = 0;
break;
case '+':
- *maskp = ~0U;
- *flagsp = flags;
+ mods->mask = ~0U;
break;
case '-':
- *maskp = ~flags;
- *flagsp = 0;
+ mods->mask = ~mods->flags;
+ mods->flags = 0;
break;
}
- vpr_info("*flagsp=0x%x *maskp=0x%x\n", *flagsp, *maskp);
+ vpr_info("*flagsp=0x%x *maskp=0x%x\n", mods->flags, mods->mask);
+
return 0;
}
static int ddebug_exec_query(char *query_string, const char *modname)
{
- unsigned int flags = 0, mask = 0;
+ struct flagsettings mods = {};
struct ddebug_query query;
#define MAXWORDS 9
int nwords, nfound;
@@ -494,7 +496,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
return -EINVAL;
}
/* check flags 1st (last arg) so query is pairs of spec,val */
- if (ddebug_parse_flags(words[nwords-1], &flags, &mask)) {
+ if (ddebug_parse_flags(words[nwords-1], &mods)) {
pr_err("flags parse failed\n");
return -EINVAL;
}
@@ -503,7 +505,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
return -EINVAL;
}
/* actually go and implement the change */
- nfound = ddebug_change(&query, flags, mask);
+ nfound = ddebug_change(&query, &mods);
vpr_info_dq(&query, nfound ? "applied" : "no-match");
return nfound;
--
2.26.2
Extend flags modifications to allow [PFMLTU] negating flags.
This allows control-queries like:
#> Q () { echo file inode.c $* > control } # to type less
#> Q -P # same as +p
#> Q +U # same as -u
#> Q u-P # same as u+p
This allows flags in a callsite to be simultaneously set and cleared,
while still starting with the current flagstate (with +- ops).
Using filter-flags with negating-flags, you can select exactly the
flagstates you want, both required and prohibited.
Then with negating-flags in modflags, you can set and clear every flag
#> Q umfLT-Pmf # select sites with u,m,f only. enable print, turn off m,f leave u
Its not an important feature, but it does fill out the logic.
and the patch is tiny, and feels more symmetrical.
---
Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++----
lib/dynamic_debug.c | 6 ++++--
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 45d3adf889ba..10e514237e83 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -257,9 +257,11 @@ only callsites with ``u`` and ``f`` cleared.
Flagsets cannot contain ``pP`` etc, a flag cannot be true and false.
-modflags containing upper-case flags is reserved/undefined for now.
-inverted-flags are currently ignored, usage gets trickier if given
-``-pXy``, it should leave x set.
+modflags may contain upper-case flags also, using these lets you
+invert the flag setting implied by the OP; '-pU' means disable
+printing, and mark that callsite with the user-flag to create a group,
+for optional further manipulation. Generally, '+p' and '-p' is your
+main choice, and use of negating flags in modflags is rare.
Notes::
@@ -269,7 +271,7 @@ For ``print_hex_dump_debug()`` and ``print_hex_dump_bytes()``, only
For display, the flags are preceded by ``=``
(mnemonic: what the flags are currently equal to).
-Note the regexp ``^[-+=][flmptu_]+$`` matches a flags specification.
+Note the regexp ``/^[-+=][flmptu_]+$/i`` matches a flags specification.
To clear all flags at once, use ``=_`` or ``-flmptu``.
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b02cde1cfc2f..d67fbbc5317f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -486,15 +486,17 @@ static int ddebug_parse_flags(const char *str,
/* calculate final mods: flags, mask based upon op */
switch (op) {
+ unsigned int tmp;
case '=':
mods->mask = 0;
break;
case '+':
- mods->mask = ~0U;
+ mods->mask = ~mods->mask;
break;
case '-':
+ tmp = mods->mask;
mods->mask = ~mods->flags;
- mods->flags = 0;
+ mods->flags = tmp;
break;
}
--
2.26.2
1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
effect on callsite behavior; it allows incremental marking of
arbitrary sets of callsites.
2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
And in ddebug_read_flags():
current code does: [pfmltu_] -> flags
copy it to: [PFMLTU_] -> mask
also disallow both of a pair: ie no 'pP', no true & false.
3. Add filtering ops into ddebug_change(), right after all the
callsite-property selections are complete. These filter on the
callsite's current flagstate before applying modflags.
Why ?
The 'u' flag lets the user assemble an arbitary set of callsites.
Then using filter flags, user can activate the 'u' set.
#> echo 'file foo.c +u; file bar.c +u' > control # and repeat
#> echo 'u+p' > control
Using negating-flags in your filter-flags, you can completely specify
the matching flagstate; not just required flags, but also prohibited
flags.
The user flag isn't strictly needed, but with it you can avoid using
[fmlt] flags for marking, which would alter logging when enabled.
---
.../admin-guide/dynamic-debug-howto.rst | 31 ++++++++++++++++---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 31 ++++++++++++++-----
3 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 4f343e6036f5..45d3adf889ba 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -238,16 +238,39 @@ The flags are::
l Include line number in the printed message
m Include module name in the printed message
t Include thread ID in messages not generated from interrupt context
+ u user flag, to mark callsites into a group
_ No flags are set. (Or'd with others on input)
-For ``print_hex_dump_debug()`` and ``print_hex_dump_bytes()``, only ``p`` flag
-have meaning, other flags ignored.
+Additionally, the flag-chars ``[pflmtu]`` have negating flag-chars
+``[PFMLTU]``, which invert the meanings above. Their use follows.
+
+Using Filters::
+
+Filter-flags specify an optional additional selector on pr_debug
+callsites; with them you can compose an arbitrary set of callsites, by
+iteratively marking them with ``+u``, then enabling them all with
+``u+p``. You can also use ``fmlt`` flags for this, unless the format
+changes are inconvenient.
+
+Filters can also contain the negating flags, like ``UF``, which select
+only callsites with ``u`` and ``f`` cleared.
+
+Flagsets cannot contain ``pP`` etc, a flag cannot be true and false.
+
+modflags containing upper-case flags is reserved/undefined for now.
+inverted-flags are currently ignored, usage gets trickier if given
+``-pXy``, it should leave x set.
+
+Notes::
+
+For ``print_hex_dump_debug()`` and ``print_hex_dump_bytes()``, only
+``p`` flag has meaning, other flags are ignored.
For display, the flags are preceded by ``=``
(mnemonic: what the flags are currently equal to).
-Note the regexp ``^[-+=][flmpt_]+$`` matches a flags specification.
-To clear all flags at once, use ``=_`` or ``-flmpt``.
+Note the regexp ``^[-+=][flmptu_]+$`` matches a flags specification.
+To clear all flags at once, use ``=_`` or ``-flmptu``.
Debug messages during Boot Process
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa9ff9e1c0b3..59960a8dd9f9 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -32,6 +32,7 @@ struct _ddebug {
#define _DPRINTK_FLAGS_INCL_FUNCNAME (1<<2)
#define _DPRINTK_FLAGS_INCL_LINENO (1<<3)
#define _DPRINTK_FLAGS_INCL_TID (1<<4)
+#define _DPRINTK_FLAGS_USR (1<<5)
#if defined DEBUG
#define _DPRINTK_FLAGS_DEFAULT _DPRINTK_FLAGS_PRINT
#else
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b3c262c009d2..b02cde1cfc2f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -83,13 +83,14 @@ static inline const char *trim_prefix(const char *path)
return path + skip;
}
-static struct { unsigned flag:8; char opt_char; } opt_array[] = {
- { _DPRINTK_FLAGS_PRINT, 'p' },
- { _DPRINTK_FLAGS_INCL_MODNAME, 'm' },
- { _DPRINTK_FLAGS_INCL_FUNCNAME, 'f' },
- { _DPRINTK_FLAGS_INCL_LINENO, 'l' },
- { _DPRINTK_FLAGS_INCL_TID, 't' },
- { _DPRINTK_FLAGS_NONE, '_' },
+static struct { unsigned flag:8; char opt_char, not_char; } opt_array[] = {
+ { _DPRINTK_FLAGS_PRINT, 'p', 'P' },
+ { _DPRINTK_FLAGS_INCL_MODNAME, 'm', 'M' },
+ { _DPRINTK_FLAGS_INCL_FUNCNAME, 'f', 'F' },
+ { _DPRINTK_FLAGS_INCL_LINENO, 'l', 'L' },
+ { _DPRINTK_FLAGS_INCL_TID, 't', 'T' },
+ { _DPRINTK_FLAGS_NONE, '_', '_' },
+ { _DPRINTK_FLAGS_USR, 'u', 'U' },
};
struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
@@ -195,6 +196,13 @@ static int ddebug_change(const struct ddebug_query *query,
dp->lineno > query->last_lineno)
continue;
+ /* filter for required flags */
+ if ((dp->flags & filter->flags) != filter->flags)
+ continue;
+ /* filter on prohibited bits */
+ if ((~dp->flags & filter->mask) != filter->mask)
+ continue;
+
nfound++;
newflags = (dp->flags & mods->mask) | mods->flags;
@@ -428,10 +436,17 @@ static int ddebug_read_flags(const char *str, struct flagsettings *f)
if (*str == opt_array[i].opt_char) {
f->flags |= opt_array[i].flag;
break;
+ } else if (*str == opt_array[i].not_char) {
+ f->mask |= opt_array[i].flag;
+ break;
}
}
if (i < 0) {
- pr_err("unknown flag '%c' in \"%s\"\n", *str, str);
+ pr_err("unknown flag '%c'", *str);
+ return -EINVAL;
+ }
+ if (f->flags & f->mask) {
+ pr_err("flag '%c' conflicts with previous\n", *str);
return -EINVAL;
}
}
--
2.26.2
derive DEFINE_DYNAMIC_DEBUG_METADATA_CL
from DEFINE_DYNAMIC_DEBUG_METADATA
then redefine latter as former(0, ...)
Also rework /(_?_?dynamic_.+)/ macros, adding _cl suffix & cl arg1
and redefine them in terms of _cl version.
in printk.h: add pr_debug_n, fix pr_debug use it with pr_cls=0
---
include/linux/dynamic_debug.h | 69 +++++++++++++++++++++++------------
include/linux/printk.h | 5 ++-
2 files changed, 50 insertions(+), 24 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 7ac822d6be87..d7f3dd6fc78a 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -80,7 +80,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
const struct ib_device *ibdev,
const char *fmt, ...);
-#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+#define DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, name, fmt) \
static struct _ddebug __aligned(8) \
__section(__dyndbg) name = { \
.modname = KBUILD_MODNAME, \
@@ -89,8 +89,11 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
.format = (fmt), \
.lineno = __LINE__, \
.flags = _DPRINTK_FLAGS_DEFAULT, \
+ .pr_class = cl, \
_DPRINTK_KEY_INIT \
}
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+ DEFINE_DYNAMIC_DEBUG_METADATA_CL(0, name, fmt)
#ifdef CONFIG_JUMP_LABEL
@@ -121,17 +124,21 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
#endif
-#define __dynamic_func_call(id, fmt, func, ...) do { \
- DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt); \
- if (DYNAMIC_DEBUG_BRANCH(id)) \
- func(&id, ##__VA_ARGS__); \
+#define __dynamic_func_call_cl(cl, id, fmt, func, ...) do { \
+ DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, id, fmt); \
+ if (DYNAMIC_DEBUG_BRANCH(id)) \
+ func(&id, ##__VA_ARGS__); \
} while (0)
+#define __dynamic_func_call(id, fmt, func, ...) \
+ __dynamic_func_call_cl(0, id, fmt, func, ...)
-#define __dynamic_func_call_no_desc(id, fmt, func, ...) do { \
- DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt); \
- if (DYNAMIC_DEBUG_BRANCH(id)) \
- func(__VA_ARGS__); \
+#define __dynamic_func_call_no_desc_cl(cl, id, fmt, func, ...) do { \
+ DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, id, fmt); \
+ if (DYNAMIC_DEBUG_BRANCH(id)) \
+ func(__VA_ARGS__); \
} while (0)
+#define __dynamic_func_call_no_desc(cl, id, fmt, func, ...) \
+ __dynamic_func_call_no_desc_cl(0, cl, id, fmt, func, ##__VA_ARGS__)
/*
* "Factory macro" for generating a call to func, guarded by a
@@ -141,31 +148,44 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
* the varargs. Note that fmt is repeated in invocations of this
* macro.
*/
-#define _dynamic_func_call(fmt, func, ...) \
- __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
+#define _dynamic_func_call_cl(cl, fmt, func, ...) \
+ __dynamic_func_call_cl(cl, __UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
+#define _dynamic_func_call(fmt, func, ...) \
+ _dynamic_func_call_cl(0, fmt, func, ##__VA_ARGS__)
/*
* A variant that does the same, except that the descriptor is not
* passed as the first argument to the function; it is only called
* with precisely the macro's varargs.
*/
-#define _dynamic_func_call_no_desc(fmt, func, ...) \
- __dynamic_func_call_no_desc(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
+#define _dynamic_func_call_no_desc_cl(cl, fmt, func, ...) \
+ __dynamic_func_call_no_desc_cl(cl, __UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
+#define _dynamic_func_call_no_desc(fmt, func, ...) \
+ _dynamic_func_call_no_desc_cl(0, fmt, func, ##__VA_ARGS__)
-#define dynamic_pr_debug(fmt, ...) \
- _dynamic_func_call(fmt, __dynamic_pr_debug, \
- pr_fmt(fmt), ##__VA_ARGS__)
+#define dynamic_pr_debug_cl(cl, fmt, ...) \
+ _dynamic_func_call_cl(cl, fmt, __dynamic_pr_debug, \
+ pr_fmt(fmt), ##__VA_ARGS__)
-#define dynamic_dev_dbg(dev, fmt, ...) \
- _dynamic_func_call(fmt,__dynamic_dev_dbg, \
+#define dynamic_dev_dbg_cl(cl, dev, fmt, ...) \
+ _dynamic_func_call_cl(cl, fmt, __dynamic_dev_dbg, \
dev, fmt, ##__VA_ARGS__)
-#define dynamic_netdev_dbg(dev, fmt, ...) \
- _dynamic_func_call(fmt, __dynamic_netdev_dbg, \
+#define dynamic_netdev_dbg_cl(cl, dev, fmt, ...) \
+ _dynamic_func_call_cl(cl, fmt, __dynamic_netdev_dbg, \
dev, fmt, ##__VA_ARGS__)
-#define dynamic_ibdev_dbg(dev, fmt, ...) \
- _dynamic_func_call(fmt, __dynamic_ibdev_dbg, \
- dev, fmt, ##__VA_ARGS__)
+#define dynamic_ibdev_dbg_cl(cl, dev, fmt, ...) \
+ _dynamic_func_call_cl(cl, fmt, __dynamic_ibdev_dbg, \
+ dev, fmt, ##__VA_ARGS__)
+
+#define dynamic_pr_debug(...) \
+ dynamic_pr_debug_cl(0, ##__VA_ARGS__)
+#define dynamic_dev_dbg(...) \
+ dynamic_dev_dbg_cl(0, ##__VA_ARGS__)
+#define dynamic_netdev_dbg(...) \
+ dynamic_netdev_dbg_cl(0, ##__VA_ARGS__)
+#define dynamic_ibdev_dbg(...) \
+ dynamic_ibdev_dbg_cl(0, ##__VA_ARGS__)
#define dynamic_hex_dump(prefix_str, prefix_type, rowsize, \
groupsize, buf, len, ascii) \
@@ -202,6 +222,9 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
return -EINVAL;
}
+#define dynamic_pr_debug_cl(cl, ...) dynamic_pr_debug(__VA_ARGS__)
+#define dynamic_dev_dbg_cl(cl, ...) dynamic_dev_dbg(__VA_ARGS__)
+
#define dynamic_pr_debug(fmt, ...) \
do { if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); } while (0)
#define dynamic_dev_dbg(dev, fmt, ...) \
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fc8f03c54543..693d7c9235b7 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -416,7 +416,10 @@ extern int kptr_restrict;
* pr_fmt() internally).
*/
#define pr_debug(fmt, ...) \
- dynamic_pr_debug(fmt, ##__VA_ARGS__)
+ dynamic_pr_debug_cl(0, fmt, ##__VA_ARGS__)
+#define pr_debug_n(num, fmt, ...) \
+ dynamic_pr_debug_cl(num, fmt, ##__VA_ARGS__)
+
#elif defined(DEBUG)
#define pr_debug(fmt, ...) \
printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
--
2.26.2
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 only.
proposed API:
Usage model is for a module developer to create N exclusive subsets of
pr_debug()s by changing some of them to pr_debug_n(1,) .. pr_debug_n(N,).
Each callsite must be a single print-class, with 0 default.
No multi-type classification ala pr_debug_M(1|2, ...) is contemplated.
- change pr_debug(...) --> pr_debug_n(pr_class=0, ...)
- all existing uses have pr_class=0
- developer creates exclusive types of log messages with pr_class>0
1, 2, 3 are disjoint groups, for example: hi, mid, low
0 is reserved for existing uses.
- adds query term: "mflags $arg"
rename keyword to prcls ?
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
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 unsigned int pr_classes into struct ddebug_query
this is a bit-vector
bit positions select which print-classes to filter callsites for
- add unsigned int pr_class:5 to struct _ddebug
picks a single debugflag bit. No subclass or multitype nonsense.
nice and dense, packs with other members.
if adoption is good, kernel will have a lot of struct _ddebugs.
- in ddebug_change()
IFF query->module is given, and matches dt->mod_name
print-classes are defined on a module, so we require one.
this is fooled by "module *"
simple fix is to exclude any wildcard when mflags given
- 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_n( pr_class=0, ....)
aka: pr_debug_class() or pr_debug_id()
bikeshedding welcome.
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_n() to make a (not-null) typed callsite.
yet - done in subsequent patches
- mflags arg-parse is primitive, placeholder
- module.debug vars
I think this can be sanely handled with a callback to handle updates.
Perhaps several to handle different debug/verbose flavors
maybe we export ddebug_exec_queries.
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 pr_class 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 ?
3- pr_class:5 gives 32 print-classes
we can map [1-9a-w] to select any pr_class with 1 char
then "12", "af" work as noted.
it is succinct, but arcane.
but it does allow mnemonic choices of pr_class
- l,m,h low, mid, hi
- l,r left right
it also allows us to treat 1-9 as levels
- by auto-setting 1-7 when 7 is enabled.
ie: "mflags 7 +pu" in effect does "mflags 1234567 +pu"
note that even if this is done,
individual callsites or sets of them can be undone.
you can even use 'u' as above to mark them for easier grepping
---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 33 +++++++++++++++++++++++++++++++--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 59960a8dd9f9..7ac822d6be87 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -20,6 +20,7 @@ struct _ddebug {
const char *function;
const char *filename;
const char *format;
+ unsigned int pr_class:5; /* >0 for distinct developer groups */
unsigned int lineno:18;
/*
* The flags field controls the behaviour at the callsite.
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index cb5c7480e026..0035218d7059 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -55,6 +55,7 @@ struct ddebug_query {
const char *function;
const char *format;
unsigned int first_lineno, last_lineno;
+ unsigned int pr_classes;
};
struct ddebug_iter {
@@ -132,13 +133,14 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
fmtlen--;
}
- vpr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u\n",
+ vpr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u prcls=0x%x\n",
msg,
maybe(query->function, ""),
maybe(query->filename, ""),
maybe(query->module, ""),
fmtlen, maybe(query->format, ""),
- query->first_lineno, query->last_lineno);
+ query->first_lineno, query->last_lineno,
+ query->pr_classes);
}
/*
@@ -203,6 +205,27 @@ static int ddebug_change(const struct ddebug_query *query,
if ((~dp->flags & filter->mask) != filter->mask)
continue;
+ /* filter on non-zero print-classes */
+ if (query->pr_classes) {
+ if (!query->module) {
+ pr_err("using prcls requires module too");
+ return -EINVAL;
+ }
+ /* since print-classes are module
+ * specific, require a module query
+ * too. For 'module kvm* mflags 1'
+ * >control, this will enable
+ * pr_class=1 in several matching modules
+ */
+ if ((query->pr_classes & (1<<(dp->pr_class-1)))
+ != (1<<(dp->pr_class-1))) {
+ v2pr_info("%s ~ %s mflags:0x%x !~ %d\n",
+ dt->mod_name, query->module,
+ query->pr_classes, dp->pr_class);
+ continue;
+ }
+ }
+
nfound++;
newflags = (dp->flags & mods->mask) | mods->flags;
@@ -427,6 +450,12 @@ static int ddebug_parse_query(char *words[], int nwords,
} else if (!strcmp(keyword, "line")) {
if (parse_linerange(query, arg))
return -EINVAL;
+ } else if (!strcmp(keyword, "mflags")) {
+ pr_info("handle mflags arg: %s\n", arg);
+ if (kstrtouint(arg, 4, &query->pr_classes) < 0) {
+ pr_err("bad arg for mflags: %s\n", arg);
+ return -EINVAL;
+ }
} else {
pr_err("unknown keyword \"%s\"\n", keyword);
return -EINVAL;
--
2.26.2
Here is a (counter?) use-case for debug-print-classes. It adds
read,write print-classes, and marks all pr_debugs in *_show and
*_store functions accordingly.
this allows manipulation of those categories as:
echo "module kset-example mflags 1 +p" > /proc/dynamic_debug/control
echo "module kset-example mflags 2 +p" > /proc/dynamic_debug/control
But this marking completely duplicates the callsite sets definable by:
#> cat enable-show-store
module kset-example function *_show +p
module kset-example function *_store +p
#> cat enable-show-store > /proc/dynamic_debug/control
IOW, dont over-use debug-print-class. Your arbitrary subset of
pr-debug calls can be recreated by an N line script.
---
samples/kobject/kset-example.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
index 27c9b1beec28..61d016cb6b4d 100644
--- a/samples/kobject/kset-example.c
+++ b/samples/kobject/kset-example.c
@@ -42,6 +42,27 @@ struct foo_attribute {
};
#define to_foo_attr(x) container_of(x, struct foo_attribute, attr)
+/*
+ * This module also uses pr_debug() to provide debug logging that
+ * makes it easy to see the module operating. Just invoke as:
+ * #> dmesg -w &
+ * #> modprobe kset-example dyndbg=+pfml
+ *
+ * Further, we *arbitrarily* use pr_debug_n() to create 2 separate
+ * (non-default) print-classes, in the *_show and *_store functions.
+ *
+ * This allows selection by those categories:
+ * echo "module kset-example mflags 1 +p" > dynamic_debug/control
+ * echo "module kset-example mflags 2 +p" > dynamic_debug/control
+ * but that is clearer as:
+ * echo "module kset-example function *_show +p" > dynamic_debug/control
+ * echo "module kset-example function *_store +p" > dynamic_debug/control
+ *
+ * IOW - mostly you dont need non-default print-classes
+ */
+#define prCATr 1 /* print-class for show (r=read) */
+#define prCATw 2 /* print-class for store (w=write) */
+
/*
* The default show function that must be passed to sysfs. This will be
* called by sysfs for whenever a show function is called by the user on a
@@ -56,7 +77,7 @@ static ssize_t foo_attr_show(struct kobject *kobj,
struct foo_attribute *attribute;
struct foo_obj *foo;
- pr_debug("called");
+ pr_debug_n(prCATr, "called");
attribute = to_foo_attr(attr);
foo = to_foo_obj(kobj);
@@ -77,7 +98,7 @@ static ssize_t foo_attr_store(struct kobject *kobj,
struct foo_attribute *attribute;
struct foo_obj *foo;
- pr_debug("called");
+ pr_debug_n(prCATw, "called");
attribute = to_foo_attr(attr);
foo = to_foo_obj(kobj);
@@ -115,7 +136,7 @@ static void foo_release(struct kobject *kobj)
static ssize_t foo_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
char *buf)
{
- pr_debug("called");
+ pr_debug_n(prCATr, "called");
return sprintf(buf, "%d\n", foo_obj->foo);
}
@@ -124,7 +145,7 @@ static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
{
int ret;
- pr_debug("called");
+ pr_debug_n(prCATw, "called");
ret = kstrtoint(buf, 10, &foo_obj->foo);
if (ret < 0)
return ret;
@@ -145,7 +166,7 @@ static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
{
int var;
- pr_debug("called");
+ pr_debug_n(prCATr, "called");
if (strcmp(attr->attr.name, "baz") == 0)
var = foo_obj->baz;
else
@@ -158,7 +179,7 @@ static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
{
int var, ret;
- pr_debug("called");
+ pr_debug_n(prCATw, "called");
ret = kstrtoint(buf, 10, &var);
if (ret < 0)
return ret;
--
2.26.2
put pr_debug()s into most functions, to easily see code operate when
module is loaded and used.
#> dmesg -w &
#> modprobe kset-example dyndbg=+pfml
#> cat /sys/kernel/kset-example/*/*
---
samples/kobject/kset-example.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
index c8010f126808..27c9b1beec28 100644
--- a/samples/kobject/kset-example.c
+++ b/samples/kobject/kset-example.c
@@ -56,6 +56,7 @@ static ssize_t foo_attr_show(struct kobject *kobj,
struct foo_attribute *attribute;
struct foo_obj *foo;
+ pr_debug("called");
attribute = to_foo_attr(attr);
foo = to_foo_obj(kobj);
@@ -76,6 +77,7 @@ static ssize_t foo_attr_store(struct kobject *kobj,
struct foo_attribute *attribute;
struct foo_obj *foo;
+ pr_debug("called");
attribute = to_foo_attr(attr);
foo = to_foo_obj(kobj);
@@ -102,6 +104,7 @@ static void foo_release(struct kobject *kobj)
{
struct foo_obj *foo;
+ pr_debug("called");
foo = to_foo_obj(kobj);
kfree(foo);
}
@@ -112,6 +115,7 @@ static void foo_release(struct kobject *kobj)
static ssize_t foo_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
char *buf)
{
+ pr_debug("called");
return sprintf(buf, "%d\n", foo_obj->foo);
}
@@ -120,6 +124,7 @@ static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
{
int ret;
+ pr_debug("called");
ret = kstrtoint(buf, 10, &foo_obj->foo);
if (ret < 0)
return ret;
@@ -140,6 +145,7 @@ static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
{
int var;
+ pr_debug("called");
if (strcmp(attr->attr.name, "baz") == 0)
var = foo_obj->baz;
else
@@ -152,6 +158,7 @@ static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
{
int var, ret;
+ pr_debug("called");
ret = kstrtoint(buf, 10, &var);
if (ret < 0)
return ret;
@@ -201,6 +208,7 @@ static struct foo_obj *create_foo_obj(const char *name)
struct foo_obj *foo;
int retval;
+ pr_debug("called");
/* allocate the memory for the whole object */
foo = kzalloc(sizeof(*foo), GFP_KERNEL);
if (!foo)
@@ -235,11 +243,13 @@ static struct foo_obj *create_foo_obj(const char *name)
static void destroy_foo_obj(struct foo_obj *foo)
{
+ pr_debug("called");
kobject_put(&foo->kobj);
}
static int __init example_init(void)
{
+ pr_debug("called");
/*
* Create a kset with the name of "kset_example",
* located under /sys/kernel/
@@ -276,6 +286,7 @@ static int __init example_init(void)
static void __exit example_exit(void)
{
+ pr_debug("called");
destroy_foo_obj(baz_obj);
destroy_foo_obj(bar_obj);
destroy_foo_obj(foo_obj);
--
2.26.2
Current code expects "keyword" "arg" as 2 space separated words.
Change to also accept "keyword:arg" and "keyword=arg" forms as well,
and drop !(nwords%2) requirement.
Then in rest of function, use new keyword,arg variables instead of
word[i],word[i+1]
---
lib/dynamic_debug.c | 49 +++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index d67fbbc5317f..cb5c7480e026 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -375,22 +375,33 @@ static int ddebug_parse_query(char *words[], int nwords,
unsigned int i;
int rc = 0;
char *fline;
-
- /* check we have an even number of words */
- if (nwords % 2 != 0) {
- pr_err("expecting pairs of match-spec <value>\n");
- return -EINVAL;
- }
+ char *keyword, *arg;
if (modname)
/* support $modname.dyndbg=<multiple queries> */
query->module = modname;
- for (i = 0; i < nwords; i += 2) {
- if (!strcmp(words[i], "func")) {
- rc = check_set(&query->function, words[i+1], "func");
- } else if (!strcmp(words[i], "file")) {
- if (check_set(&query->filename, words[i+1], "file"))
+ for (i = 0; i < nwords; i++) {
+ /* accept keyword[:=]arg */
+ vpr_info("%d w:%s\n", i, words[i]);
+
+ keyword = words[i];
+ if ((arg = strpbrk(keyword, ":="))) {
+ *arg++ = '\0';
+ } else {
+ i++; /* next word is arg */
+ if (!(i < nwords)) {
+ pr_err("missing arg to keyword:%s\n", keyword);
+ return -EINVAL;
+ }
+ arg = words[i];
+ }
+ vpr_info("%d key:%s arg:%s\n", i, keyword, arg);
+
+ if (!strcmp(keyword, "func")) {
+ rc = check_set(&query->function, arg, "func");
+ } else if (!strcmp(keyword, "file")) {
+ if (check_set(&query->filename, arg, "file"))
return -EINVAL;
/* tail :$info is function or line-range */
@@ -406,18 +417,18 @@ static int ddebug_parse_query(char *words[], int nwords,
if (parse_linerange(query, fline))
return -EINVAL;
}
- } else if (!strcmp(words[i], "module")) {
- rc = check_set(&query->module, words[i+1], "module");
- } else if (!strcmp(words[i], "format")) {
- string_unescape_inplace(words[i+1], UNESCAPE_SPACE |
+ } else if (!strcmp(keyword, "module")) {
+ rc = check_set(&query->module, arg, "module");
+ } else if (!strcmp(keyword, "format")) {
+ string_unescape_inplace(arg, UNESCAPE_SPACE |
UNESCAPE_OCTAL |
UNESCAPE_SPECIAL);
- rc = check_set(&query->format, words[i+1], "format");
- } else if (!strcmp(words[i], "line")) {
- if (parse_linerange(query, words[i+1]))
+ rc = check_set(&query->format, arg, "format");
+ } else if (!strcmp(keyword, "line")) {
+ if (parse_linerange(query, arg))
return -EINVAL;
} else {
- pr_err("unknown keyword \"%s\"\n", words[i]);
+ pr_err("unknown keyword \"%s\"\n", keyword);
return -EINVAL;
}
if (rc)
--
2.26.2
This change adds trailing 0 for all existing callsites, and N>0 for
any uses of pr_debug_n().
subsequent patches add pr_debug_n(1) & pr_debug_n(2) to kset-example,
demonstrating that the callsite descriptors are getting initialized
properly by the *_cl adaptations done previously.
bash-5.0# modprobe kset-example
bash-5.0# grep -v '0$' control
[ 311.060809] dynamic_debug:ddebug_proc_open: called
samples/kobject/kset-example.c:80 [kset_example]foo_attr_store =_ "called" 2
samples/kobject/kset-example.c:59 [kset_example]foo_attr_show =_ "called" 1
---
lib/dynamic_debug.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0035218d7059..262a6a761219 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -919,7 +919,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
if (p == SEQ_START_TOKEN) {
seq_puts(m,
- "# filename:lineno [module]function flags format\n");
+ "# filename:lineno [module]function flags format prcls\n");
return 0;
}
@@ -928,7 +928,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
iter->table->mod_name, dp->function,
ddebug_describe_flags(dp->flags, &flags));
seq_escape(m, dp->format, "\t\r\n\"");
- seq_puts(m, "\"\n");
+ seq_printf(m, "\" %d\n", dp->pr_class);
return 0;
}
--
2.26.2
ddebug_describe_flags currently fills a caller provided string buffer,
after testing its size (also passed) in a BUG_ON. Fix this by
replacing them with a known-big-enough string buffer wrapped in a
struct, and passing that instead.
Also simplify the flags parameter, and instead de-ref the flags struct
in the caller; this makes the function reusable (soon) where flags are
unpacked.
---
lib/dynamic_debug.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9b2445507988..aaace13d7536 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -87,22 +87,22 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
{ _DPRINTK_FLAGS_NONE, '_' },
};
+struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
+
/* format a string into buf[] which describes the _ddebug's flags */
-static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
- size_t maxlen)
+static char *ddebug_describe_flags(unsigned int flags, struct flagsbuf *fb)
{
- char *p = buf;
+ char *p = fb->buf;
int i;
- BUG_ON(maxlen < 6);
for (i = 0; i < ARRAY_SIZE(opt_array); ++i)
- if (dp->flags & opt_array[i].flag)
+ if (flags & opt_array[i].flag)
*p++ = opt_array[i].opt_char;
- if (p == buf)
+ if (p == fb->buf)
*p++ = '_';
*p = '\0';
- return buf;
+ return fb->buf;
}
#define vnpr_info(lvl, fmt, ...) \
@@ -141,13 +141,13 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
* logs the changes. Takes ddebug_lock.
*/
static int ddebug_change(const struct ddebug_query *query,
- unsigned int flags, unsigned int mask)
+ unsigned int pflags, unsigned int mask)
{
int i;
struct ddebug_table *dt;
unsigned int newflags;
unsigned int nfound = 0;
- char flagbuf[10];
+ struct flagsbuf flags;
/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
@@ -190,22 +190,21 @@ static int ddebug_change(const struct ddebug_query *query,
nfound++;
- newflags = (dp->flags & mask) | flags;
+ newflags = (dp->flags & mask) | pflags;
if (newflags == dp->flags)
continue;
#ifdef CONFIG_JUMP_LABEL
if (dp->flags & _DPRINTK_FLAGS_PRINT) {
- if (!(flags & _DPRINTK_FLAGS_PRINT))
+ if (!(pflags & _DPRINTK_FLAGS_PRINT))
static_branch_disable(&dp->key.dd_key_true);
- } else if (flags & _DPRINTK_FLAGS_PRINT)
+ } else if (pflags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(&dp->key.dd_key_true);
#endif
dp->flags = newflags;
v2pr_info("changed %s:%d [%s]%s =%s\n",
trim_prefix(dp->filename), dp->lineno,
dt->mod_name, dp->function,
- ddebug_describe_flags(dp, flagbuf,
- sizeof(flagbuf)));
+ ddebug_describe_flags(dp->flags, &flags));
}
}
mutex_unlock(&ddebug_lock);
@@ -814,7 +813,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
{
struct ddebug_iter *iter = m->private;
struct _ddebug *dp = p;
- char flagsbuf[10];
+ struct flagsbuf flags;
if (p == SEQ_START_TOKEN) {
seq_puts(m,
@@ -825,7 +824,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
seq_printf(m, "%s:%u [%s]%s =%s \"",
trim_prefix(dp->filename), dp->lineno,
iter->table->mod_name, dp->function,
- ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
+ ddebug_describe_flags(dp->flags, &flags));
seq_escape(m, dp->format, "\t\r\n\"");
seq_puts(m, "\"\n");
--
2.26.2
drop memset in ddebug_parse_query, instead initialize the stack
variable in the caller; let the compiler decide how to do it.
---
lib/dynamic_debug.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 40436270d38a..b3c262c009d2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -373,7 +373,6 @@ static int ddebug_parse_query(char *words[], int nwords,
pr_err("expecting pairs of match-spec <value>\n");
return -EINVAL;
}
- memset(query, 0, sizeof(*query));
if (modname)
/* support $modname.dyndbg=<multiple queries> */
@@ -494,7 +493,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
{
struct flagsettings mods = {};
struct flagsettings filter = {};
- struct ddebug_query query;
+ struct ddebug_query query = {};
#define MAXWORDS 9
int nwords, nfound;
char *words[MAXWORDS];
--
2.26.2
Add a new local *filter variable to ddebug_exec_query(), pass it into
ddebug_parse_flags(), which fills it, communicating optional filter
flags back to its caller. Then caller passes same to ddebug_change()
to effect the changes.
Also, ddebug_change doesn't alter any of its arguments, including its 2
new ones; mods, filter. Say so by adding const modifier to them.
---
lib/dynamic_debug.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8dc073a6e8a4..0f393a930fdc 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -147,7 +147,8 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
* logs the changes. Takes ddebug_lock.
*/
static int ddebug_change(const struct ddebug_query *query,
- struct flagsettings *mods)
+ const struct flagsettings *mods,
+ const struct flagsettings *filter)
{
int i;
struct ddebug_table *dt;
@@ -445,7 +446,10 @@ static int ddebug_read_flags(const char *str, struct flagsettings *f)
* flags fields of matched _ddebug's. Returns 0 on success
* or <0 on error.
*/
-static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
+
+static int ddebug_parse_flags(const char *str,
+ struct flagsettings *mods,
+ struct flagsettings *filter)
{
int op;
@@ -477,7 +481,9 @@ static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
mods->flags = 0;
break;
}
- vpr_info("*flagsp=0x%x *maskp=0x%x\n", mods->flags, mods->mask);
+
+ vpr_info("mods:flags=0x%x,mask=0x%x filter:flags=0x%x,mask=0x%x\n",
+ mods->flags, mods->mask, filter->flags, filter->mask);
return 0;
}
@@ -485,6 +491,7 @@ static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
static int ddebug_exec_query(char *query_string, const char *modname)
{
struct flagsettings mods = {};
+ struct flagsettings filter = {};
struct ddebug_query query;
#define MAXWORDS 9
int nwords, nfound;
@@ -496,7 +503,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
return -EINVAL;
}
/* check flags 1st (last arg) so query is pairs of spec,val */
- if (ddebug_parse_flags(words[nwords-1], &mods)) {
+ if (ddebug_parse_flags(words[nwords-1], &mods, &filter)) {
pr_err("flags parse failed\n");
return -EINVAL;
}
@@ -505,7 +512,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
return -EINVAL;
}
/* actually go and implement the change */
- nfound = ddebug_change(&query, &mods);
+ nfound = ddebug_change(&query, &mods, &filter);
vpr_info_dq(&query, nfound ? "applied" : "no-match");
return nfound;
--
2.26.2
loadable modules are the last in on this list, and are the only
modules that could be removed. ddebug_remove_module() searches from
head, but ddebug_add_module() uses list_add_tail(). Change it to
list_add() for a micro-optimization.
---
lib/dynamic_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index aaace13d7536..31d3be30776e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -896,7 +896,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
dt->ddebugs = tab;
mutex_lock(&ddebug_lock);
- list_add_tail(&dt->link, &ddebug_tables);
+ list_add(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
v2pr_info("%u debug prints in module %s\n", n, dt->mod_name);
--
2.26.2
Currently, ddebug_parse_flags accepts [+-=][pflmt_]+ as flag-spec
strings. If we allow [pflmt_]*[+-=][pflmt_]+ instead, the (new) 1st
flagset can be used as a filter to select callsites, before applying
changes in the 2nd flagset. 1st step is to split out the flags-reader
so we can use it again.
The point of this is to allow user to compose an arbitrary set of
changes, by marking callsites with [fmlt] flags, and then to
activate that composed set in a single query.
#> echo '=_' > control # clear all flags
#> echo 'module usb* +fmlt' > control # build the marked set, repeat
#> echo 'fmlt+p' > control # activate
---
lib/dynamic_debug.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 784c075c7db9..93c627e9c094 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -414,6 +414,26 @@ static int ddebug_parse_query(char *words[], int nwords,
return 0;
}
+static int ddebug_read_flags(const char *str, unsigned int *flags)
+{
+ int i;
+
+ for (; *str ; ++str) {
+ for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
+ if (*str == opt_array[i].opt_char) {
+ *flags |= opt_array[i].flag;
+ break;
+ }
+ }
+ if (i < 0) {
+ pr_err("unknown flag '%c' in \"%s\"\n", *str, str);
+ return -EINVAL;
+ }
+ }
+ vpr_info("flags=0x%x\n", *flags);
+ return 0;
+}
+
/*
* Parse `str' as a flags specification, format [-+=][p]+.
* Sets up *maskp and *flagsp to be used when changing the
@@ -424,7 +444,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
unsigned int *maskp)
{
unsigned flags = 0;
- int op = '=', i;
+ int op;
switch (*str) {
case '+':
@@ -438,19 +458,8 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
}
vpr_info("op='%c'\n", op);
- for (; *str ; ++str) {
- for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
- if (*str == opt_array[i].opt_char) {
- flags |= opt_array[i].flag;
- break;
- }
- }
- if (i < 0) {
- pr_err("unknown flag '%c' in \"%s\"\n", *str, str);
- return -EINVAL;
- }
- }
- vpr_info("flags=0x%x\n", flags);
+ if (ddebug_read_flags(str, &flags))
+ return -EINVAL;
/* calculate final *flagsp, *maskp according to mask and op */
switch (op) {
--
2.26.2
Regarding:
commit 2b6783191da7 ("dynamic_debug: add trim_prefix() to provide source-root relative paths")
commit a73619a845d5 ("kbuild: use -fmacro-prefix-map to make __FILE__ a relative path")
2nd commit broke dynamic-debug's "file $fullpath" query form, but
nobody noticed because 1st commit had trimmed prefixes from
control-file output, so the click-copy-pasting of fullpaths into new
queries had ceased; that query form became unused.
Removing the function is cleanest, but it could be useful in
old-compiler corner cases, where __FILE__ still has /full/path,
and it safely does nothing otherwize.
So instead, quietly deprecate "file /full/path" query form, by
removing all /full/paths examples in the docs. I skipped adding a
back-compat note.
---
.../admin-guide/dynamic-debug-howto.rst | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 1012bd9305e9..57108f64afc8 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -70,10 +70,10 @@ statements via::
nullarbor:~ # cat <debugfs>/dynamic_debug/control
# filename:lineno [module]function flags format
- /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:323 [svcxprt_rdma]svc_rdma_cleanup =_ "SVCRDMA Module Removed, deregister RPC RDMA transport\012"
- /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:341 [svcxprt_rdma]svc_rdma_init =_ "\011max_inline : %d\012"
- /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:340 [svcxprt_rdma]svc_rdma_init =_ "\011sq_depth : %d\012"
- /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:338 [svcxprt_rdma]svc_rdma_init =_ "\011max_requests : %d\012"
+ net/sunrpc/svc_rdma.c:323 [svcxprt_rdma]svc_rdma_cleanup =_ "SVCRDMA Module Removed, deregister RPC RDMA transport\012"
+ net/sunrpc/svc_rdma.c:341 [svcxprt_rdma]svc_rdma_init =_ "\011max_inline : %d\012"
+ net/sunrpc/svc_rdma.c:340 [svcxprt_rdma]svc_rdma_init =_ "\011sq_depth : %d\012"
+ net/sunrpc/svc_rdma.c:338 [svcxprt_rdma]svc_rdma_init =_ "\011max_requests : %d\012"
...
@@ -93,7 +93,7 @@ the debug statement callsites with any non-default flags::
nullarbor:~ # awk '$3 != "=_"' <debugfs>/dynamic_debug/control
# filename:lineno [module]function flags format
- /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svcsock.c:1603 [sunrpc]svc_send p "svc_process: st_sendto returned %d\012"
+ net/sunrpc/svcsock.c:1603 [sunrpc]svc_send p "svc_process: st_sendto returned %d\012"
Command Language Reference
==========================
@@ -166,13 +166,12 @@ func
func svc_tcp_accept
file
- The given string is compared against either the full pathname, the
- src-root relative pathname, or the basename of the source file of
- each callsite. Examples::
+ The given string is compared against either the src-root relative
+ pathname, or the basename of the source file of each callsite.
+ Examples::
file svcsock.c
- file kernel/freezer.c
- file /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svcsock.c
+ file kernel/freezer.c # ie column 1 of control file
module
The given string is compared against the module name
--
2.26.2
Accept these additional query forms:
echo "file $filestr +_" > control
path/to/file.c:100 # as from control, column 1
path/to/file.c:1-100 # or any legal line-range
path/to/file.c:func_A # as from an editor/browser
path/to/file.c:drm_\* # wildcards still work
path/to/file.c:*_foo # lead wildcard too
1st 2 examples are treated as line-ranges, 3,4 are treated as func's
Doc these changes, and sprinkle in a few extra wild-card examples and
trailing # explanation texts.
---
.../admin-guide/dynamic-debug-howto.rst | 5 +++++
lib/dynamic_debug.c | 20 ++++++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 1423af580bed..6c04aea8f4cd 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -164,6 +164,7 @@ func
of each callsite. Example::
func svc_tcp_accept
+ func *recv* # in rfcomm, bluetooth, ping, tcp
file
The given string is compared against either the src-root relative
@@ -172,6 +173,9 @@ file
file svcsock.c
file kernel/freezer.c # ie column 1 of control file
+ file drivers/usb/* # all callsites under it
+ file inode.c:start_* # parse :tail as a func (above)
+ file inode.c:1-100 # parse :tail as a line-range (above)
module
The given string is compared against the module name
@@ -181,6 +185,7 @@ module
module sunrpc
module nfsd
+ module drm* # both drm, drm_kms_helper
format
The given string is searched for in the dynamic debug format
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index f87a7bef4204..784c075c7db9 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -322,6 +322,8 @@ static int parse_linerange(struct ddebug_query *query, const char *first)
} else {
query->last_lineno = query->first_lineno;
}
+ vpr_info("parsed line %d-%d\n", query->first_lineno,
+ query->last_lineno);
return 0;
}
@@ -358,6 +360,7 @@ static int ddebug_parse_query(char *words[], int nwords,
{
unsigned int i;
int rc = 0;
+ char *fline;
/* check we have an even number of words */
if (nwords % 2 != 0) {
@@ -374,7 +377,22 @@ static int ddebug_parse_query(char *words[], int nwords,
if (!strcmp(words[i], "func")) {
rc = check_set(&query->function, words[i+1], "func");
} else if (!strcmp(words[i], "file")) {
- rc = check_set(&query->filename, words[i+1], "file");
+ if (check_set(&query->filename, words[i+1], "file"))
+ return -EINVAL;
+
+ /* tail :$info is function or line-range */
+ fline = strchr(query->filename, ':');
+ if (!fline)
+ break;
+ *fline++ = '\0';
+ if (isalpha(*fline) || *fline == '*' || *fline == '?') {
+ /* take as function name */
+ if (check_set(&query->function, fline, "func"))
+ return -EINVAL;
+ } else {
+ if (parse_linerange(query, fline))
+ return -EINVAL;
+ }
} else if (!strcmp(words[i], "module")) {
rc = check_set(&query->module, words[i+1], "module");
} else if (!strcmp(words[i], "format")) {
--
2.26.2
change ddebug_parse_flags to accept optional filterflags before OP.
this now sets the parameter added in ~1
---
.../admin-guide/dynamic-debug-howto.rst | 18 +++++++----
lib/dynamic_debug.c | 30 ++++++++++---------
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 6c04aea8f4cd..4f343e6036f5 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -217,13 +217,19 @@ line
line -1605 // the 1605 lines from line 1 to line 1605
line 1600- // all lines from line 1600 to the end of the file
-The flags specification comprises a change operation followed
-by one or more flag characters. The change operation is one
-of the characters::
+Flags Specification::
- - remove the given flags
- + add the given flags
- = set the flags to the given flags
+ flagspec ::= filterflags? OP modflags
+ filterflags ::= flagset
+ modflags ::= flagset
+ flagset ::= ([pfmltu_] | [PFMLTU_])+ # also cant have pP etc
+ OP ::= [-+=]
+
+OP: modify callsites per following flagset::
+
+ - remove the following flags
+ + add the following flags
+ = set the flags to the following flags
The flags are::
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0f393a930fdc..40436270d38a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -441,34 +441,36 @@ static int ddebug_read_flags(const char *str, struct flagsettings *f)
}
/*
- * Parse `str' as a flags specification, format [-+=][p]+.
- * Sets up *maskp and *flagsp to be used when changing the
- * flags fields of matched _ddebug's. Returns 0 on success
- * or <0 on error.
+ * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
+ * Fills flagsettings provided. Returns 0 on success or <0 on error.
*/
-
static int ddebug_parse_flags(const char *str,
struct flagsettings *mods,
struct flagsettings *filter)
{
int op;
+ char *opp = strpbrk(str, "-+=");
- switch (*str) {
- case '+':
- case '-':
- case '=':
- op = *str++;
- break;
- default:
- pr_err("bad flag-op %c, at start of %s\n", *str, str);
+ if (!opp) {
+ pr_err("no OP given in %s\n", str);
return -EINVAL;
}
+ op = *opp;
vpr_info("op='%c'\n", op);
+ if (opp != str) {
+ /* filterflags precedes OP, grab it */
+ *opp++ = '\0';
+ if (ddebug_read_flags(str, filter))
+ return -EINVAL;
+ str = opp;
+ } else
+ str++;
+
if (ddebug_read_flags(str, mods))
return -EINVAL;
- /* calculate final flags, mask based upon op */
+ /* calculate final mods: flags, mask based upon op */
switch (op) {
case '=':
mods->mask = 0;
--
2.26.2
commit 4bad78c55002 ("lib/dynamic_debug.c: use seq_open_private() instead of seq_open()")'
The commit was one of a tree-wide set which replaced open-coded
boilerplate with a single tail-call. It therefore obsoleted the
comment about that boilerplate, clean that up now.
---
lib/dynamic_debug.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 321437bbf87d..2989a590ce9a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -853,13 +853,6 @@ static const struct seq_operations ddebug_proc_seqops = {
.stop = ddebug_proc_stop
};
-/*
- * File_ops->open method for <debugfs>/dynamic_debug/control. Does
- * the seq_file setup dance, and also creates an iterator to walk the
- * _ddebugs. Note that we create a seq_file always, even for O_WRONLY
- * files where it's not needed, as doing so simplifies the ->release
- * method.
- */
static int ddebug_proc_open(struct inode *inode, struct file *file)
{
vpr_info("called\n");
--
2.26.2
On Sat, 2020-06-13 at 09:57 -0600, Jim Cromie wrote:
> reduce word count via macro, no actual object change.
>
> OTOH, maybe() could be scrubbed if printk's default printing (iirc) of
> "(null)" pointers is deemed appropriate for the log-msg.
[]
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
[]
> @@ -114,6 +114,7 @@ do { \
> #define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
> #define v2pr_info(fmt, ...) vnpr_info(2, fmt, ##__VA_ARGS__)
>
> +#define maybe(str, empty) ( str ? str : empty )
This macro is unnecessary.
An even shorter very commonly used gcc extension would be
str ?: empty
Hi Jim,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on next-20200613]
[cannot apply to jeyu/modules-next linux/master v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jim-Cromie/dynamic_debug-cleanups-query-features-WIP-print-classes/20200614-000302
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git df2fbf5bfa0e7fff8b4784507e4d68f200454318
config: um-allmodconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=um
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
In file included from include/linux/kernel.h:11,
from include/linux/skbuff.h:13,
from include/linux/netlink.h:7,
from include/uapi/linux/genetlink.h:6,
from include/linux/genetlink.h:5,
from include/net/genetlink.h:5,
from drivers/net/wireless/ti/wl18xx/event.c:8:
include/asm-generic/fixmap.h: In function 'fix_to_virt':
include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
32 | BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
| ^~
include/linux/compiler.h:372:9: note: in definition of macro '__compiletime_assert'
372 | if (!(condition)) | ^~~~~~~~~
include/linux/compiler.h:392:2: note: in expansion of macro '_compiletime_assert'
392 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
32 | BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
| ^~~~~~~~~~~~
In file included from include/linux/uaccess.h:11,
from include/linux/sched/task.h:11,
from include/linux/sched/signal.h:9,
from include/linux/rcuwait.h:6,
from include/linux/percpu-rwsem.h:7,
from include/linux/fs.h:33,
from include/linux/huge_mm.h:8,
from include/linux/mm.h:675,
from include/linux/bvec.h:13,
from include/linux/skbuff.h:17,
from include/linux/netlink.h:7,
from include/uapi/linux/genetlink.h:6,
from include/linux/genetlink.h:5,
from include/net/genetlink.h:5,
from drivers/net/wireless/ti/wl18xx/event.c:8:
arch/um/include/asm/uaccess.h: In function '__access_ok':
arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
17 | (((unsigned long) (addr) >= FIXADDR_USER_START) && | ^~
arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall'
45 | __access_ok_vsyscall(addr, size) ||
| ^~~~~~~~~~~~~~~~~~~~
In file included from include/linux/printk.h:404,
from include/linux/kernel.h:15,
from include/linux/skbuff.h:13,
from include/linux/netlink.h:7,
from include/uapi/linux/genetlink.h:6,
from include/linux/genetlink.h:5,
from include/net/genetlink.h:5,
from drivers/net/wireless/ti/wl18xx/event.c:8:
drivers/net/wireless/ti/wl18xx/event.c: In function 'wlcore_smart_config_decode_event':
include/linux/kernel.h:943:63: warning: comparison is always true due to limited range of data type [-Wtype-limits]
943 | #define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
| ^
include/linux/dynamic_debug.h:138:8: note: in definition of macro '__dynamic_func_call_no_desc_cl'
138 | func(__VA_ARGS__); | ^~~~~~~~~~~
>> include/linux/dynamic_debug.h:163:2: note: in expansion of macro '_dynamic_func_call_no_desc_cl'
163 | _dynamic_func_call_no_desc_cl(0, fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:192:2: note: in expansion of macro '_dynamic_func_call_no_desc'
192 | _dynamic_func_call_no_desc(__builtin_constant_p(prefix_str) ? prefix_str : "hexdump", | ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:594:2: note: in expansion of macro 'dynamic_hex_dump'
594 | dynamic_hex_dump(prefix_str, prefix_type, rowsize, | ^~~~~~~~~~~~~~~~
drivers/net/wireless/ti/wl18xx/../wlcore/debug.h:91:4: note: in expansion of macro 'print_hex_dump_debug'
91 | print_hex_dump_debug(DRIVER_PREFIX prefix, | ^~~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:876:3: note: in expansion of macro '__cmp'
876 | __cmp(x, y, op), | ^~~~~
include/linux/kernel.h:943:27: note: in expansion of macro '__careful_cmp'
943 | #define min_t(type, x, y) __careful_cmp((type)(x), (type)(y), <)
| ^~~~~~~~~~~~~
drivers/net/wireless/ti/wl18xx/../wlcore/debug.h:94:6: note: in expansion of macro 'min_t'
94 | min_t(size_t, len, DEBUG_DUMP_LIMIT), | ^~~~~
drivers/net/wireless/ti/wl18xx/event.c:86:2: note: in expansion of macro 'wl1271_dump_ascii'
86 | wl1271_dump_ascii(DEBUG_EVENT, "SSID:", ssid, ssid_len);
| ^~~~~~~~~~~~~~~~~
vim +/_dynamic_func_call_no_desc_cl +163 include/linux/dynamic_debug.h
126
127 #define __dynamic_func_call_cl(cl, id, fmt, func, ...) do { \
128 DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, id, fmt); \
129 if (DYNAMIC_DEBUG_BRANCH(id)) \
130 func(&id, ##__VA_ARGS__); \
131 } while (0)
132 #define __dynamic_func_call(id, fmt, func, ...) \
133 __dynamic_func_call_cl(0, id, fmt, func, ...)
134
135 #define __dynamic_func_call_no_desc_cl(cl, id, fmt, func, ...) do { \
136 DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, id, fmt); \
137 if (DYNAMIC_DEBUG_BRANCH(id)) \
138 func(__VA_ARGS__); \
139 } while (0)
140 #define __dynamic_func_call_no_desc(cl, id, fmt, func, ...) \
141 __dynamic_func_call_no_desc_cl(0, cl, id, fmt, func, ##__VA_ARGS__)
142
143 /*
144 * "Factory macro" for generating a call to func, guarded by a
145 * DYNAMIC_DEBUG_BRANCH. The dynamic debug descriptor will be
146 * initialized using the fmt argument. The function will be called with
147 * the address of the descriptor as first argument, followed by all
148 * the varargs. Note that fmt is repeated in invocations of this
149 * macro.
150 */
151 #define _dynamic_func_call_cl(cl, fmt, func, ...) \
152 __dynamic_func_call_cl(cl, __UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
153 #define _dynamic_func_call(fmt, func, ...) \
154 _dynamic_func_call_cl(0, fmt, func, ##__VA_ARGS__)
155 /*
156 * A variant that does the same, except that the descriptor is not
157 * passed as the first argument to the function; it is only called
158 * with precisely the macro's varargs.
159 */
160 #define _dynamic_func_call_no_desc_cl(cl, fmt, func, ...) \
161 __dynamic_func_call_no_desc_cl(cl, __UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
162 #define _dynamic_func_call_no_desc(fmt, func, ...) \
> 163 _dynamic_func_call_no_desc_cl(0, fmt, func, ##__VA_ARGS__)
164
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Sat, 2020-06-13 at 09:57 -0600, Jim Cromie wrote:
> 3- pr_class:5 gives 32 print-classes
> we can map [1-9a-w] to select any pr_class with 1 char
> then "12", "af" work as noted.
> it is succinct, but arcane.
arcane generally isn't useful.
> but it does allow mnemonic choices of pr_class
> - l,m,h low, mid, hi
> - l,r left right
I believe there to be no value in mnemonic input
to debug_<class>
I think 0x<hex_mask>, 0b<binary_bits>, 0<octal_bits>
and <decimal> level is more than enough.
And likely just 0x<hex> and <decimal> is enough.
Hi Jim,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on next-20200613]
[cannot apply to jeyu/modules-next linux/master v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jim-Cromie/dynamic_debug-cleanups-query-features-WIP-print-classes/20200614-000302
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git df2fbf5bfa0e7fff8b4784507e4d68f200454318
config: arc-allyesconfig (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/dma/iop-adma.c: In function 'iop_adma_alloc_chan_resources':
drivers/dma/iop-adma.c:448:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
448 | hw_desc = (char *) iop_chan->device->dma_desc_pool;
| ^
drivers/dma/iop-adma.c:450:4: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
450 | (dma_addr_t) &hw_desc[idx * IOP_ADMA_SLOT_SIZE];
| ^
In file included from include/linux/printk.h:404,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/dma/iop-adma.c:13:
drivers/dma/iop-adma.c: In function 'iop_adma_probe':
drivers/dma/iop-adma.c:1302:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
1302 | (void *) adev->dma_desc_pool);
| ^
include/linux/dynamic_debug.h:130:15: note: in definition of macro '__dynamic_func_call_cl'
130 | func(&id, ##__VA_ARGS__); | ^~~~~~~~~~~
>> include/linux/dynamic_debug.h:170:2: note: in expansion of macro '_dynamic_func_call_cl'
170 | _dynamic_func_call_cl(cl, fmt, __dynamic_dev_dbg, | ^~~~~~~~~~~~~~~~~~~~~
>> include/linux/dynamic_debug.h:184:2: note: in expansion of macro 'dynamic_dev_dbg_cl'
184 | dynamic_dev_dbg_cl(0, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:115:2: note: in expansion of macro 'dynamic_dev_dbg'
115 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
drivers/dma/iop-adma.c:1300:2: note: in expansion of macro 'dev_dbg'
1300 | dev_dbg(&pdev->dev, "%s: allocated descriptor pool virt %p phys %pn",
| ^~~~~~~
vim +/_dynamic_func_call_cl +170 include/linux/dynamic_debug.h
126
127 #define __dynamic_func_call_cl(cl, id, fmt, func, ...) do { \
128 DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, id, fmt); \
129 if (DYNAMIC_DEBUG_BRANCH(id)) \
130 func(&id, ##__VA_ARGS__); \
131 } while (0)
132 #define __dynamic_func_call(id, fmt, func, ...) \
133 __dynamic_func_call_cl(0, id, fmt, func, ...)
134
135 #define __dynamic_func_call_no_desc_cl(cl, id, fmt, func, ...) do { \
136 DEFINE_DYNAMIC_DEBUG_METADATA_CL(cl, id, fmt); \
137 if (DYNAMIC_DEBUG_BRANCH(id)) \
138 func(__VA_ARGS__); \
139 } while (0)
140 #define __dynamic_func_call_no_desc(cl, id, fmt, func, ...) \
141 __dynamic_func_call_no_desc_cl(0, cl, id, fmt, func, ##__VA_ARGS__)
142
143 /*
144 * "Factory macro" for generating a call to func, guarded by a
145 * DYNAMIC_DEBUG_BRANCH. The dynamic debug descriptor will be
146 * initialized using the fmt argument. The function will be called with
147 * the address of the descriptor as first argument, followed by all
148 * the varargs. Note that fmt is repeated in invocations of this
149 * macro.
150 */
151 #define _dynamic_func_call_cl(cl, fmt, func, ...) \
152 __dynamic_func_call_cl(cl, __UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
153 #define _dynamic_func_call(fmt, func, ...) \
154 _dynamic_func_call_cl(0, fmt, func, ##__VA_ARGS__)
155 /*
156 * A variant that does the same, except that the descriptor is not
157 * passed as the first argument to the function; it is only called
158 * with precisely the macro's varargs.
159 */
160 #define _dynamic_func_call_no_desc_cl(cl, fmt, func, ...) \
161 __dynamic_func_call_no_desc_cl(cl, __UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
162 #define _dynamic_func_call_no_desc(fmt, func, ...) \
163 _dynamic_func_call_no_desc_cl(0, fmt, func, ##__VA_ARGS__)
164
165 #define dynamic_pr_debug_cl(cl, fmt, ...) \
166 _dynamic_func_call_cl(cl, fmt, __dynamic_pr_debug, \
167 pr_fmt(fmt), ##__VA_ARGS__)
168
169 #define dynamic_dev_dbg_cl(cl, dev, fmt, ...) \
> 170 _dynamic_func_call_cl(cl, fmt, __dynamic_dev_dbg, \
171 dev, fmt, ##__VA_ARGS__)
172
173 #define dynamic_netdev_dbg_cl(cl, dev, fmt, ...) \
174 _dynamic_func_call_cl(cl, fmt, __dynamic_netdev_dbg, \
175 dev, fmt, ##__VA_ARGS__)
176
177 #define dynamic_ibdev_dbg_cl(cl, dev, fmt, ...) \
178 _dynamic_func_call_cl(cl, fmt, __dynamic_ibdev_dbg, \
179 dev, fmt, ##__VA_ARGS__)
180
181 #define dynamic_pr_debug(...) \
182 dynamic_pr_debug_cl(0, ##__VA_ARGS__)
183 #define dynamic_dev_dbg(...) \
> 184 dynamic_dev_dbg_cl(0, ##__VA_ARGS__)
185 #define dynamic_netdev_dbg(...) \
186 dynamic_netdev_dbg_cl(0, ##__VA_ARGS__)
187 #define dynamic_ibdev_dbg(...) \
188 dynamic_ibdev_dbg_cl(0, ##__VA_ARGS__)
189
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Sat, Jun 13, 2020 at 09:57:15AM -0600, Jim Cromie wrote:
> Regarding:
> commit 2b6783191da7 ("dynamic_debug: add trim_prefix() to provide source-root relative paths")
> commit a73619a845d5 ("kbuild: use -fmacro-prefix-map to make __FILE__ a relative path")
>
> 2nd commit broke dynamic-debug's "file $fullpath" query form, but
> nobody noticed because 1st commit had trimmed prefixes from
> control-file output, so the click-copy-pasting of fullpaths into new
> queries had ceased; that query form became unused.
>
> Removing the function is cleanest, but it could be useful in
> old-compiler corner cases, where __FILE__ still has /full/path,
> and it safely does nothing otherwize.
>
> So instead, quietly deprecate "file /full/path" query form, by
> removing all /full/paths examples in the docs. I skipped adding a
> back-compat note.
> ---
> .../admin-guide/dynamic-debug-howto.rst | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
None of your patches have a signed-off-by line so they can't be applied
anywhere :(
On Sat, Jun 13, 2020 at 09:57:37AM -0600, Jim Cromie wrote:
> put pr_debug()s into most functions, to easily see code operate when
> module is loaded and used.
>
> #> dmesg -w &
> #> modprobe kset-example dyndbg=+pfml
> #> cat /sys/kernel/kset-example/*/*
> ---
> samples/kobject/kset-example.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
> index c8010f126808..27c9b1beec28 100644
> --- a/samples/kobject/kset-example.c
> +++ b/samples/kobject/kset-example.c
> @@ -56,6 +56,7 @@ static ssize_t foo_attr_show(struct kobject *kobj,
> struct foo_attribute *attribute;
> struct foo_obj *foo;
>
> + pr_debug("called");
> attribute = to_foo_attr(attr);
> foo = to_foo_obj(kobj);
>
> @@ -76,6 +77,7 @@ static ssize_t foo_attr_store(struct kobject *kobj,
> struct foo_attribute *attribute;
> struct foo_obj *foo;
>
> + pr_debug("called");
> attribute = to_foo_attr(attr);
> foo = to_foo_obj(kobj);
>
> @@ -102,6 +104,7 @@ static void foo_release(struct kobject *kobj)
> {
> struct foo_obj *foo;
>
> + pr_debug("called");
> foo = to_foo_obj(kobj);
> kfree(foo);
> }
> @@ -112,6 +115,7 @@ static void foo_release(struct kobject *kobj)
> static ssize_t foo_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
> char *buf)
> {
> + pr_debug("called");
> return sprintf(buf, "%d\n", foo_obj->foo);
> }
>
> @@ -120,6 +124,7 @@ static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
> {
> int ret;
>
> + pr_debug("called");
> ret = kstrtoint(buf, 10, &foo_obj->foo);
> if (ret < 0)
> return ret;
> @@ -140,6 +145,7 @@ static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
> {
> int var;
>
> + pr_debug("called");
> if (strcmp(attr->attr.name, "baz") == 0)
> var = foo_obj->baz;
> else
> @@ -152,6 +158,7 @@ static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
> {
> int var, ret;
>
> + pr_debug("called");
> ret = kstrtoint(buf, 10, &var);
> if (ret < 0)
> return ret;
> @@ -201,6 +208,7 @@ static struct foo_obj *create_foo_obj(const char *name)
> struct foo_obj *foo;
> int retval;
>
> + pr_debug("called");
> /* allocate the memory for the whole object */
> foo = kzalloc(sizeof(*foo), GFP_KERNEL);
> if (!foo)
> @@ -235,11 +243,13 @@ static struct foo_obj *create_foo_obj(const char *name)
>
> static void destroy_foo_obj(struct foo_obj *foo)
> {
> + pr_debug("called");
> kobject_put(&foo->kobj);
> }
>
> static int __init example_init(void)
> {
> + pr_debug("called");
Why??? If you want to do something like this, use ftrace, that is what
it is for.
thanks,
greg k-h
On Sun, Jun 14, 2020 at 12:04 AM Greg KH <[email protected]> wrote:
>
> On Sat, Jun 13, 2020 at 09:57:15AM -0600, Jim Cromie wrote:
> > Regarding:
> > ---
> > .../admin-guide/dynamic-debug-howto.rst | 19 +++++++++----------
> > 1 file changed, 9 insertions(+), 10 deletions(-)
>
> None of your patches have a signed-off-by line so they can't be applied
> anywhere :(
oof. I missed the -s in format-patch
will resend soon, after some time for remaining feedback.
On Sat 2020-06-13 09:57:21, Jim Cromie wrote:
> ddebug_describe_flags currently fills a caller provided string buffer,
> after testing its size (also passed) in a BUG_ON. Fix this by
> replacing them with a known-big-enough string buffer wrapped in a
> struct, and passing that instead.
>
> Also simplify the flags parameter, and instead de-ref the flags struct
> in the caller; this makes the function reusable (soon) where flags are
> unpacked.
In all patches is missing:
Signed-off-by: Jim Cromie <[email protected]>
> ---
> lib/dynamic_debug.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 9b2445507988..aaace13d7536 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -87,22 +87,22 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
> { _DPRINTK_FLAGS_NONE, '_' },
> };
>
> +struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
This looks too complicated. What about?
typedef char flags_buf[ARRAY_SIZE(opt_array) + 1];
used as
flags_buf fb;
#define FLAGS_BUF_SIZE (ARRAY_SIZE(opt_array) + 1)
used as
char flags_buf[FLAGS_BUF_SIZE];
Best Regards,
Petr
On Sat 2020-06-13 09:57:23, Jim Cromie wrote:
> reduce word count via macro, no actual object change.
>
> OTOH, maybe() could be scrubbed if printk's default printing (iirc) of
> "(null)" pointers is deemed appropriate for the log-msg.
> ---
> lib/dynamic_debug.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 31d3be30776e..20b712652ee4 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -114,6 +114,7 @@ do { \
> #define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
> #define v2pr_info(fmt, ...) vnpr_info(2, fmt, ##__VA_ARGS__)
>
> +#define maybe(str, empty) ( str ? str : empty )
> static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
> {
> /* trim any trailing newlines */
> @@ -127,10 +128,10 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
>
> vpr_info("%s: func=\"%s\" file=\"%s\" module=\"%s\" format=\"%.*s\" lineno=%u-%u\n",
> msg,
> - query->function ? query->function : "",
> - query->filename ? query->filename : "",
> - query->module ? query->module : "",
> - fmtlen, query->format ? query->format : "",
> + maybe(query->function, ""),
> + maybe(query->filename, ""),
> + maybe(query->module, ""),
> + fmtlen, maybe(query->format, ""),
From my POV this makes the code less readable. Open coding is much
more clear than an ambiguous macro name.
Best Regards,
Petr
On Sat 2020-06-13 09:57:24, Jim Cromie wrote:
> make the code-block reusable to later handle "file foo.c:101-200" etc.
> This should be a 90%+ code-move, with minimal adaptations; reindent,
> and scafolding.
This sentence sounds like the author did some hidden
microoptimizations and potentially broke the code.
It made me nervous.
But in fact, I do not see any real change except that the variable
"first" does not longer need to be defined. So, it is just a code move.
In this case, I usually write:
This patch does not change the existing behavior.
Best Regards,
Petr
On Sat 2020-06-13 09:57:25, Jim Cromie wrote:
> Accept these additional query forms:
>
> echo "file $filestr +_" > control
>
> path/to/file.c:100 # as from control, column 1
> path/to/file.c:1-100 # or any legal line-range
> path/to/file.c:func_A # as from an editor/browser
> path/to/file.c:drm_\* # wildcards still work
^
Should the backslash be there?
> path/to/file.c:*_foo # lead wildcard too
>
> 1st 2 examples are treated as line-ranges, 3,4 are treated as func's
There is also 5th example.
> Doc these changes, and sprinkle in a few extra wild-card examples and
> trailing # explanation texts.
> ---
> .../admin-guide/dynamic-debug-howto.rst | 5 +++++
> lib/dynamic_debug.c | 20 ++++++++++++++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 1423af580bed..6c04aea8f4cd 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -164,6 +164,7 @@ func
> of each callsite. Example::
>
> func svc_tcp_accept
> + func *recv* # in rfcomm, bluetooth, ping, tcp
>
> file
> The given string is compared against either the src-root relative
> @@ -172,6 +173,9 @@ file
>
> file svcsock.c
> file kernel/freezer.c # ie column 1 of control file
> + file drivers/usb/* # all callsites under it
> + file inode.c:start_* # parse :tail as a func (above)
> + file inode.c:1-100 # parse :tail as a line-range (above)
>
> module
> The given string is compared against the module name
> @@ -181,6 +185,7 @@ module
>
> module sunrpc
> module nfsd
> + module drm* # both drm, drm_kms_helper
>
> format
> The given string is searched for in the dynamic debug format
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index f87a7bef4204..784c075c7db9 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -322,6 +322,8 @@ static int parse_linerange(struct ddebug_query *query, const char *first)
> } else {
> query->last_lineno = query->first_lineno;
> }
> + vpr_info("parsed line %d-%d\n", query->first_lineno,
> + query->last_lineno);
Is this supposed to be in the final code?
I do not see such messages printed for other parsed variants.
> return 0;
> }
>
> @@ -358,6 +360,7 @@ static int ddebug_parse_query(char *words[], int nwords,
> {
> unsigned int i;
> int rc = 0;
> + char *fline;
>
> /* check we have an even number of words */
> if (nwords % 2 != 0) {
> @@ -374,7 +377,22 @@ static int ddebug_parse_query(char *words[], int nwords,
> if (!strcmp(words[i], "func")) {
> rc = check_set(&query->function, words[i+1], "func");
> } else if (!strcmp(words[i], "file")) {
> - rc = check_set(&query->filename, words[i+1], "file");
> + if (check_set(&query->filename, words[i+1], "file"))
> + return -EINVAL;
There is no reason to hard code the error code. It should look like:
rc = check_set(&query->filename, words[i+1], "file");
if (rc)
return rc;
> +
> + /* tail :$info is function or line-range */
> + fline = strchr(query->filename, ':');
> + if (!fline)
> + break;
> + *fline++ = '\0';
> + if (isalpha(*fline) || *fline == '*' || *fline == '?') {
I would do the oposite and check whether is starts with number.
> + /* take as function name */
> + if (check_set(&query->function, fline, "func"))
> + return -EINVAL;
> + } else {
> + if (parse_linerange(query, fline))
> + return -EINVAL;
> + }
Also I would hide this into another function:
rc = parse_filenane(...);
> } else if (!strcmp(words[i], "module")) {
> rc = check_set(&query->module, words[i+1], "module");
> } else if (!strcmp(words[i], "format")) {
> --
> 2.26.2
Best Regards,
Petr
On Sat 2020-06-13 09:57:27, Jim Cromie wrote:
> combine flags & mask into a struct, and replace those 2 parameters in
> 3 functions: ddebug_change, ddebug_parse_flags, ddebug_read_flags,
> altering the derefs in them accordingly.
>
> This simplifies the 3 function sigs, preparing for more changes.
> We dont yet need mask from ddebug_read_flags, but will soon.
> ---
> lib/dynamic_debug.c | 46 +++++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 93c627e9c094..8dc073a6e8a4 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -62,6 +62,11 @@ struct ddebug_iter {
> unsigned int idx;
> };
>
> +struct flagsettings {
Please. use underscore to help with parsing such a long names.
I mean use: flags_settings.
> + unsigned int flags;
> + unsigned int mask;
> +};
static int ddebug_change(const struct ddebug_query *query,
> - unsigned int pflags, unsigned int mask)
> + struct flagsettings *mods)
> -static int ddebug_read_flags(const char *str, unsigned int *flags)
> +static int ddebug_read_flags(const char *str, struct flagsettings *f)
> -static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
> - unsigned int *maskp)
> +static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
What "mods" stands for, please?
I have to say that using a parameter called "mods" in a function
called parse_flags() is inconsistent and confusing.
Best Regards,
Petr
On Sat 2020-06-13 09:57:28, Jim Cromie wrote:
> Add a new local *filter variable to ddebug_exec_query(), pass it into
> ddebug_parse_flags(), which fills it, communicating optional filter
> flags back to its caller. Then caller passes same to ddebug_change()
> to effect the changes.
>
> Also, ddebug_change doesn't alter any of its arguments, including its 2
> new ones; mods, filter. Say so by adding const modifier to them.
What is the purpose of the filter variable, please?
It is newer set. It is printed in ddebug_parse_flags().
It is passed to ddebug_exec_query() but never used there.
Best Regards,
Petr
On Sat 2020-06-13 09:57:29, Jim Cromie wrote:
> change ddebug_parse_flags to accept optional filterflags before OP.
> this now sets the parameter added in ~1
What is "~1", please?
> ---
> .../admin-guide/dynamic-debug-howto.rst | 18 +++++++----
> lib/dynamic_debug.c | 30 ++++++++++---------
> 2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 6c04aea8f4cd..4f343e6036f5 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -217,13 +217,19 @@ line
> line -1605 // the 1605 lines from line 1 to line 1605
> line 1600- // all lines from line 1600 to the end of the file
>
> -The flags specification comprises a change operation followed
> -by one or more flag characters. The change operation is one
> -of the characters::
This removes rather useful information and there is no replacement.
> +Flags Specification::
>
> - - remove the given flags
> - + add the given flags
> - = set the flags to the given flags
> + flagspec ::= filterflags? OP modflags
> + filterflags ::= flagset
> + modflags ::= flagset
> + flagset ::= ([pfmltu_] | [PFMLTU_])+ # also cant have pP etc
> + OP ::= [-+=]
I have to say that dynamic debug interface always looked pretty
complicated to me. But I have no idea what the above means.
It explans some syntax. But it does not explain what filterfalgs
and modflags mean and how they would affect the operation.
Also some examples would be very useful.
Best Regards,
Petr
On Sat, Jun 13, 2020 at 10:14 AM Joe Perches <[email protected]> wrote:
>
> On Sat, 2020-06-13 at 09:57 -0600, Jim Cromie wrote:
> > +#define maybe(str, empty) ( str ? str : empty )
>
> This macro is unnecessary.
>
> An even shorter very commonly used gcc extension would be
>
> str ?: empty
>
ooh yes. I'll do this
On Mon, Jun 15, 2020 at 7:28 AM Petr Mladek <[email protected]> wrote:
>
> On Sat 2020-06-13 09:57:23, Jim Cromie wrote:
> From my POV this makes the code less readable. Open coding is much
> more clear than an ambiguous macro name.
>
> Best Regards,
> Petr
Im going with Joes approach, which addresses your concerns too
thanks
On Mon, Jun 15, 2020 at 7:20 AM Petr Mladek <[email protected]> wrote:
>
> On Sat 2020-06-13 09:57:21, Jim Cromie wrote:
> In all patches is missing:
>
> Signed-off-by: Jim Cromie <[email protected]>
right, I missed the -s invoking format-patch, v3 will have them
>
> > ---
> > lib/dynamic_debug.c | 31 +++++++++++++++----------------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 9b2445507988..aaace13d7536 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -87,22 +87,22 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
> > { _DPRINTK_FLAGS_NONE, '_' },
> > };
> >
> > +struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
>
> This looks too complicated. What about?
>
> typedef char flags_buf[ARRAY_SIZE(opt_array) + 1];
> used as
> flags_buf fb;
>
I used the struct to give type safety.
old code passed a pointer to a string, and hoped it was big enough.
then used a BUG_ON to insist.
passing (the address of the) struct means the contained string is
known big enough.
and the addy is also that of the string itself (member offset 0), no overhead.
>
> #define FLAGS_BUF_SIZE (ARRAY_SIZE(opt_array) + 1)
> used as
> char flags_buf[FLAGS_BUF_SIZE];
>
I never needed that constant, cuz the string is filled once,
in the function just below the struct def, using the same expression (sans +1)
I would/will update the 1-line comment on ddebug_describe_flags
and add another on the struct itself, once I figure out how to say
all of this succinctly and clearly enough.
Im open to suggestions.
thanks
jimc
>
> Best Regards,
> Petr
On Sun, Jun 14, 2020 at 12:05 AM Greg KH <[email protected]> wrote:
>
> On Sat, Jun 13, 2020 at 09:57:37AM -0600, Jim Cromie wrote:
> > put pr_debug()s into most functions, to easily see code operate when
> > module is loaded and used.
> >
> > #> dmesg -w &
> > #> modprobe kset-example dyndbg=+pfml
> > #> cat /sys/kernel/kset-example/*/*
> > ---
> > static int __init example_init(void)
> > {
> > + pr_debug("called");
>
> Why??? If you want to do something like this, use ftrace, that is what
> it is for.
>
> thanks,
>
> greg k-h
mostly I needed an easy place to try out pr_debug_n in the next patch.
if that next patch seems like a good anti-pattern for pr_debug_n use/misuse,
then I could combine the 2, and add a 'dont do this, use ftrace' comment too.
or not, of course.
On Mon, Jun 15, 2020 at 7:37 AM Petr Mladek <[email protected]> wrote:
>
> On Sat 2020-06-13 09:57:24, Jim Cromie wrote:
> > make the code-block reusable to later handle "file foo.c:101-200" etc.
>
> > This should be a 90%+ code-move, with minimal adaptations; reindent,
> > and scafolding.
>
> This sentence sounds like the author did some hidden
> microoptimizations and potentially broke the code.
> It made me nervous.
>
> But in fact, I do not see any real change except that the variable
> "first" does not longer need to be defined. So, it is just a code move.
>
> In this case, I usually write:
>
> This patch does not change the existing behavior.
I see your point.
it was code move, reindent, add function wrapper, add call, compile
I just dont recall if I had to touch anything else, add/move var decls etc.
On Mon, Jun 15, 2020 at 9:14 AM Petr Mladek <[email protected]> wrote:
>
> On Sat 2020-06-13 09:57:27, Jim Cromie wrote:
> > combine flags & mask into a struct, and replace those 2 parameters in
> > 3 functions: ddebug_change, ddebug_parse_flags, ddebug_read_flags,
> > altering the derefs in them accordingly.
> >
> > This simplifies the 3 function sigs, preparing for more changes.
> > We dont yet need mask from ddebug_read_flags, but will soon.
> > ---
> > lib/dynamic_debug.c | 46 +++++++++++++++++++++++----------------------
> > 1 file changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 93c627e9c094..8dc073a6e8a4 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -62,6 +62,11 @@ struct ddebug_iter {
> > unsigned int idx;
> > };
> >
> > +struct flagsettings {
>
> Please. use underscore to help with parsing such a long names.
> I mean use: flags_settings.
>
ok
> > + unsigned int flags;
> > + unsigned int mask;
> > +};
>
> static int ddebug_change(const struct ddebug_query *query,
> > - unsigned int pflags, unsigned int mask)
> > + struct flagsettings *mods)
>
> > -static int ddebug_read_flags(const char *str, unsigned int *flags)
> > +static int ddebug_read_flags(const char *str, struct flagsettings *f)
>
> > -static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
> > - unsigned int *maskp)
> > +static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
>
> What "mods" stands for, please?
>
modifying_flags, or modifiers.
the original flags & mask bundled together
ie the pfmlt in
echo +pfmlt > control
> I have to say that using a parameter called "mods" in a function
> called parse_flags() is inconsistent and confusing.
>
does the above help ?
I could go with modifying_flags
keep in mind the name has to suit all + - = operations
I'll review all the new names. I recall you didnt like filterflags either,
so I wasnt sufficently clear there either.
Im mulling a better explanation.
> Best Regards,
> Petr
On Mon, Jun 15, 2020 at 04:18:38PM -0600, [email protected] wrote:
> On Sun, Jun 14, 2020 at 12:05 AM Greg KH <[email protected]> wrote:
> >
> > On Sat, Jun 13, 2020 at 09:57:37AM -0600, Jim Cromie wrote:
> > > put pr_debug()s into most functions, to easily see code operate when
> > > module is loaded and used.
> > >
> > > #> dmesg -w &
> > > #> modprobe kset-example dyndbg=+pfml
> > > #> cat /sys/kernel/kset-example/*/*
> > > ---
>
> > > static int __init example_init(void)
> > > {
> > > + pr_debug("called");
> >
> > Why??? If you want to do something like this, use ftrace, that is what
> > it is for.
> >
> > thanks,
> >
> > greg k-h
>
>
> mostly I needed an easy place to try out pr_debug_n in the next patch.
> if that next patch seems like a good anti-pattern for pr_debug_n use/misuse,
> then I could combine the 2, and add a 'dont do this, use ftrace' comment too.
> or not, of course.
This is not a good place to use it at all, as I do not want to see
people copying it. Anything that does "called" is ripe to just be
removed entirely.
Which again leads me to the "are you sure you want to do any of this?"
question as almost always, complex debugging stuff like this is never
used once the driver is up and running properly.
thanks,
greg k-h
On Mon 2020-06-15 23:47:26, [email protected] wrote:
> On Mon, Jun 15, 2020 at 9:14 AM Petr Mladek <[email protected]> wrote:
> >
> > On Sat 2020-06-13 09:57:27, Jim Cromie wrote:
> > > combine flags & mask into a struct, and replace those 2 parameters in
> > > 3 functions: ddebug_change, ddebug_parse_flags, ddebug_read_flags,
> > > altering the derefs in them accordingly.
> > >
> > > This simplifies the 3 function sigs, preparing for more changes.
> > > We dont yet need mask from ddebug_read_flags, but will soon.
> > > ---
> > > lib/dynamic_debug.c | 46 +++++++++++++++++++++++----------------------
> > > 1 file changed, 24 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > > index 93c627e9c094..8dc073a6e8a4 100644
> > > --- a/lib/dynamic_debug.c
> > > +++ b/lib/dynamic_debug.c
> > > +struct flagsettings {
> > > + unsigned int flags;
> > > + unsigned int mask;
> > > +};
> >
> > static int ddebug_change(const struct ddebug_query *query,
> > > - unsigned int pflags, unsigned int mask)
> > > + struct flagsettings *mods)
> >
> > > -static int ddebug_read_flags(const char *str, unsigned int *flags)
> > > +static int ddebug_read_flags(const char *str, struct flagsettings *f)
> >
> > > -static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
> > > - unsigned int *maskp)
> > > +static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
> >
> > What "mods" stands for, please?
> >
> modifying_flags, or modifiers.
> the original flags & mask bundled together
> ie the pfmlt in
> echo +pfmlt > control
Honestly, storing flags and mask is a hack that makes the code
tricky like hell.
IMHO, it would be much easier to define something like:
struct flags_operation {
unsinged int flags;
enum flags_operation_type op;
}
Where the opration would be:
enum flags_operation_type {
DD_FLAGS_SET, /* for '=' */
DD_FLAGS_ADD, /* for '+' */
DD_FLAGS_DEL, /* for '-' */
DD_FLAGS_FILTER_MATCH,
DD_FLAGS_FILTER_NON_MATCH,
};
Then you could define
struct flags_operation fop_change;
struct flags_operation fop_filter;
Then you could do in ddebug_change():
if (fop_filter) {
switch(fop_filter->op) {
case DD_FLAGS_FILTER_MATCH:
if ((dp->flags & fop_filter->flags) != fop_filter->flags)
continue;
break;
case: DD_FLAGS_FILTER_NON_MATCH:
if ((dp->flags & fop_filter->flags)
continue;
break;
default:
// warn error;
}
}
switch (fop_change->op) {
case DD_FLAGS_SET:
dp->flags = fop_change->flags;
break;
case DD_FLAGS_ADD:
dp->flags |= fop_change->flags;
break;
case DD_FLAGS_DEL:
dp->flgas &= ^(fop_change->flgas);
break;
default:
// error;
}
It might be more lines. But the bit operations will become straightforward.
and the code self-explaining,
> does the above help ?
> I could go with modifying_flags
> keep in mind the name has to suit all + - = operations
>
> I'll review all the new names. I recall you didnt like filterflags either,
> so I wasnt sufficently clear there either.
> Im mulling a better explanation.
The above would make the code manageable. Another question is the user
interface.
I still wonder if it is worth it.
What is the motivation for this fitlering?
Is it requested by users?
Or is it just a prerequisite for the user-specific filters?
We need to be really careful. User interface is hard to change
or remove later.
Best Regards,
Petr
On Sat 2020-06-13 09:57:31, Jim Cromie wrote:
> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
> effect on callsite behavior; it allows incremental marking of
> arbitrary sets of callsites.
>
> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
> And in ddebug_read_flags():
> current code does: [pfmltu_] -> flags
> copy it to: [PFMLTU_] -> mask
>
> also disallow both of a pair: ie no 'pP', no true & false.
>
> 3. Add filtering ops into ddebug_change(), right after all the
> callsite-property selections are complete. These filter on the
> callsite's current flagstate before applying modflags.
>
> Why ?
>
> The 'u' flag lets the user assemble an arbitary set of callsites.
> Then using filter flags, user can activate the 'u' set.
>
> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> #> echo 'u+p' > control
What was the motivation for this please?
Is it common to manipulate the same set of callsites again and again?
Do you have any usecase, please?
Best Regards,
Petr
On Sat 2020-06-13 09:57:32, Jim Cromie wrote:
> Extend flags modifications to allow [PFMLTU] negating flags.
> This allows control-queries like:
>
> #> Q () { echo file inode.c $* > control } # to type less
> #> Q -P # same as +p
> #> Q +U # same as -u
> #> Q u-P # same as u+p
>
> This allows flags in a callsite to be simultaneously set and cleared,
> while still starting with the current flagstate (with +- ops).
>
> Using filter-flags with negating-flags, you can select exactly the
> flagstates you want, both required and prohibited.
>
> Then with negating-flags in modflags, you can set and clear every flag
>
> #> Q umfLT-Pmf # select sites with u,m,f only. enable print, turn off m,f leave u
>
> Its not an important feature, but it does fill out the logic.
> and the patch is tiny, and feels more symmetrical.
I do not think that it is a good idea.
Many people do not like perl because it allows to do the same thing
many ways. The result is that the code is hard to read. There are too
many coding styles and tricks to understand.
Best Regards,
Petr
On Sat 2020-06-13 09:57:33, Jim Cromie wrote:
> Current code expects "keyword" "arg" as 2 space separated words.
> Change to also accept "keyword:arg" and "keyword=arg" forms as well,
> and drop !(nwords%2) requirement.
>
> Then in rest of function, use new keyword,arg variables instead of
> word[i],word[i+1]
I like the idea. But please allow only one form. IMHO, parameter=value
is a common way to pass values to commandline parameters.
Note that "keyword" and "arg" is strange naming, especially "arg".
Best Regards,
Petr
On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> 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.
I agree that it might make sense to provide some common mechanism.
> 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 only.
>
> proposed API:
>
> Usage model is for a module developer to create N exclusive subsets of
> pr_debug()s by changing some of them to pr_debug_n(1,) .. pr_debug_n(N,).
> Each callsite must be a single print-class, with 0 default.
>
> No multi-type classification ala pr_debug_M(1|2, ...) is contemplated.
>
> 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
> Qfoo mflags 0 +p # ignored atm TBD[2]
> Qfoo mflags af +p # TBD[3]: groups a or f (10 or 15)
My problem with this approach is that it is too generic. Each class
would have different meaning in each subsystem.
It might help to replace any existing variants. But it would be hard
for developers debugging the code. They would need to study/remember
the meaning of these groups for particular subsystems. They would
need to set different values for different messages.
Could you please provide more details about the potential users?
Would be possible to find some common patterns and common
meaning of the groups?
Best Regards,
Petr
On Tue, Jun 16, 2020 at 5:57 AM Petr Mladek <[email protected]> wrote:
>
> On Sat 2020-06-13 09:57:33, Jim Cromie wrote:
> > Current code expects "keyword" "arg" as 2 space separated words.
> > Change to also accept "keyword:arg" and "keyword=arg" forms as well,
> > and drop !(nwords%2) requirement.
> >
> > Then in rest of function, use new keyword,arg variables instead of
> > word[i],word[i+1]
>
> I like the idea. But please allow only one form. IMHO, parameter=value
> is a common way to pass values to commandline parameters.
>
I dont see a basis to prefer one over the other.
we already now accept " file foo.c:func "
that might argue for file=foo:func
but file:foo:func is what youd expect reading left-to-right
> Note that "keyword" and "arg" is strange naming, especially "arg".
>
I think keyword is clear in context. query_term is suitable, but no better.
arg is pretty generic, without overloaded meaning like value ( like
lvalue ? rvalue ?)
almost as old as 'i', but generally a string (not an int)
Is there an alternative you favor ?
> Best Regards,
> Petr
On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote:
> On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > 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.
>
> I agree that it might make sense to provide some common mechanism.
[]
> My problem with this approach is that it is too generic. Each class
> would have different meaning in each subsystem.
>
> It might help to replace any existing variants. But it would be hard
> for developers debugging the code. They would need to study/remember
> the meaning of these groups for particular subsystems. They would
> need to set different values for different messages.
>
> Could you please provide more details about the potential users?
> Would be possible to find some common patterns and common
> meaning of the groups?
I doubt the utility of common patterns.
Verbosity is common but groupings are not.
Look at the DRM patterns vs other groups.
$ git grep 'MODULE_PARM_DESC.*debug'
hi Petr,
On Tue, Jun 16, 2020 at 7:45 AM Petr Mladek <[email protected]> wrote:
>
> On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > 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.
>
> I agree that it might make sense to provide some common mechanism.
>
>
> > 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 only.
> >
> > proposed API:
> >
> > Usage model is for a module developer to create N exclusive subsets of
> > pr_debug()s by changing some of them to pr_debug_n(1,) .. pr_debug_n(N,).
> > Each callsite must be a single print-class, with 0 default.
> >
> > No multi-type classification ala pr_debug_M(1|2, ...) is contemplated.
> >
> > 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
> > Qfoo mflags 0 +p # ignored atm TBD[2]
> > Qfoo mflags af +p # TBD[3]: groups a or f (10 or 15)
>
> My problem with this approach is that it is too generic. Each class
> would have different meaning in each subsystem.
>
I think generic is a feature.
subsystem and module are the organizational level
where print-classes would be sensibly defined.
Thats why I required a module query-term with mflags,
so that no mflags query could operate on all the callsites.
I might go further to prohibit a wildcard in the module query-term value,
so its absolutely locked in to a specific module.
maybe there would be consensus about having 1 or 2 kernel-wide print-classes,
but other than something akin to stdin, stdout, stderr, I cant think
of what it would look like.
> It might help to replace any existing variants. But it would be hard
> for developers debugging the code. They would need to study/remember
> the meaning of these groups for particular subsystems. They would
> need to set different values for different messages.
>
> Could you please provide more details about the potential users?
Ive ccd Stanimir, who wanted HI MID LOW classifications on some of his
debug prints.
Im doing the simplest possible thing that might work for him.
> Would be possible to find some common patterns and common
> meaning of the groups?
probably should start with anti-patterns.
KISAP - simple as possible
I offer exclusive classes only, no this or that.
prclass = 0 is reserved for every current use.
If module authors think they need many print-classes, rethink.
control output exposes whole structure of code,
file and function names are chosen to convey organization,
a small set of queries will recreate most arbitrary print-classes
use the echo >control interface only.
I briefly thought about checking module debug parameters,
that now sounds like madness
Any chunk of code with N pr-debug* callsites, however badly architected,
can be completely controlled by N separate queries.
for better code, its much smaller.
ISTM the only sane way to allow external modules to control their own
dynamic debug callsites is to expose ddebug_exec_queries,
and let them issue the callsite modifications they want.
then, they can map any updates to their debug-flags storage
onto a pair of on-off queries for each bit.
Lastly, Id note that exclusive classes doesnt mean levels (debug++, debug--)
cant be handled.
we could reserve pr-classes 1-9 to mean all inclusive below N.
Or that meaning could be handled by merely issuing the fill-in activations.
In the module that wants debug levels
echo module foo mflags 4 >control
auto generates same query 3 more times, with mflags 3 flags 2 mflags:1
>
> Best Regards,
> Petr
> Or that meaning could be handled by merely issuing the fill-in activations.
> In the module that wants debug levels
>
> echo module foo mflags 4 >control
> auto generates same query 3 more times, with mflags 3 flags 2 mflags:1
>
let me also note that just because a module might do the
descending loop N..1 to implement debug levels,
doesnt mean you cant then override manually using echo >control
to say disable N=3 but keep 4,5,
except 4,5 in file x
youre in >control ;-)
>
>
> >
> > Best Regards,
> > Petr
On Tue, Jun 16, 2020 at 02:05:27PM -0700, Joe Perches wrote:
> On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote:
> > On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > > 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.
> >
> > I agree that it might make sense to provide some common mechanism.
> []
> > My problem with this approach is that it is too generic. Each class
> > would have different meaning in each subsystem.
> >
> > It might help to replace any existing variants. But it would be hard
> > for developers debugging the code. They would need to study/remember
> > the meaning of these groups for particular subsystems. They would
> > need to set different values for different messages.
> >
> > Could you please provide more details about the potential users?
> > Would be possible to find some common patterns and common
> > meaning of the groups?
>
> I doubt the utility of common patterns.
> Verbosity is common but groupings are not.
>
> Look at the DRM patterns vs other groups.
I've seen drm.debug mentioned a couple of times but the comments about
it seem to only learn part of what is shows us.
drm.debug is a form of common grouping but it acts at a sub-system level
rather then whole system (and gives a whole sub-system enable/disable).
This is where grouping makes most sense.
The result is that drm.debug is easy to document, both in official
kernel docs and in other resources (like the arch distro documentation).
Having controls that are easy to document makes them easy to find and
thus sub-system grouping leads directly to higher quality bug reports.
Daniel.
On Wed 2020-06-17 10:31:54, Daniel Thompson wrote:
> On Tue, Jun 16, 2020 at 02:05:27PM -0700, Joe Perches wrote:
> > On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote:
> > > On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > > > 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.
> > >
> > > I agree that it might make sense to provide some common mechanism.
> > []
> > > My problem with this approach is that it is too generic. Each class
> > > would have different meaning in each subsystem.
> > >
> > > It might help to replace any existing variants. But it would be hard
> > > for developers debugging the code. They would need to study/remember
> > > the meaning of these groups for particular subsystems. They would
> > > need to set different values for different messages.
> > >
> > > Could you please provide more details about the potential users?
> > > Would be possible to find some common patterns and common
> > > meaning of the groups?
> >
> > I doubt the utility of common patterns.
> > Verbosity is common but groupings are not.
> >
> > Look at the DRM patterns vs other groups.
>
> I've seen drm.debug mentioned a couple of times but the comments about
> it seem to only learn part of what is shows us.
>
> drm.debug is a form of common grouping but it acts at a sub-system level
> rather then whole system (and gives a whole sub-system enable/disable).
> This is where grouping makes most sense.
>
> The result is that drm.debug is easy to document, both in official
> kernel docs and in other resources (like the arch distro documentation).
> Having controls that are easy to document makes them easy to find and
> thus sub-system grouping leads directly to higher quality bug reports.
Thanks a lot for explanation.
Now, could anyone please tell me how this new dynamic debug feature
would allow to replace drm.debug option?
I mean what steps/commands will be needed instead of, for example
drm.debug=0x3 command line option?
Best Regards,
Petr
On Tue 2020-06-16 14:08:57, [email protected] wrote:
> On Tue, Jun 16, 2020 at 5:57 AM Petr Mladek <[email protected]> wrote:
> >
> > On Sat 2020-06-13 09:57:33, Jim Cromie wrote:
> > > Current code expects "keyword" "arg" as 2 space separated words.
> > > Change to also accept "keyword:arg" and "keyword=arg" forms as well,
> > > and drop !(nwords%2) requirement.
> > >
> > > Then in rest of function, use new keyword,arg variables instead of
> > > word[i],word[i+1]
> >
> > I like the idea. But please allow only one form. IMHO, parameter=value
> > is a common way to pass values to commandline parameters.
> >
>
> I dont see a basis to prefer one over the other.
> we already now accept " file foo.c:func "
> that might argue for file=foo:func
> but file:foo:func is what youd expect reading left-to-right
>
> > Note that "keyword" and "arg" is strange naming, especially "arg".
> >
>
> I think keyword is clear in context. query_term is suitable, but no better.
>
> arg is pretty generic, without overloaded meaning like value ( like
> lvalue ? rvalue ?)
> almost as old as 'i', but generally a string (not an int)
> Is there an alternative you favor ?
You made to do some research and I was wrong. For example, getopt()
operates with options and their arguments. So, 'keyword' and 'arg' names
look good after all.
Well, I still think that only one syntax should be supported. And it
is better to distinguish keywords and arguments, so I prefer keyword=arg.
I see "filename:func" or "filename:line" as a compound parameter. People are
familiar with this syntax, for example, from gdb.
But using '=' is very common for first level delimiter: getopt,
qemu.
Well, I do not have strong opinion on this.
Best Regards,
Petr
On Wed, Jun 17, 2020 at 3:52 AM Petr Mladek <[email protected]> wrote:
>
> On Wed 2020-06-17 10:31:54, Daniel Thompson wrote:
> > On Tue, Jun 16, 2020 at 02:05:27PM -0700, Joe Perches wrote:
> > > On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote:
> > > > On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > > > > 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.
> > > >
> > > > I agree that it might make sense to provide some common mechanism.
> > > []
> > > > My problem with this approach is that it is too generic. Each class
> > > > would have different meaning in each subsystem.
> > > >
> > > > It might help to replace any existing variants. But it would be hard
> > > > for developers debugging the code. They would need to study/remember
> > > > the meaning of these groups for particular subsystems. They would
> > > > need to set different values for different messages.
> > > >
> > > > Could you please provide more details about the potential users?
> > > > Would be possible to find some common patterns and common
> > > > meaning of the groups?
> > >
> > > I doubt the utility of common patterns.
> > > Verbosity is common but groupings are not.
> > >
> > > Look at the DRM patterns vs other groups.
> >
> > I've seen drm.debug mentioned a couple of times but the comments about
> > it seem to only learn part of what is shows us.
> >
> > drm.debug is a form of common grouping but it acts at a sub-system level
> > rather then whole system (and gives a whole sub-system enable/disable).
> > This is where grouping makes most sense.
> >
> > The result is that drm.debug is easy to document, both in official
> > kernel docs and in other resources (like the arch distro documentation).
> > Having controls that are easy to document makes them easy to find and
> > thus sub-system grouping leads directly to higher quality bug reports.
>
> Thanks a lot for explanation.
>
> Now, could anyone please tell me how this new dynamic debug feature
> would allow to replace drm.debug option?
>
> I mean what steps/commands will be needed instead of, for example
> drm.debug=0x3 command line option?
>
> Best Regards,
> Petr
[jimc@frodo linux.git]$ git log -1
commit 12a67ffb3e63c40027e251b44b2abc77463dc2da (HEAD -> dd-v3)
Author: Jim Cromie <[email protected]>
Date: Tue Jun 16 15:36:37 2020 -0600
dyndbg: export ddebug_exec_queries
Exporting ddebug_exec_queries will allow module authors using dynamic
debug to actually control/enable/disable their own pr-debug callsites
dynamically.
With it, module authors can tie any update of their internal debug
variables to a corresponding ddebug_exec_queries invocation, which
will modify all their selected callsites accordingly.
Generally, authors would exec +p or -p on some subsets of their set of
callsites, and leave fmlt flags for user to choose the appropriate
amount of structural context desired in the logs.
Depending upon the user needs, the module might be known, and
therefore a waste of screen width, function would be valuable, file
would be long and familiar to the author, etc.. That said, author
could harness the message-prefix facility if they saw fit to do so.
Any author preferences can be overridden with echo >control
Is it safe ?
ddebug_exec_queries() is currently 'exposed' to user space in
several limited ways;
1 it is called from module-load callback, where it implements the
$modname.dyndbg=+p "fake" parameter provided to all modules.
2 it handles query input from >control directly
IOW, it is "fully" exposed to local root user; exposing the same
functionality to other kernel modules is no additional risk.
The other big issue to check is locking:
dyndbg has a single mutex, taken by ddebug_change to handle >control,
and by ddebug_proc_(start|stop) to span `cat control`. ISTM this
proposed export presents no locking problems.
drm use case:
drm.debug=0x03 appears to be a kernel boot-arg example, setting 2
internal debug flags. Each bit is probably controlling a separate
subset of all debug-prints, they may be overlapping subsets.
Those subsets are *definitely* expressible as a few dyndbg queries
each. Any arbitrary subset is.
drm.dyndbg='file drm_gem_* +p' # gem debug
drm.dyndbg='file *_gem_* +p' # *gem debug
With this proposed export, drm authors could exec these examples, most
likely in the callback that handles updates to the drm.debug variable.
(END)
I should note that none of this needs the WIP patch
hi Petr
> You made to do some research and I was wrong. For example, getopt()
> operates with options and their arguments. So, 'keyword' and 'arg' names
> look good after all.
>
> Well, I still think that only one syntax should be supported. And it
> is better to distinguish keywords and arguments, so I prefer keyword=arg.
>
hehe, Im gonna cite some RFC wisdom to convince you ;-)
Be strict in what you emit, and permissive in what you accept.
I see no potential for real ambiguity that would override that bias.
thanks
jimc
On Wed, Jun 17, 2020 at 07:32:44AM -0600, [email protected] wrote:
> hi Petr
>
> > You made to do some research and I was wrong. For example, getopt()
> > operates with options and their arguments. So, 'keyword' and 'arg' names
> > look good after all.
> >
> > Well, I still think that only one syntax should be supported. And it
> > is better to distinguish keywords and arguments, so I prefer keyword=arg.
> >
>
> hehe, Im gonna cite some RFC wisdom to convince you ;-)
>
> Be strict in what you emit, and permissive in what you accept.
>
> I see no potential for real ambiguity that would override that bias.
No, the kernel should be strict in what it accepts, otherwise it is a
huge maintance burden for no good reason at all.
Only one syntax is a wise idea, stick with that please.
thanks,
greg k-h
On Wed 2020-06-17 07:23:34, [email protected] wrote:
> On Wed, Jun 17, 2020 at 3:52 AM Petr Mladek <[email protected]> wrote:
> >
> > On Wed 2020-06-17 10:31:54, Daniel Thompson wrote:
> > > On Tue, Jun 16, 2020 at 02:05:27PM -0700, Joe Perches wrote:
> > > > On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote:
> > > > > On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > > > > > 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.
> > > > >
> > > > > I agree that it might make sense to provide some common mechanism.
> > > > []
> > > > > My problem with this approach is that it is too generic. Each class
> > > > > would have different meaning in each subsystem.
> > > > >
> > > > > It might help to replace any existing variants. But it would be hard
> > > > > for developers debugging the code. They would need to study/remember
> > > > > the meaning of these groups for particular subsystems. They would
> > > > > need to set different values for different messages.
> > > > >
> > > > > Could you please provide more details about the potential users?
> > > > > Would be possible to find some common patterns and common
> > > > > meaning of the groups?
> > > >
> > > > I doubt the utility of common patterns.
> > > > Verbosity is common but groupings are not.
> > > >
> > > > Look at the DRM patterns vs other groups.
> > >
> > > I've seen drm.debug mentioned a couple of times but the comments about
> > > it seem to only learn part of what is shows us.
> > >
> > > drm.debug is a form of common grouping but it acts at a sub-system level
> > > rather then whole system (and gives a whole sub-system enable/disable).
> > > This is where grouping makes most sense.
> > >
> > > The result is that drm.debug is easy to document, both in official
> > > kernel docs and in other resources (like the arch distro documentation).
> > > Having controls that are easy to document makes them easy to find and
> > > thus sub-system grouping leads directly to higher quality bug reports.
> >
> > Thanks a lot for explanation.
> >
> > Now, could anyone please tell me how this new dynamic debug feature
> > would allow to replace drm.debug option?
> >
> > I mean what steps/commands will be needed instead of, for example
> > drm.debug=0x3 command line option?
> >
> drm use case:
>
> drm.debug=0x03 appears to be a kernel boot-arg example, setting 2
> internal debug flags. Each bit is probably controlling a separate
> subset of all debug-prints, they may be overlapping subsets.
The meaning of the bits is define in drivers/gpu/drm/drm_print.c:
MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n"
"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n"
"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n"
"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n"
"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n"
"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n"
"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n"
"\t\tBit 8 (0x100) will enable DP messages (displayport code)");
Wrappers using these id's are defined in include/drm/drm_print.h,
for example:
#define DRM_DEBUG_ATOMIC(fmt, ...) \
__drm_dbg(DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
or
#define drm_dbg_atomic(drm, fmt, ...) \
drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> Those subsets are *definitely* expressible as a few dyndbg queries
> each. Any arbitrary subset is.
>
> drm.dyndbg='file drm_gem_* +p' # gem debug
> drm.dyndbg='file *_gem_* +p' # *gem debug
I do not understand this. Each category is used in many files and
some files use more categories at the same time:
git grep DRM_DEBUG_
> With this proposed export, drm authors could exec these examples, most
> likely in the callback that handles updates to the drm.debug variable.
I am afraid that this will not work. It would be hard to maintain
especially when more categories are used in the same source file.
It would be needed some easy to use API:
/*
* Print message when this module and feature is enable in dynamic
* debug interface.
*/
pr_debug_feature(int feature_id, fmt, ...); to print a part
/* Enable/disable printing debugging messages, work during early boot??? */
dd_enable_module_feature(char *module_name, int *feature_id);
dd_disable_module_feature(char *module_name, int *feature_id);
Note that the enable/disable functions manipulates only the "p" flag
by intention. The module.debug option decides only whether the messages
should be printed or not.
IMHO, the other flags (flmt) should be global flags. It is too big
detail to be enabled per-message. IMHO, it just makes the interface
too complicated and over engineered.
Best Regards,
Petr