this is v3, changes from previous:
- moved non-controversial commits up front, refactors to help this.
- a few more minor cleanups
- left out the WIP patches
- export ddebug_exec_queries()
- accept file=foo:func only, not module:foo
- varname changes
v2: https://lore.kernel.org/lkml/[email protected]/
v1: https://lore.kernel.org/lkml/[email protected]/
Patchset starts with 11 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. etc..
Next, add 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
accept keyword=value, not just "keyword value" (and not keyword:value)
Then export ddebug_exec_queries, to tie to drm.debug, etc.
Since its an export, I expect some discussion...
gpl-only would be fine.
The Flags extension stuff has received mixed reviews.
Ive refactored these commits, partly to move drive-by-cleanups up
front, which also decluttered these controversial patches; I think
theres a cleanup value to the early rework patches, even if
filterflags doesnt make it in.
Ive reworked all the flag-features commit messages to improve
the usefulness argument, hopefully well enough now.
Jim Cromie (21):
-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: fix pr_err with empty string
dyndbg: prefer declarative init in caller, to memset in callee
dyndbg: make ddebug_tables list LIFO for add/remove_module
dyndbg: use gcc ?: to reduce word count
-feature file:func
dyndbg: refactor parse_linerange out of ddebug_parse_query
dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'
-feature, new in v3
dyndbg: accept query terms like file=bar
-export, new in v3
dyndbg: export ddebug_exec_queries
-rework
dyndbg: combine flags & mask into a struct, simplify with it
dyndbg: refactor ddebug_read_flags out of ddebug_parse_flags
dyndbg: add filter channel to the internals
-flags features exposed
dyndbg: extend ddebug_parse_flags to accept optional leading
filter-flags
dyndbg: add user-flag, negating-flags, and filtering on flags
dyndbg: allow negating flag-chars in modifier flags
.../admin-guide/dynamic-debug-howto.rst | 79 +++--
include/asm-generic/vmlinux.lds.h | 6 +-
include/linux/dynamic_debug.h | 5 +-
kernel/module.c | 2 +-
lib/dynamic_debug.c | 334 ++++++++++--------
5 files changed, 260 insertions(+), 166 deletions(-)
--
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.
Signed-off-by: Jim Cromie <[email protected]>
---
.../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
since cf964976484 in 2012, initialization is done with early_initcall,
update the Docs, which still say arch_initcall.
Signed-off-by: Jim Cromie <[email protected]>
---
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
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
Signed-off-by: Jim Cromie <[email protected]>
---
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
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 ddebug_describe_flags() flags parameter from a struct to
a member in that struct, and hoist the member deref up to the caller.
This makes the function reusable (soon) where flags are unpacked.
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9b2445507988..0cb5679f6c54 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, ...) \
@@ -147,7 +147,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct ddebug_table *dt;
unsigned int newflags;
unsigned int nfound = 0;
- char flagbuf[10];
+ struct flagsbuf fbuf;
/* search for matching ddebugs */
mutex_lock(&ddebug_lock);
@@ -204,8 +204,7 @@ static int ddebug_change(const struct ddebug_query *query,
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, &fbuf));
}
}
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
reduce word count via gcc ?: extension, no actual code change.
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index e879af4e66e0..6d0159075308 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -127,10 +127,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 : "",
+ query->function ?: "",
+ query->filename ?: "",
+ query->module ?: "",
+ fmtlen, query->format ?: "",
query->first_lineno, query->last_lineno);
}
--
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.
Signed-off-by: Jim Cromie <[email protected]>
---
.../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 ae6e523fdecd..7eb963b1bd11 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -321,6 +321,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;
}
@@ -357,6 +359,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) {
@@ -372,7 +375,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
Current code expects "keyword" "arg" as 2 space separated words.
Change to also accept "keyword=arg" form as well, and drop !(nwords%2)
requirement.
Then in rest of function, use new keyword, arg variables instead of
word[i], word[i+1]
Signed-off-by: Jim Cromie <[email protected]>
---
.../admin-guide/dynamic-debug-howto.rst | 1 +
lib/dynamic_debug.c | 51 ++++++++++++-------
2 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 6c04aea8f4cd..e5a8def45f3f 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -156,6 +156,7 @@ against. Possible keywords are:::
``line-range`` cannot contain space, e.g.
"1-30" is valid range but "1 - 30" is not.
+ ``module=foo`` combined keyword=value form is interchangably accepted
The meanings of each keyword are:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7eb963b1bd11..e1dd96178f18 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -351,6 +351,8 @@ static int check_set(const char **dest, char *src, char *name)
* line <lineno>
* line <first-lineno>-<last-lineno> // where either may be empty
*
+ * Also accept combined keyword=value and keyword:value forms
+ *
* Only 1 of each type is allowed.
* Returns 0 on success, <0 on error.
*/
@@ -360,22 +362,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 = strchr(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 */
@@ -391,18 +404,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
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.
Signed-off-by: Jim Cromie <[email protected]>
---
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 da3ed54a6521..e879af4e66e0 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -895,7 +895,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
Once again, adjust the interface between 3 static functions:
- ddebug_exec_query calls ..
- ddebug_parse_query - unchanged here, mentioned for context
- ddebug_parse_flags, to read the modifying flags
- ddebug_change, to apply mods to callsites that match the query
This time, add a 2nd flag_settings variable int ddebug_exec_query(),
to specify a secondary constraint on callsites which match the query,
and which are therefore about to be modified.
Here, we only add the channel; ddebug_parse_flags doesnt fill the
filter, and ddebug_change doesnt act upon it.
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.
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8400e4f90b67..0fcc688789f4 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -146,7 +146,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 flag_settings *modifiers)
+ const struct flag_settings *modifiers,
+ const struct flag_settings *filter)
{
int i;
struct ddebug_table *dt;
@@ -456,7 +457,9 @@ static int ddebug_read_flags(const char *str, struct flag_settings *modifiers)
* flags fields of matched _ddebug's. Returns 0 on success
* or <0 on error.
*/
-static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
+static int ddebug_parse_flags(const char *str,
+ struct flag_settings *modifiers,
+ struct flag_settings *filter)
{
int op;
@@ -489,7 +492,8 @@ static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
modifiers->flags = 0;
break;
}
- vpr_info("*flagsp=0x%x *maskp=0x%x\n", modifiers->flags, modifiers->mask);
+ vpr_info("mods:flags=0x%x,mask=0x%x filter:flags=0x%x,mask=0x%x\n",
+ modifiers->flags, modifiers->mask, filter->flags, filter->mask);
return 0;
}
@@ -498,6 +502,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
{
struct flag_settings modifiers = {};
struct ddebug_query query = {};
+ struct flag_settings filter = {};
#define MAXWORDS 9
int nwords, nfound;
char *words[MAXWORDS];
@@ -508,7 +513,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], &modifiers)) {
+ if (ddebug_parse_flags(words[nwords-1], &modifiers, &filter)) {
pr_err("flags parse failed\n");
return -EINVAL;
}
@@ -517,7 +522,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
return -EINVAL;
}
/* actually go and implement the change */
- nfound = ddebug_change(&query, &modifiers);
+ nfound = ddebug_change(&query, &modifiers, &filter);
vpr_info_dq(&query, nfound ? "applied" : "no-match");
return nfound;
--
2.26.2
flags & mask are used together everywhere, and are passed around
together between multiple functions; they belong together in a struct,
call that struct flag_settings.
Use struct flag_settings to rework 3 functions:
- ddebug_exec_query - calls other 2
- ddebug_parse_flags - fills flag_settings and returns
- ddebug_change - test all callsites against query,
modify passing sites.
benefits:
- bit-banging always needs flags & mask, best together.
- simpler function signatures
- 1 less parameter, less stack overhead
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 45 ++++++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ff97938b5849..22335f7dbac1 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -62,6 +62,11 @@ struct ddebug_iter {
unsigned int idx;
};
+struct flag_settings {
+ unsigned int flags;
+ unsigned int mask;
+};
+
static DEFINE_MUTEX(ddebug_lock);
static LIST_HEAD(ddebug_tables);
static int verbose;
@@ -141,7 +146,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 flags, unsigned int mask)
+ struct flag_settings *modifiers)
{
int i;
struct ddebug_table *dt;
@@ -190,14 +195,14 @@ static int ddebug_change(const struct ddebug_query *query,
nfound++;
- newflags = (dp->flags & mask) | flags;
+ newflags = (dp->flags & modifiers->mask) | modifiers->flags;
if (newflags == dp->flags)
continue;
#ifdef CONFIG_JUMP_LABEL
if (dp->flags & _DPRINTK_FLAGS_PRINT) {
- if (!(flags & _DPRINTK_FLAGS_PRINT))
+ if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
static_branch_disable(&dp->key.dd_key_true);
- } else if (flags & _DPRINTK_FLAGS_PRINT)
+ } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(&dp->key.dd_key_true);
#endif
dp->flags = newflags;
@@ -431,11 +436,9 @@ static int ddebug_parse_query(char *words[], int nwords,
* 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 flag_settings *modifiers)
{
- unsigned flags = 0;
- int op = '=', i;
+ int op, i;
switch (*str) {
case '+':
@@ -452,7 +455,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
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;
+ modifiers->flags |= opt_array[i].flag;
break;
}
}
@@ -461,30 +464,30 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
return -EINVAL;
}
}
- vpr_info("flags=0x%x\n", flags);
+ vpr_info("flags=0x%x\n", modifiers->flags);
- /* calculate final *flagsp, *maskp according to mask and op */
+ /* calculate final flags, mask based upon op */
switch (op) {
case '=':
- *maskp = 0;
- *flagsp = flags;
+ /* modifiers->flags already set */
+ modifiers->mask = 0;
break;
case '+':
- *maskp = ~0U;
- *flagsp = flags;
+ modifiers->mask = ~0U;
break;
case '-':
- *maskp = ~flags;
- *flagsp = 0;
+ modifiers->mask = ~modifiers->flags;
+ modifiers->flags = 0;
break;
}
- vpr_info("*flagsp=0x%x *maskp=0x%x\n", *flagsp, *maskp);
+ vpr_info("*flagsp=0x%x *maskp=0x%x\n", modifiers->flags, modifiers->mask);
+
return 0;
}
static int ddebug_exec_query(char *query_string, const char *modname)
{
- unsigned int flags = 0, mask = 0;
+ struct flag_settings modifiers = {};
struct ddebug_query query = {};
#define MAXWORDS 9
int nwords, nfound;
@@ -496,7 +499,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], &modifiers)) {
pr_err("flags parse failed\n");
return -EINVAL;
}
@@ -505,7 +508,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, &modifiers);
vpr_info_dq(&query, nfound ? "applied" : "no-match");
return nfound;
--
2.26.2
This allows a user to set some flags and clear others at the same
time. This will make it easier to use the flags for creation of
transient sets, by making it easier to change the markings on those
sets by one flag, or all flags.
Consider the 3 operators [+-=]
= resets entire flag-set, no memory of before
- clears flags only, but preserves otherwise
+ sets flags only, but preserves otherwise
It would be nice to be able to set or clear all bits, or any subset,
while preserving untouched bits. Using -/+ and negating flags
together let you do so.
echo -ft > control # in all callsites, clear 2 bits, preserve others
echo f-ft > control # if f-bit is true, clear 2 bits, preserve others
using non-empty query terms gives another dimension of selectivity
echo $qtrms -ft > control # for a callsite subset, clear 2 bits, preserve others
echo $qtrms f-ft > control # for a callsite subset, if f-bit is true, clear 2 bits, preserve others
Signed-off-by: Jim Cromie <[email protected]>
---
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 e910865b2edc..4539793e39bf 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -258,9 +258,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::
@@ -270,7 +272,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 a302a7d8a722..c539bdd7fbe3 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -498,16 +498,18 @@ static int ddebug_parse_flags(const char *str,
/* calculate final mods: flags, mask based upon op */
switch (op) {
+ unsigned int tmp;
case '=':
/* modifiers->flags already set */
modifiers->mask = 0;
break;
case '+':
- modifiers->mask = ~0U;
+ modifiers->mask = ~modifiers->mask;
break;
case '-':
+ tmp = modifiers->mask;
modifiers->mask = ~modifiers->flags;
- modifiers->flags = 0;
+ modifiers->flags = tmp;
break;
}
vpr_info("mods:flags=0x%x,mask=0x%x filter:flags=0x%x,mask=0x%x\n",
--
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 & filter flags
The 'u' flag lets the user assemble an arbitary set of callsites.
Then using filter flags, user can activate the 'u' callsite set.
#> echo 'file foo.c +u; file bar.c +u' > control # and repeat
#> echo 'u+p' > control
Of course, you can continue to just activate your set without ever
marking it 1st, but you could trivially add the markup as you go, then
be able to use it as a constraint later, to undo or modify your set.
#> echo 'file foo.c +up' >control
.. monitor, debug, finish ..
#> echo 'u-p' >control
# then later resume
#> echo 'u+p' >control
# disable some cluttering messages, and remove from u-set
#> echo 'file noisy.c function:jabber_* u-pu' >control
# for doc, recollection
grep =pu control > my-favorite-callsites
Note:
Your flagstate after boot is generally not all =_. -DDEBUG will arm
compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
enable them early, and $module.dyndbg=+p bootargs will arm them when
the module is loaded. But you could manage them with u-flags:
#> echo '-t' >control # clear t-flag to use it as 2ndary markup
#> echo 'p+ut' >control # mark the boot-enabled set of callsites
#> echo '-p' >control # clean your dmesg -w stream
... monitor, debug ..
#> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
#> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
The user flag isn't strictly needed, but with it you can avoid using
[fmlt] flags for marking, which would alter logging when enabled.
Negating-flags:
Using negating-flags in your filter-flags, you can completely specify
the matching flagstate; not just required flags, but also prohibited
flags.
So if you want to avoid altering your u-set, prohibit the flag with U,
and all marked callsites will be skipped, for whatever change.
#> echo U-pt >control
At the outer limits, you could use many flag patterns to form separate
subgroups of callsites, then enable those groups by filtering on just
their flagstates, or you could add further constraints on callsite
selection.
Signed-off-by: Jim Cromie <[email protected]>
---
.../admin-guide/dynamic-debug-howto.rst | 31 ++++++++++++++++---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 29 ++++++++++++-----
3 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index e1ea0c307fcf..e910865b2edc 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -239,16 +239,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 cf3379b40483..a302a7d8a722 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]; };
@@ -194,6 +195,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 & modifiers->mask) | modifiers->flags;
@@ -440,12 +448,19 @@ static int ddebug_read_flags(const char *str, struct flag_settings *modifiers)
if (*str == opt_array[i].opt_char) {
modifiers->flags |= opt_array[i].flag;
break;
+ } else if (*str == opt_array[i].not_char) {
+ modifiers->mask |= opt_array[i].flag;
+ break;
}
}
if (i < 0) {
pr_err("unknown flag '%c'\n", *str);
return -EINVAL;
}
+ if (modifiers->flags & modifiers->mask) {
+ pr_err("flag '%c' conflicts with previous\n", *str);
+ return -EINVAL;
+ }
}
vpr_info("flags=0x%x\n", modifiers->flags);
return 0;
--
2.26.2
Current code expects "keyword" "arg" as 2 space separated words.
Change to also accept "keyword=arg" form as well, and drop !(nwords%2)
requirement.
Then in rest of function, use new keyword,arg variables instead of
word[i],word[i+1]
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 51 ++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7eb963b1bd11..e1dd96178f18 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -351,6 +351,8 @@ static int check_set(const char **dest, char *src, char *name)
* line <lineno>
* line <first-lineno>-<last-lineno> // where either may be empty
*
+ * Also accept combined keyword=value and keyword:value forms
+ *
* Only 1 of each type is allowed.
* Returns 0 on success, <0 on error.
*/
@@ -360,22 +362,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 = strchr(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 */
@@ -391,18 +404,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
Change ddebug_parse_flags to accept optional filterflags before the
required operator [-+=]. Read the flags into the filter_flags
parameter added in the previous patch. So this now supplies the
filterflags to ddebug_exec_query.
filterflags work like query terms, they constrain what callsites get
matched before theyre modified. So like a query, they can be empty.
Filterflags let you read callsite's flagstate, including results of
previous modifications, and require that certain flags are set, before
modifying the callsite further.
So you can build up sets of callsites by marking them with a
particular flagstate, for example 'fmlt', then enable that set in a
batch.
echo fmlt+p >control
Naturally you can use almost any combo of flags you want for marking,
and can mark several different sets with different patterns. And then
you can activate them in a bunch:
echo 'ft+p; mt+p; lt+p;' >control
You just can't prohibit true flags.
Signed-off-by: Jim Cromie <[email protected]>
---
.../admin-guide/dynamic-debug-howto.rst | 18 ++++++++----
lib/dynamic_debug.c | 29 ++++++++++---------
2 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index e5a8def45f3f..e1ea0c307fcf 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -218,13 +218,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 0fcc688789f4..cf3379b40483 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -452,33 +452,36 @@ static int ddebug_read_flags(const char *str, struct flag_settings *modifiers)
}
/*
- * 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 flag_settings *modifiers,
struct flag_settings *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, modifiers))
return -EINVAL;
- /* calculate final flags, mask based upon op */
+ /* calculate final mods: flags, mask based upon op */
switch (op) {
case '=':
/* modifiers->flags already set */
--
2.26.2
this pr_err attempts to print the string after the OP, but the string
has been parsed and chopped up, so looks empty.
Signed-off-by: Jim Cromie <[email protected]>
---
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 0cb5679f6c54..1d25a846553b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -420,7 +420,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
}
}
if (i < 0) {
- pr_err("unknown flag '%c' in \"%s\"\n", *str, str);
+ pr_err("unknown flag '%c'\n", *str);
return -EINVAL;
}
}
--
2.26.2
Current code expects "keyword" "arg" as 2 space separated words.
Change to also accept "keyword=arg" form as well, and drop !(nwords%2)
requirement.
Then in rest of function, use new keyword, arg variables instead of
word[i], word[i+1]
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 51 ++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7eb963b1bd11..e1dd96178f18 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -351,6 +351,8 @@ static int check_set(const char **dest, char *src, char *name)
* line <lineno>
* line <first-lineno>-<last-lineno> // where either may be empty
*
+ * Also accept combined keyword=value and keyword:value forms
+ *
* Only 1 of each type is allowed.
* Returns 0 on success, <0 on error.
*/
@@ -360,22 +362,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 = strchr(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 */
@@ -391,18 +404,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
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
Signed-off-by: Jim Cromie <[email protected]>
---
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
Make the code-block reusable to later handle "file foo.c:101-200" etc.
This is a 99% code move, with reindent, function wrap & call, etc.
Signed-off-by: Jim Cromie <[email protected]>
---
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 6d0159075308..ae6e523fdecd 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -291,6 +291,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;
@@ -348,34 +381,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
ddebug_exec_query declares an auto var, and passes it to
ddebug_parse_query, which memsets it before using it. Drop that
memset, instead initialize the variable in the caller; let the
compiler decide how to do it.
Signed-off-by: Jim Cromie <[email protected]>
---
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 1d25a846553b..da3ed54a6521 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -330,7 +330,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> */
@@ -448,7 +447,7 @@ static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
static int ddebug_exec_query(char *query_string, const char *modname)
{
unsigned int flags = 0, mask = 0;
- struct ddebug_query query;
+ struct ddebug_query query = {};
#define MAXWORDS 9
int nwords, nfound;
char *words[MAXWORDS];
--
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.
Signed-off-by: Jim Cromie <[email protected]>
---
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
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.
Signed-off-by: Jim Cromie <[email protected]>
---
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
Move flag-char reading and validating code to a separate function.
This will allow later reuse to read 2 sets of flags:
1- flags to set or clear (after the [=-+] Operator)
2- flags to require or prohibit before changing a callsite's flagstate
Signed-off-by: Jim Cromie <[email protected]>
---
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 22335f7dbac1..8400e4f90b67 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -430,6 +430,26 @@ static int ddebug_parse_query(char *words[], int nwords,
return 0;
}
+static int ddebug_read_flags(const char *str, struct flag_settings *modifiers)
+{
+ int i;
+
+ for (; *str ; ++str) {
+ for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
+ if (*str == opt_array[i].opt_char) {
+ modifiers->flags |= opt_array[i].flag;
+ break;
+ }
+ }
+ if (i < 0) {
+ pr_err("unknown flag '%c'\n", *str);
+ return -EINVAL;
+ }
+ }
+ vpr_info("flags=0x%x\n", modifiers->flags);
+ return 0;
+}
+
/*
* Parse `str' as a flags specification, format [-+=][p]+.
* Sets up *maskp and *flagsp to be used when changing the
@@ -438,7 +458,7 @@ static int ddebug_parse_query(char *words[], int nwords,
*/
static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
{
- int op, i;
+ int op;
switch (*str) {
case '+':
@@ -452,19 +472,8 @@ static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
}
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) {
- modifiers->flags |= opt_array[i].flag;
- break;
- }
- }
- if (i < 0) {
- pr_err("unknown flag '%c'\n", *str);
- return -EINVAL;
- }
- }
- vpr_info("flags=0x%x\n", modifiers->flags);
+ if (ddebug_read_flags(str, modifiers))
+ return -EINVAL;
/* calculate final flags, mask based upon op */
switch (op) {
--
2.26.2
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.
Signed-off-by: Jim Cromie <[email protected]>
---
lib/dynamic_debug.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index e1dd96178f18..ff97938b5849 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -547,6 +547,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
return exitcode;
return nfound;
}
+EXPORT_SYMBOL(ddebug_exec_queries);
#define PREFIX_SIZE 64
--
2.26.2
On 17/06/2020 18.25, Jim Cromie wrote:
> this is v3, changes from previous:
> - moved non-controversial commits up front, refactors to help this.
> - a few more minor cleanups
> - left out the WIP patches
> - export ddebug_exec_queries()
> - accept file=foo:func only, not module:foo
> - varname changes
>
> v2: https://lore.kernel.org/lkml/[email protected]/
> v1: https://lore.kernel.org/lkml/[email protected]/
>
> Patchset starts with 11 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. etc..
So I haven't been following too closely, but I'm also a bit skeptical
about the new custom flag thing. OTOH, I'd really like to see those
first cleanups go in soon, especially patch 6 - which not only makes the
ram use a bit more accurate, it also avoids ~10000 calls of strlen() on
cache-cold memory during boot.
So, FWIW, you have my Acked-by for patches 1 through 11, and I hope
those can be picked up before the next merge window. But the remaining
ones seem to still require some discussion.
Rasmus
On Wed, 2020-06-17 at 10:25 -0600, 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 & filter flags
>
> The 'u' flag lets the user assemble an arbitary set of callsites.
> Then using filter flags, user can activate the 'u' callsite set.
>
> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> #> echo 'u+p' > control
>
> Of course, you can continue to just activate your set without ever
> marking it 1st, but you could trivially add the markup as you go, then
> be able to use it as a constraint later, to undo or modify your set.
Does this set selection also allow for selection by
increasing decimal level?
Can sites be enabled for a value less than x?
On Wed, Jun 17, 2020 at 4:13 PM Joe Perches <[email protected]> wrote:
>
> On Wed, 2020-06-17 at 10:25 -0600, 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 & filter flags
> >
> > The 'u' flag lets the user assemble an arbitary set of callsites.
> > Then using filter flags, user can activate the 'u' callsite set.
> >
> > #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> > #> echo 'u+p' > control
> >
> > Of course, you can continue to just activate your set without ever
> > marking it 1st, but you could trivially add the markup as you go, then
> > be able to use it as a constraint later, to undo or modify your set.
>
> Does this set selection also allow for selection by
> increasing decimal level?
>
> Can sites be enabled for a value less than x?
>
>
no, theres no levels added here.
that would be a variation on the WIP pr-class patch in v2, dropped from v3
legacy dyndbg - select on RO callsite info only - file, module, func, line.
flag state is write only.
filterflags - additionally select on callsite's flagstate, ie
reading the current flagstate
please look at the export patch 15/21
for how I now think levels, and drm.debug=0x03 can be done.
On Wed 2020-06-17 10:25:34, Jim Cromie wrote:
> Change ddebug_parse_flags to accept optional filterflags before the
> required operator [-+=]. Read the flags into the filter_flags
> parameter added in the previous patch. So this now supplies the
> filterflags to ddebug_exec_query.
>
> filterflags work like query terms, they constrain what callsites get
> matched before theyre modified. So like a query, they can be empty.
>
> Filterflags let you read callsite's flagstate, including results of
> previous modifications, and require that certain flags are set, before
> modifying the callsite further.
>
> So you can build up sets of callsites by marking them with a
> particular flagstate, for example 'fmlt', then enable that set in a
> batch.
>
> echo fmlt+p >control
>
> Naturally you can use almost any combo of flags you want for marking,
> and can mark several different sets with different patterns. And then
> you can activate them in a bunch:
>
> echo 'ft+p; mt+p; lt+p;' >control
>
> + * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
This interface is simply _horrible_ and I do not see a point in this feature!!!
I as a normal dynamic debug user am interested into:
+ enabling/disabling messages from a given module/file/line/function
+ list of available modules/files/lines/functions
+ list of enabled modules/files/lines/functions
I do not understand why I would ever want to do something like:
+ enable messages that print module name and line number
+ disable message that does not print a module name
In fact, IMHO, all the 'flmt' flags were a wrong idea and nobody
really needed them. This information in not needed by other
printk() messages. Why pr_debug() would need them?
They just made the code and interface complicated.
Please, stop all this non-sense!!!
Best Regards,
Petr
On Wed 2020-06-17 10:25:35, 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 & filter flags
>
> The 'u' flag lets the user assemble an arbitary set of callsites.
> Then using filter flags, user can activate the 'u' callsite set.
>
> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> #> echo 'u+p' > control
>
> Of course, you can continue to just activate your set without ever
> marking it 1st, but you could trivially add the markup as you go, then
> be able to use it as a constraint later, to undo or modify your set.
>
> #> echo 'file foo.c +up' >control
> .. monitor, debug, finish ..
> #> echo 'u-p' >control
>
> # then later resume
> #> echo 'u+p' >control
>
> # disable some cluttering messages, and remove from u-set
> #> echo 'file noisy.c function:jabber_* u-pu' >control
>
> # for doc, recollection
> grep =pu control > my-favorite-callsites
>
> Note:
>
> Your flagstate after boot is generally not all =_. -DDEBUG will arm
> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> enable them early, and $module.dyndbg=+p bootargs will arm them when
> the module is loaded. But you could manage them with u-flags:
>
> #> echo '-t' >control # clear t-flag to use it as 2ndary markup
> #> echo 'p+ut' >control # mark the boot-enabled set of callsites
> #> echo '-p' >control # clean your dmesg -w stream
>
> ... monitor, debug ..
> #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
> #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
Does anyone requested this feature, please?
For me, it is really hard to imagine people using these complex and hacky
steps.
Not to say that using t-flag as a markup looks like a real hack.
People either always need the line number in the kernel log or
they do not need it at all.
Let me repeat. Please, stop this non-sense.
Best Regards,
Petr
On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
> On Wed 2020-06-17 10:25:35, 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 & filter flags
> >
> > The 'u' flag lets the user assemble an arbitary set of callsites.
> > Then using filter flags, user can activate the 'u' callsite set.
> >
> > #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> > #> echo 'u+p' > control
> >
> > Of course, you can continue to just activate your set without ever
> > marking it 1st, but you could trivially add the markup as you go, then
> > be able to use it as a constraint later, to undo or modify your set.
> >
> > #> echo 'file foo.c +up' >control
> > .. monitor, debug, finish ..
> > #> echo 'u-p' >control
> >
> > # then later resume
> > #> echo 'u+p' >control
> >
> > # disable some cluttering messages, and remove from u-set
> > #> echo 'file noisy.c function:jabber_* u-pu' >control
> >
> > # for doc, recollection
> > grep =pu control > my-favorite-callsites
> >
> > Note:
> >
> > Your flagstate after boot is generally not all =_. -DDEBUG will arm
> > compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> > enable them early, and $module.dyndbg=+p bootargs will arm them when
> > the module is loaded. But you could manage them with u-flags:
> >
> > #> echo '-t' >control # clear t-flag to use it as 2ndary markup
> > #> echo 'p+ut' >control # mark the boot-enabled set of callsites
> > #> echo '-p' >control # clean your dmesg -w stream
> >
> > ... monitor, debug ..
> > #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
> > #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
>
> Does anyone requested this feature, please?
>
> For me, it is really hard to imagine people using these complex and hacky
> steps.
I think that all this is motivated by adding support for module
specific groups.
What about storing the group as yet another information for each
message? I mean the same way as we store module name, file, line,
function name.
Then we could add API to define group for a given message:
pr_debug_group(group_id, fmt, ...);
the interface for the control file might be via new keyword "group".
You could then do something like:
echo module=drm group=0x3 +p >control
But more importantly you should add functions that might be called
when the drm.debug parameter is changes. I have already mentioned
it is another reply:
dd_enable_module_group(module_name, group_id);
dd_disable_module_group(module_name, group_id);
It will _not_ need any new flag or flag filtering.
Best Regards,
Petr
On 6/18/20 1:40 PM, Petr Mladek wrote:
> On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
>> On Wed 2020-06-17 10:25:35, 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 & filter flags
>>>
>>> The 'u' flag lets the user assemble an arbitary set of callsites.
>>> Then using filter flags, user can activate the 'u' callsite set.
>>>
>>> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
>>> #> echo 'u+p' > control
>>>
>>> Of course, you can continue to just activate your set without ever
>>> marking it 1st, but you could trivially add the markup as you go, then
>>> be able to use it as a constraint later, to undo or modify your set.
>>>
>>> #> echo 'file foo.c +up' >control
>>> .. monitor, debug, finish ..
>>> #> echo 'u-p' >control
>>>
>>> # then later resume
>>> #> echo 'u+p' >control
>>>
>>> # disable some cluttering messages, and remove from u-set
>>> #> echo 'file noisy.c function:jabber_* u-pu' >control
>>>
>>> # for doc, recollection
>>> grep =pu control > my-favorite-callsites
>>>
>>> Note:
>>>
>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
>>> enable them early, and $module.dyndbg=+p bootargs will arm them when
>>> the module is loaded. But you could manage them with u-flags:
>>>
>>> #> echo '-t' >control # clear t-flag to use it as 2ndary markup
>>> #> echo 'p+ut' >control # mark the boot-enabled set of callsites
>>> #> echo '-p' >control # clean your dmesg -w stream
>>>
>>> ... monitor, debug ..
>>> #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
>>> #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
>>
>> Does anyone requested this feature, please?
>>
>> For me, it is really hard to imagine people using these complex and hacky
>> steps.
>
> I think that all this is motivated by adding support for module
> specific groups.
>
> What about storing the group as yet another information for each
> message? I mean the same way as we store module name, file, line,
> function name.
>
> Then we could add API to define group for a given message:
>
> pr_debug_group(group_id, fmt, ...);
>
> the interface for the control file might be via new keyword "group".
> You could then do something like:
>
> echo module=drm group=0x3 +p >control
>
> But more importantly you should add functions that might be called
> when the drm.debug parameter is changes. I have already mentioned
> it is another reply:
>
> dd_enable_module_group(module_name, group_id);
> dd_disable_module_group(module_name, group_id);
>
>
> It will _not_ need any new flag or flag filtering.
>
> Best Regards,
> Petr
>
Yes, I'm wondering as well if people are really going to use the
new flags and filter flags - I mentioned that here:
https://lkml.org/lkml/2020/6/12/732
The grouping stuff is already being used by lots of modules so
that seems useful.
Thanks,
-Jason
On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
>
>
>
> On 6/18/20 1:40 PM, Petr Mladek wrote:
> > On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
> >> On Wed 2020-06-17 10:25:35, 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 & filter flags
> >>>
> >>> The 'u' flag lets the user assemble an arbitary set of callsites.
> >>> Then using filter flags, user can activate the 'u' callsite set.
> >>>
> >>> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> >>> #> echo 'u+p' > control
> >>>
> >>> Of course, you can continue to just activate your set without ever
> >>> marking it 1st, but you could trivially add the markup as you go, then
> >>> be able to use it as a constraint later, to undo or modify your set.
> >>>
> >>> #> echo 'file foo.c +up' >control
> >>> .. monitor, debug, finish ..
> >>> #> echo 'u-p' >control
> >>>
> >>> # then later resume
> >>> #> echo 'u+p' >control
> >>>
> >>> # disable some cluttering messages, and remove from u-set
> >>> #> echo 'file noisy.c function:jabber_* u-pu' >control
> >>>
> >>> # for doc, recollection
> >>> grep =pu control > my-favorite-callsites
> >>>
> >>> Note:
> >>>
> >>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
> >>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> >>> enable them early, and $module.dyndbg=+p bootargs will arm them when
> >>> the module is loaded. But you could manage them with u-flags:
> >>>
> >>> #> echo '-t' >control # clear t-flag to use it as 2ndary markup
> >>> #> echo 'p+ut' >control # mark the boot-enabled set of callsites
> >>> #> echo '-p' >control # clean your dmesg -w stream
> >>>
> >>> ... monitor, debug ..
> >>> #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
> >>> #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
> >>
> >> Does anyone requested this feature, please?
> >>
> >> For me, it is really hard to imagine people using these complex and hacky
> >> steps.
> >
> > I think that all this is motivated by adding support for module
> > specific groups.
> >
> > What about storing the group as yet another information for each
> > message? I mean the same way as we store module name, file, line,
> > function name.
> >
> > Then we could add API to define group for a given message:
> >
> > pr_debug_group(group_id, fmt, ...);
> >
> > the interface for the control file might be via new keyword "group".
> > You could then do something like:
> >
> > echo module=drm group=0x3 +p >control
> >
> > But more importantly you should add functions that might be called
> > when the drm.debug parameter is changes. I have already mentioned
> > it is another reply:
> >
> > dd_enable_module_group(module_name, group_id);
> > dd_disable_module_group(module_name, group_id);
> >
> >
> > It will _not_ need any new flag or flag filtering.
> >
> > Best Regards,
> > Petr
> >
>
> Yes, I'm wondering as well if people are really going to use the
> new flags and filter flags - I mentioned that here:
> https://lkml.org/lkml/2020/6/12/732
>
yes, I saw, and replied there.
but since that was v1, and we're on v3, we should refresh.
the central use-case is above, 1-liner version summarized here:
1- enable sites as you chase a problem with +up
2- examine them with grep =pu
3- change the set to suit, either by adding or subtracting callsites.
4- continue debugging, and changing callsites to suit
5- grep =pu control > ~/debugging-session-task1-callsites
6- echo up-p >control # disable for now, leave u-set for later
7- do other stuff
8 echo uP+p >control # reactivate useful debug-state and resume
> The grouping stuff is already being used by lots of modules so
> that seems useful.
I now dont see the need.
given N debug callsites, any group can be defined by <N queries,
probably a lot less
if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
then they can act (+p or -p) on those sets defined by <N queries.
and now any callsite can be in any number of groups, not just one.
It would be prudent to evaluate such groupings case by case,
because the intersecting callsites are subject to "last manipulator wins"
but its unnecessary to insist that all sets are disjoint.
Unlike pr_debug_n, however its spelled.
>
> Thanks,
>
> -Jason
On 6/18/20 3:11 PM, [email protected] wrote:
> On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
>>
>>
>>
>> On 6/18/20 1:40 PM, Petr Mladek wrote:
>>> On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
>>>> On Wed 2020-06-17 10:25:35, 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 & filter flags
>>>>>
>>>>> The 'u' flag lets the user assemble an arbitary set of callsites.
>>>>> Then using filter flags, user can activate the 'u' callsite set.
>>>>>
>>>>> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
>>>>> #> echo 'u+p' > control
>>>>>
>>>>> Of course, you can continue to just activate your set without ever
>>>>> marking it 1st, but you could trivially add the markup as you go, then
>>>>> be able to use it as a constraint later, to undo or modify your set.
>>>>>
>>>>> #> echo 'file foo.c +up' >control
>>>>> .. monitor, debug, finish ..
>>>>> #> echo 'u-p' >control
>>>>>
>>>>> # then later resume
>>>>> #> echo 'u+p' >control
>>>>>
>>>>> # disable some cluttering messages, and remove from u-set
>>>>> #> echo 'file noisy.c function:jabber_* u-pu' >control
>>>>>
>>>>> # for doc, recollection
>>>>> grep =pu control > my-favorite-callsites
>>>>>
>>>>> Note:
>>>>>
>>>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
>>>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
>>>>> enable them early, and $module.dyndbg=+p bootargs will arm them when
>>>>> the module is loaded. But you could manage them with u-flags:
>>>>>
>>>>> #> echo '-t' >control # clear t-flag to use it as 2ndary markup
>>>>> #> echo 'p+ut' >control # mark the boot-enabled set of callsites
>>>>> #> echo '-p' >control # clean your dmesg -w stream
>>>>>
>>>>> ... monitor, debug ..
>>>>> #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
>>>>> #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
>>>>
>>>> Does anyone requested this feature, please?
>>>>
>>>> For me, it is really hard to imagine people using these complex and hacky
>>>> steps.
>>>
>>> I think that all this is motivated by adding support for module
>>> specific groups.
>>>
>>> What about storing the group as yet another information for each
>>> message? I mean the same way as we store module name, file, line,
>>> function name.
>>>
>>> Then we could add API to define group for a given message:
>>>
>>> pr_debug_group(group_id, fmt, ...);
>>>
>>> the interface for the control file might be via new keyword "group".
>>> You could then do something like:
>>>
>>> echo module=drm group=0x3 +p >control
>>>
>>> But more importantly you should add functions that might be called
>>> when the drm.debug parameter is changes. I have already mentioned
>>> it is another reply:
>>>
>>> dd_enable_module_group(module_name, group_id);
>>> dd_disable_module_group(module_name, group_id);
>>>
>>>
>>> It will _not_ need any new flag or flag filtering.
>>>
>>> Best Regards,
>>> Petr
>>>
>>
>> Yes, I'm wondering as well if people are really going to use the
>> new flags and filter flags - I mentioned that here:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_6_12_732&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=vltk6sSzPDQIqO4gGkJeDY6jcEarG4xTztab2EHtPFY&s=6x1EHNoRxebA99Tu-C2i0s5dmdzyEF9bXVcv_cYoM_I&e=
>>
>
> yes, I saw, and replied there.
> but since that was v1, and we're on v3, we should refresh.
>
> the central use-case is above, 1-liner version summarized here:
>
> 1- enable sites as you chase a problem with +up
> 2- examine them with grep =pu
> 3- change the set to suit, either by adding or subtracting callsites.
> 4- continue debugging, and changing callsites to suit
> 5- grep =pu control > ~/debugging-session-task1-callsites
> 6- echo up-p >control # disable for now, leave u-set for later
> 7- do other stuff
> 8 echo uP+p >control # reactivate useful debug-state and resume
>
>
>> The grouping stuff is already being used by lots of modules so
>> that seems useful.
>
> I now dont see the need.
>
> given N debug callsites, any group can be defined by <N queries,
> probably a lot less
> if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
> then they can act (+p or -p) on those sets defined by <N queries.
>
> and now any callsite can be in any number of groups, not just one.
> It would be prudent to evaluate such groupings case by case,
> because the intersecting callsites are subject to "last manipulator wins"
> but its unnecessary to insist that all sets are disjoint.
> Unlike pr_debug_n, however its spelled.
>
hmm - so I think you are saying there is then no need to change the
calling functions themselves - its still 'pr_debug()'. You could even
use the 'format' qualifier for example to implement your groups that
way.
For example:
pr_debug("failure type1: blah");
pr_debug("failure type2: blah blah");
and then do: ddebug_exec_queries("format type1 +p", module);
I would be curious to see what Stanimir thinks of this proposal
and whether it would work for his venus driver, which is what
prompted this module group discussion.
Thanks,
-Jason
On Thu, Jun 18, 2020 at 6:44 AM Petr Mladek <[email protected]> wrote:
>
> On Wed 2020-06-17 10:25:34, Jim Cromie wrote:
> > Change ddebug_parse_flags to accept optional filterflags before the
> > required operator [-+=]. Read the flags into the filter_flags
> > parameter added in the previous patch. So this now supplies the
> > filterflags to ddebug_exec_query.
> >
> > filterflags work like query terms, they constrain what callsites get
> > matched before theyre modified. So like a query, they can be empty.
> >
> > Filterflags let you read callsite's flagstate, including results of
> > previous modifications, and require that certain flags are set, before
> > modifying the callsite further.
> >
> > So you can build up sets of callsites by marking them with a
> > particular flagstate, for example 'fmlt', then enable that set in a
> > batch.
> >
> > echo fmlt+p >control
> >
> > Naturally you can use almost any combo of flags you want for marking,
> > and can mark several different sets with different patterns. And then
> > you can activate them in a bunch:
> >
> > echo 'ft+p; mt+p; lt+p;' >control
> >
> > + * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
>
> This interface is simply _horrible_ and I do not see a point in this feature!!!
>
> I as a normal dynamic debug user am interested into:
>
> + enabling/disabling messages from a given module/file/line/function
> + list of available modules/files/lines/functions
> + list of enabled modules/files/lines/functions
>
> I do not understand why I would ever want to do something like:
>
> + enable messages that print module name and line number
> + disable message that does not print a module name
messages dont print them, the flags do, according to USER CHOICE.
a developer who is deeply familiar with the code doesnt need to
see most of it in the logs, average user might need them to comprehend things.
>
> In fact, IMHO, all the 'flmt' flags were a wrong idea and nobody
> really needed them. This information in not needed by other
> printk() messages. Why pr_debug() would need them?
> They just made the code and interface complicated.
>
it looks like they landed fully formed in lib/dynamic_debug.c
probably because that was a unification of several different print
debug systems.
you are free to set them globally:
echo +fmlt >control
or just the ones youre using
echo up+fmlt >control
> Please, stop all this non-sense!!!
>
> Best Regards,
> Petr
On Thu 2020-06-18 08:54:58, [email protected] wrote:
> On Thu, Jun 18, 2020 at 6:44 AM Petr Mladek <[email protected]> wrote:
> >
> > On Wed 2020-06-17 10:25:34, Jim Cromie wrote:
> > > Change ddebug_parse_flags to accept optional filterflags before the
> > > required operator [-+=]. Read the flags into the filter_flags
> > > parameter added in the previous patch. So this now supplies the
> > > filterflags to ddebug_exec_query.
> > >
> > > filterflags work like query terms, they constrain what callsites get
> > > matched before theyre modified. So like a query, they can be empty.
> > >
> > > Filterflags let you read callsite's flagstate, including results of
> > > previous modifications, and require that certain flags are set, before
> > > modifying the callsite further.
> > >
> > > So you can build up sets of callsites by marking them with a
> > > particular flagstate, for example 'fmlt', then enable that set in a
> > > batch.
> > >
> > > echo fmlt+p >control
> > >
> > > Naturally you can use almost any combo of flags you want for marking,
> > > and can mark several different sets with different patterns. And then
> > > you can activate them in a bunch:
> > >
> > > echo 'ft+p; mt+p; lt+p;' >control
> > >
> > > + * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
> >
> > This interface is simply _horrible_ and I do not see a point in this feature!!!
> >
> > I as a normal dynamic debug user am interested into:
> >
> > + enabling/disabling messages from a given module/file/line/function
> > + list of available modules/files/lines/functions
> > + list of enabled modules/files/lines/functions
> >
> > I do not understand why I would ever want to do something like:
> >
> > + enable messages that print module name and line number
> > + disable message that does not print a module name
>
> messages dont print them, the flags do, according to USER CHOICE.
> a developer who is deeply familiar with the code doesnt need to
> see most of it in the logs, average user might need them to comprehend things.
Any user, even average, has to deal also with non-debug messages that
do not include this extra information. Why pr_debug() message would
need it?
Message should be useful on its own. The location can be found by
grepping like for any other printk() messages.
Yes, the information might be handy. But all these configuration
choices also make the interface and code complicated. IMHO, it has
been over engineered. And this patch makes it even worse.
Anyway, you answered why the flags are there. But you did not explain
why anyone would need to use a filter based on them. Answer this,
please!!!
> > In fact, IMHO, all the 'flmt' flags were a wrong idea and nobody
> > really needed them. This information in not needed by other
> > printk() messages. Why pr_debug() would need them?
> > They just made the code and interface complicated.
> >
>
> it looks like they landed fully formed in lib/dynamic_debug.c
> probably because that was a unification of several different print
> debug systems.
No, they were added by the commit 8ba6ebf583f12da32036fc0 ("Dynamic debug:
Add more flags").
There is no explanation why they were added. It probably just looked
like a good idea to the author and nobody complained at that time.
It has been included wihtout any comment, see
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
> you are free to set them globally:
> echo +fmlt >control
>
> or just the ones youre using
> echo up+fmlt >control
The question is not if I could do so. The question is how many users
do it or need to do so.
Features in this patchset affect the interface with userspace. It
means that they would need to be maintained "forewer". For this,
you need to prove that it is widely useful. Ideally, it should
be outcome of some discussion where people missed this.
I do not see any reasonable usecase for anything like:
echo 'ft+p; mt+p; lt+p;' >control
Why people would do this, please?
Best Regards,
Petr
On Thu, Jun 18, 2020 at 10:19 AM Petr Mladek <[email protected]> wrote:
>
> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
OK.
Please tell me how this chunk of prose fails to explain a use case for
the u-flag
we can differ on how useful it looks.
if u-flag is useful, then filtering on flags is also needed,
to use the flag to enable/disable an arbitrary set of callsites
all the other "flag abuse" you disliked in last patch is avoidable,
unless 2 people are chasing 2 separate problems,
and need to keep their sets distinct
> > Why ?
> >
> > The u-flag & filter flags
> >
> > The 'u' flag lets the user assemble an arbitary set of callsites.
> > Then using filter flags, user can activate the 'u' callsite set.
> >
> > #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
> > #> echo 'u+p' > control
> >
> > Of course, you can continue to just activate your set without ever
> > marking it 1st, but you could trivially add the markup as you go, then
> > be able to use it as a constraint later, to undo or modify your set.
> >
> > #> echo 'file foo.c +up' >control
> > .. monitor, debug, finish ..
> > #> echo 'u-p' >control
> >
> > # then later resume
> > #> echo 'u+p' >control
> >
> > # disable some cluttering messages, and remove from u-set
> > #> echo 'file noisy.c function:jabber_* u-pu' >control
> >
> > # for doc, recollection
> > grep =pu control > my-favorite-callsites
> >
> > Note:
> >
> > Your flagstate after boot is generally not all =_. -DDEBUG will arm
> > compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> > enable them early, and $module.dyndbg=+p bootargs will arm them when
> > the module is loaded. But you could manage them with u-flags:
> >
> > #> echo '-t' >control # clear t-flag to use it as 2ndary markup
> > #> echo 'p+ut' >control # mark the boot-enabled set of callsites
> > #> echo '-p' >control # clean your dmesg -w stream
> >
> > ... monitor, debug ..
> > #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
> > #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
>
> Does anyone requested this feature, please?
>
> For me, it is really hard to imagine people using these complex and hacky
> steps.
>
> Not to say that using t-flag as a markup looks like a real hack.
> People either always need the line number in the kernel log or
> they do not need it at all.
>
> Let me repeat. Please, stop this non-sense.
>
> Best Regards,
> Petr
oops. got 3 copies of 14/21, this is the good one. with module=foo
AND file=bar
On Wed, Jun 17, 2020 at 10:26 AM Jim Cromie <[email protected]> wrote:
>
> Current code expects "keyword" "arg" as 2 space separated words.
> Change to also accept "keyword=arg" form as well, and drop !(nwords%2)
> requirement.
>
> Then in rest of function, use new keyword, arg variables instead of
> word[i], word[i+1]
>
> Signed-off-by: Jim Cromie <[email protected]>
> ---
> .../admin-guide/dynamic-debug-howto.rst | 1 +
> lib/dynamic_debug.c | 51 ++++++++++++-------
> 2 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 6c04aea8f4cd..e5a8def45f3f 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -156,6 +156,7 @@ against. Possible keywords are:::
> ``line-range`` cannot contain space, e.g.
> "1-30" is valid range but "1 - 30" is not.
>
> + ``module=foo`` combined keyword=value form is interchangably accepted
>
> The meanings of each keyword are:
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 7eb963b1bd11..e1dd96178f18 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -351,6 +351,8 @@ static int check_set(const char **dest, char *src, char *name)
> * line <lineno>
> * line <first-lineno>-<last-lineno> // where either may be empty
> *
> + * Also accept combined keyword=value and keyword:value forms
> + *
> * Only 1 of each type is allowed.
> * Returns 0 on success, <0 on error.
> */
> @@ -360,22 +362,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 = strchr(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 */
> @@ -391,18 +404,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
>
Hi Jason, Jim,
On 6/18/20 10:40 PM, Jason Baron wrote:
>
>
> On 6/18/20 3:11 PM, [email protected] wrote:
>> On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
>>>
>>>
>>>
>>> On 6/18/20 1:40 PM, Petr Mladek wrote:
>>>> On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
>>>>> On Wed 2020-06-17 10:25:35, 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 & filter flags
>>>>>>
>>>>>> The 'u' flag lets the user assemble an arbitary set of callsites.
>>>>>> Then using filter flags, user can activate the 'u' callsite set.
>>>>>>
>>>>>> #> echo 'file foo.c +u; file bar.c +u' > control # and repeat
>>>>>> #> echo 'u+p' > control
>>>>>>
>>>>>> Of course, you can continue to just activate your set without ever
>>>>>> marking it 1st, but you could trivially add the markup as you go, then
>>>>>> be able to use it as a constraint later, to undo or modify your set.
>>>>>>
>>>>>> #> echo 'file foo.c +up' >control
>>>>>> .. monitor, debug, finish ..
>>>>>> #> echo 'u-p' >control
>>>>>>
>>>>>> # then later resume
>>>>>> #> echo 'u+p' >control
>>>>>>
>>>>>> # disable some cluttering messages, and remove from u-set
>>>>>> #> echo 'file noisy.c function:jabber_* u-pu' >control
>>>>>>
>>>>>> # for doc, recollection
>>>>>> grep =pu control > my-favorite-callsites
>>>>>>
>>>>>> Note:
>>>>>>
>>>>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
>>>>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
>>>>>> enable them early, and $module.dyndbg=+p bootargs will arm them when
>>>>>> the module is loaded. But you could manage them with u-flags:
>>>>>>
>>>>>> #> echo '-t' >control # clear t-flag to use it as 2ndary markup
>>>>>> #> echo 'p+ut' >control # mark the boot-enabled set of callsites
>>>>>> #> echo '-p' >control # clean your dmesg -w stream
>>>>>>
>>>>>> ... monitor, debug ..
>>>>>> #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
>>>>>> #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
>>>>>
>>>>> Does anyone requested this feature, please?
>>>>>
>>>>> For me, it is really hard to imagine people using these complex and hacky
>>>>> steps.
>>>>
>>>> I think that all this is motivated by adding support for module
>>>> specific groups.
>>>>
>>>> What about storing the group as yet another information for each
>>>> message? I mean the same way as we store module name, file, line,
>>>> function name.
>>>>
>>>> Then we could add API to define group for a given message:
>>>>
>>>> pr_debug_group(group_id, fmt, ...);
>>>>
>>>> the interface for the control file might be via new keyword "group".
>>>> You could then do something like:
>>>>
>>>> echo module=drm group=0x3 +p >control
>>>>
>>>> But more importantly you should add functions that might be called
>>>> when the drm.debug parameter is changes. I have already mentioned
>>>> it is another reply:
>>>>
>>>> dd_enable_module_group(module_name, group_id);
>>>> dd_disable_module_group(module_name, group_id);
>>>>
>>>>
>>>> It will _not_ need any new flag or flag filtering.
>>>>
>>>> Best Regards,
>>>> Petr
>>>>
>>>
>>> Yes, I'm wondering as well if people are really going to use the
>>> new flags and filter flags - I mentioned that here:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_6_12_732&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=vltk6sSzPDQIqO4gGkJeDY6jcEarG4xTztab2EHtPFY&s=6x1EHNoRxebA99Tu-C2i0s5dmdzyEF9bXVcv_cYoM_I&e=
>>>
>>
>> yes, I saw, and replied there.
>> but since that was v1, and we're on v3, we should refresh.
>>
>> the central use-case is above, 1-liner version summarized here:
>>
>> 1- enable sites as you chase a problem with +up
>> 2- examine them with grep =pu
>> 3- change the set to suit, either by adding or subtracting callsites.
>> 4- continue debugging, and changing callsites to suit
>> 5- grep =pu control > ~/debugging-session-task1-callsites
>> 6- echo up-p >control # disable for now, leave u-set for later
>> 7- do other stuff
>> 8 echo uP+p >control # reactivate useful debug-state and resume
>>
>>
>>> The grouping stuff is already being used by lots of modules so
>>> that seems useful.
>>
>> I now dont see the need.
>>
>> given N debug callsites, any group can be defined by <N queries,
>> probably a lot less
>> if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
>> then they can act (+p or -p) on those sets defined by <N queries.
>>
>> and now any callsite can be in any number of groups, not just one.
>> It would be prudent to evaluate such groupings case by case,
>> because the intersecting callsites are subject to "last manipulator wins"
>> but its unnecessary to insist that all sets are disjoint.
>> Unlike pr_debug_n, however its spelled.
>>
>
> hmm - so I think you are saying there is then no need to change the
> calling functions themselves - its still 'pr_debug()'. You could even
> use the 'format' qualifier for example to implement your groups that
> way.
>
> For example:
>
> pr_debug("failure type1: blah");
> pr_debug("failure type2: blah blah");
>
> and then do: ddebug_exec_queries("format type1 +p", module);
>
> I would be curious to see what Stanimir thinks of this proposal
> and whether it would work for his venus driver, which is what
> prompted this module group discussion.
Hmm, we spin in a circle :)
Infact this was my first way of implementing the groups in Venus driver,
you can see it at [1].
+#define VDBGL(fmt, args...) pr_debug("VENUSL: " fmt, ##args)
+#define VDBGM(fmt, args...) pr_debug("VENUSM: " fmt, ##args)
+#define VDBGH(fmt, args...) pr_debug("VENUSH: " fmt, ##args)
+#define VDBGFW(fmt, args...) pr_debug("VENUSFW: " fmt, ##args)
[1] https://lkml.org/lkml/2020/5/21/668
--
regards,
Stan
On Thu, Jun 18, 2020 at 1:40 PM Jason Baron <[email protected]> wrote:
>
>
>
> On 6/18/20 3:11 PM, [email protected] wrote:
> > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
> >>
> >
> >> The grouping stuff is already being used by lots of modules so
> >> that seems useful.
> >
> > I now dont see the need.
> >
> > given N debug callsites, any group can be defined by <N queries,
> > probably a lot less
> > if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
> > then they can act (+p or -p) on those sets defined by <N queries.
> >
> > and now any callsite can be in any number of groups, not just one.
> > It would be prudent to evaluate such groupings case by case,
> > because the intersecting callsites are subject to "last manipulator wins"
> > but its unnecessary to insist that all sets are disjoint.
> > Unlike pr_debug_n, however its spelled.
> >
>
> hmm - so I think you are saying there is then no need to change the
> calling functions themselves - its still 'pr_debug()'. You could even
> use the 'format' qualifier for example to implement your groups that
> way.
>
> For example:
>
> pr_debug("failure type1: blah");
> pr_debug("failure type2: blah blah");
>
> and then do: ddebug_exec_queries("format type1 +p", module);
Exactly
and using format, which always have user relevant info,
and often some severity indication (forex warn info err)
are a workable classification scheme already in use at least informally
So Id expect that this classification can often be done in 1 query.
define the set of callsites in 1 query-string, add +p or -p to it, and
manipulate away.
Amplifying,
this is the only user interface of consequence in dyndbg.
/sys/.../verbose doesnt count
Letting module authors use it is the full-featured way,
everything else is crap (movie reference)
and would require far more maintenance
>
> I would be curious to see what Stanimir thinks of this proposal
> and whether it would work for his venus driver, which is what
> prompted this module group discussion.
>
Indeed.
Id also like to hear from drm folks
./drm/amd/display/include/logger_types.h:#define DC_LOG_SURFACE(...)
pr_debug("[SURFACE]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_HW_LINK_TRAINING(...)
pr_debug("[HW_LINK_TRAINING]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_HW_AUDIO(...)
pr_debug("[HW_AUDIO]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_SCALER(...)
pr_debug("[SCALER]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_BIOS(...)
pr_debug("[BIOS]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_BANDWIDTH_CALCS(...) pr_debug("[BANDWIDTH_CALCS]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_DML(...)
pr_debug("[DML]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_IF_TRACE(...)
pr_debug("[IF_TRACE]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_GAMMA(...)
pr_debug("[GAMMA]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_ALL_GAMMA(...)
pr_debug("[GAMMA]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_ALL_TF_CHANNELS(...) pr_debug("[GAMMA]:"__VA_ARGS__)
those defines suggest that they are already doing this with existing formats
with the export,
they can implement this group control using dyndbg with little effort.
including the tie-in to the __debug var if thats useful
and of course, user can add or subtract from that set ad-hoc.
> Thanks,
>
> -Jason
thanks
jimc
On Thu, Jun 18, 2020 at 4:34 PM Stanimir Varbanov
<[email protected]> wrote:
>
> Hi Jason, Jim,
>
> > I would be curious to see what Stanimir thinks of this proposal
> > and whether it would work for his venus driver, which is what
> > prompted this module group discussion.
>
> Hmm, we spin in a circle :)
>
> Infact this was my first way of implementing the groups in Venus driver,
> you can see it at [1].
>
> +#define VDBGL(fmt, args...) pr_debug("VENUSL: " fmt, ##args)
> +#define VDBGM(fmt, args...) pr_debug("VENUSM: " fmt, ##args)
> +#define VDBGH(fmt, args...) pr_debug("VENUSH: " fmt, ##args)
> +#define VDBGFW(fmt, args...) pr_debug("VENUSFW: " fmt, ##args)
>
I recall :-)
I think Greg K-Hs distaste for those defines was for using them,
as it tosses the utility of grep pr_debug
pr_debug("VENUSM:"
is barely longer than
VDBGM
with ddebug_exec_queries, you can leverage the existing format.
>
> [1] https://lkml.org/lkml/2020/5/21/668
>
> --
> regards,
> Stan
thanks
Jim
On Fri 2020-06-19 09:45:55, Petr Mladek wrote:
> On Thu 2020-06-18 13:11:05, [email protected] wrote:
> > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
> > > Yes, I'm wondering as well if people are really going to use the
> > > new flags and filter flags - I mentioned that here:
> > > https://lkml.org/lkml/2020/6/12/732
> >
> > yes, I saw, and replied there.
>
> No, the repply only explains how the interface might be used. There is
> no prove that people would actually use it.
>
> > but since that was v1, and we're on v3, we should refresh.
> >
> > the central use-case is above, 1-liner version summarized here:
> >
> > 1- enable sites as you chase a problem with +up
> > 2- examine them with grep =pu
> > 3- change the set to suit, either by adding or subtracting callsites.
> > 4- continue debugging, and changing callsites to suit
> > 5- grep =pu control > ~/debugging-session-task1-callsites
> > 6- echo up-p >control # disable for now, leave u-set for later
> > 7- do other stuff
> > 8 echo uP+p >control # reactivate useful debug-state and resume
>
> In short, this feature allows repeatedly enable/disable some
> slowly growing maze of debug messages. Who need this, please? !!!
>
> If I am debugging then I add/remove debug messages. But I never
> enable/disable all of them repeatedly.
Not to say that I usually need to reboot when I reproduce the problem
and before I could try it again. So all dyndbg flags gets lost
between two tests anyway.
Best Regards,
Petr
On Fri, Jun 19, 2020 at 10:10:24AM +0200, Petr Mladek wrote:
> On Fri 2020-06-19 09:45:55, Petr Mladek wrote:
> > On Thu 2020-06-18 13:11:05, [email protected] wrote:
> > > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
> > > > Yes, I'm wondering as well if people are really going to use the
> > > > new flags and filter flags - I mentioned that here:
> > > > https://lkml.org/lkml/2020/6/12/732
> > >
> > > yes, I saw, and replied there.
> >
> > No, the repply only explains how the interface might be used. There is
> > no prove that people would actually use it.
> >
> > > but since that was v1, and we're on v3, we should refresh.
> > >
> > > the central use-case is above, 1-liner version summarized here:
> > >
> > > 1- enable sites as you chase a problem with +up
> > > 2- examine them with grep =pu
> > > 3- change the set to suit, either by adding or subtracting callsites.
> > > 4- continue debugging, and changing callsites to suit
> > > 5- grep =pu control > ~/debugging-session-task1-callsites
> > > 6- echo up-p >control # disable for now, leave u-set for later
> > > 7- do other stuff
> > > 8 echo uP+p >control # reactivate useful debug-state and resume
> >
> > In short, this feature allows repeatedly enable/disable some
> > slowly growing maze of debug messages. Who need this, please? !!!
> >
> > If I am debugging then I add/remove debug messages. But I never
> > enable/disable all of them repeatedly.
>
> Not to say that I usually need to reboot when I reproduce the problem
> and before I could try it again. So all dyndbg flags gets lost
> between two tests anyway.
I agree, this feels way too complex for no good reason. Users only need
a specific set of "run this command to enable messages and send us the
logs" instructions. Nothing complex like this at all.
thanks,
greg k-h
On Thu 2020-06-18 13:11:05, [email protected] wrote:
> On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <[email protected]> wrote:
> > Yes, I'm wondering as well if people are really going to use the
> > new flags and filter flags - I mentioned that here:
> > https://lkml.org/lkml/2020/6/12/732
>
> yes, I saw, and replied there.
No, the repply only explains how the interface might be used. There is
no prove that people would actually use it.
> but since that was v1, and we're on v3, we should refresh.
>
> the central use-case is above, 1-liner version summarized here:
>
> 1- enable sites as you chase a problem with +up
> 2- examine them with grep =pu
> 3- change the set to suit, either by adding or subtracting callsites.
> 4- continue debugging, and changing callsites to suit
> 5- grep =pu control > ~/debugging-session-task1-callsites
> 6- echo up-p >control # disable for now, leave u-set for later
> 7- do other stuff
> 8 echo uP+p >control # reactivate useful debug-state and resume
In short, this feature allows repeatedly enable/disable some
slowly growing maze of debug messages. Who need this, please? !!!
If I am debugging then I add/remove debug messages. But I never
enable/disable all of them repeatedly.
Also this is far from the original problem. It was about debugging
a single driver (venus, drm). In this case, people need something
easy to use. The following is the easy way:
drm.debug = area_of_interest
venus.debug = level_of_interest
echo module=drm group=area_of_interest +p >control [*]
echo module=venus group=level_of_interes +p >control
Anyway, why filtering and 'u' flag would be necessary to debug these drivers?
Is anyone going to use it?
I would really like to hear the motivation for these features.
Has anyone asked for them?
Or are these just some "interesting" ideas from some brainstorming?
[*] Well, I wonder if the dyndbg interface would even be useful for drm
because it it is actually split into many modules. So it might require
creating/maintaining several filters.
Best Regards,
Petr
PS: This is probably my last mail in this thread. It goes in cycle.
You repeatedly explain how many possibilities the new features
allow. I repeatedly doubt that they are worth it.
I also proposed another solution for the original problem (venus)
but it has never been commented.
I just hope that these features will not get merged without
a clear interest in them.
On 6/18/20 6:48 PM, [email protected] wrote:
> On Thu, Jun 18, 2020 at 4:34 PM Stanimir Varbanov
> <[email protected]> wrote:
>>
>> Hi Jason, Jim,
>>
>
>
>
>>> I would be curious to see what Stanimir thinks of this proposal
>>> and whether it would work for his venus driver, which is what
>>> prompted this module group discussion.
>>
>> Hmm, we spin in a circle :)
>>
>> Infact this was my first way of implementing the groups in Venus driver,
>> you can see it at [1].
>>
>> +#define VDBGL(fmt, args...) pr_debug("VENUSL: " fmt, ##args)
>> +#define VDBGM(fmt, args...) pr_debug("VENUSM: " fmt, ##args)
>> +#define VDBGH(fmt, args...) pr_debug("VENUSH: " fmt, ##args)
>> +#define VDBGFW(fmt, args...) pr_debug("VENUSFW: " fmt, ##args)
>>
>
> I recall :-)
>
> I think Greg K-Hs distaste for those defines was for using them,
> as it tosses the utility of grep pr_debug
>
> pr_debug("VENUSM:"
> is barely longer than
> VDBGM
>
> with ddebug_exec_queries, you can leverage the existing format.
>
>>
Ok, yes, I like this approach because its simple (just exports
ddebug_exec_queries()), and it seems to be quite flexible. Module
authors can 'tag' their queries any way they want.
We could provide some structure, if desired, something like:
#define DYNAMIC_DEBUG_LOW "-V "
#define DYNAMIC_DEBUG_MED "-VV "
#define DYNAMIC_DEBUG_HIGH "-VVV "
#define DYNAMIC_DEBUG_REALLY_HIGH "-VVVV "
And then these could be added to the pr_debug() so:
#define VDBGL(fmt, args...) pr_debug("VENUSL: " DYNAMIC_DEBUG_LOW fmt, ##args)
or
#define VDBGL(fmt, args...) pr_debug(DYNAMIC_DEBUG_LOW fmt, ##args)
or just:
pr_debug(DYNAMIC_DEBUG_LOW "ERROR HERE: %d", err)
Thanks,
-Jason
>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_5_21_668&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=29UzIGELhVL1znJsgyjGDKGIEdSkSlCsmAh0jpbWHVQ&s=_szr6DQOsbdQ-oYCR9-fs4b-XG_fotTiObUfG3z6UtY&e=
>>
>> --
>> regards,
>> Stan
>
> thanks
> Jim
>