[PATCH 2/3] dyndbg: refine export, rename to dynamic_debug_exec_queries()
commit 59cf47e7df31 dyndbg: export ddebug_exec_queries
left a few configs broken, fix them with ifdef-stubs.
Rename the export to dynamic_debug_exec_queries(). This is a more
canonical function name, instead of exposing the 'ddebug' internal
name prefix. Do this now, before export hits v5.9.0
Implement as new function wrapping ddebug_exec_queries(now static
again), which copies the query-string, preserving ddebug_exec_queries'
in-place parsing, while allowing users to pass "const strings".
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 20
lib/dynamic_debug.c | 24 ++--
2 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa9ff9e1c0b3..b0191d3aff26 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -49,6 +49,10 @@ struct _ddebug {
#if defined(CONFIG_DYNAMIC_DEBUG_CORE)
+
+/* exported for module authors to exercise >control */
+int dynamic_debug_exec_queries(const char *query, const char *modname);
+
int ddebug_add_module(struct _ddebug *tab, unsigned int n,
const char *modname);
extern int ddebug_remove_module(const char *mod_name);
@@ -105,7 +109,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
static_branch_unlikely(&descriptor.key.dd_key_false)
#endif
-#else /* !HAVE_JUMP_LABEL */
+#else /* !CONFIG_JUMP_LABEL */
#define _DPRINTK_KEY_INIT
@@ -117,7 +121,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
#endif
-#endif
+#endif /* CONFIG_JUMP_LABEL */
#define __dynamic_func_call(id, fmt, func, ...) do { \
DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt); \
@@ -172,10 +176,11 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
KERN_DEBUG, prefix_str, prefix_type, \
rowsize, groupsize, buf, len, ascii)
-#else
+#else /* !CONFIG_DYNAMIC_DEBUG_CORE */
#include
#include
+#include
static inline int ddebug_add_module(struct _ddebug *tab, unsigned int n,
const char *modname)
@@ -210,6 +215,13 @@ static inline int ddebug_dyndbg_module_param_cb(char
*param, char *val,
print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, \
rowsize, groupsize, buf, len, ascii); \
} while (0)
-#endif
+
+static inline int dynamic_debug_exec_queries(const char *query, const char
*modname)
+{
+ printk(KERN_WARNING "kernel not built w CONFIG_DYNAMIC_DEBUG_CORE\n");
+ return 0;
+}
+
+#endif /* !CONFIG_DYNAMIC_DEBUG_CORE */
#endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 01b7d0210412..b6ab2c643116 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -525,7 +525,7 @@ static int ddebug_exec_query(char *query_string, const char
*modname)
last error or number of matching callsites. Module name is either
in param (for boot arg) or perhaps in query string.
*/
-int ddebug_exec_queries(char *query, const char *modname)
+static int ddebug_exec_queries(char *query, const char *modname)
{
char *split;
int i, errs = 0, exitcode = 0, rc, nfound = 0;
@@ -557,7 +557,27 @@ int ddebug_exec_queries(char *query, const char *modname)
return exitcode;
return nfound;
}
-EXPORT_SYMBOL_GPL(ddebug_exec_queries);
+
+/**
+ * dynamic_debug_exec_queries - apply changes to selected dynamic-debug prints
+ * @query: string with callsite-selectors +enablement+decorations
+ * @modname: string containing module name
+ *
+ * This implements the >/proc/dynamic_debug/control reader, allowing
+ * module authors to modify their dynamic-debug callsites. The modname
+ * is canonically struct module.mod_name, but can also be null or a
+ * module-wildcard, for example: "drm*".
+ */
+int dynamic_debug_exec_queries(const char *query, const char *modname)
+{
+ char *qry = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ int rc;
+ strncpy(qry, query, PAGE_SIZE);
+ rc = ddebug_exec_queries(qry, modname);
+ kfree(qry);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(dynamic_debug_exec_queries);
#define PREFIX_SIZE 64
--
2.26.2
[PATCH 0/3] dynamic-debug fixups for 5.9
- fix new export name, with a wrapper for more utility. - parse format="foo bar" like "format" "foo bar" - pretty-print Jim Cromie (3): dyndbg: give %3u width in pr-format, cosmetic only dyndbg: refine export, rename to dynamic_debug_exec_queries() dyndbg: fix problem parsing format="foo bar" include/linux/dynamic_debug.h | 20 +--- lib/dynamic_debug.c | 59 ++- 2 files changed, 53 insertions(+), 26 deletions(-) -- 2.26.2
[PATCH 1/3] dyndbg: give %3u width in pr-format, cosmetic only
Specify the print-width so log entries line up nicely.
no functional changes.
Signed-off-by: Jim Cromie
---
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 1d012e597cc3..01b7d0210412 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -947,7 +947,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
list_add(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
- v2pr_info("%u debug prints in module %s\n", n, dt->mod_name);
+ v2pr_info("%3u debug prints in module %s\n", n, dt->mod_name);
return 0;
}
--
2.26.2
[PATCH 3/3] dyndbg: fix problem parsing format="foo bar"
14775b049642 dyndbg: accept query terms like file=bar and module=foo
That commit broke on a tokenization modality where a word could start
with a quote, but couldnt continue with one. So the above would
tokenize as 'format="foo' and 'bar"', and fail hard.
This commit fixes the tokenizer by terminating an unquoted token on
the '=', avoiding that problem. And since ddebug-parse-query will
never see a combined 'keyword=value', revert those parts of the
previous commit.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 33 ++---
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b6ab2c643116..4af686e6ef59 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -237,6 +237,7 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
{
int nwords = 0;
+ vpr_info("entry, buf:'%s'\n", buf);
while (*buf) {
char *end;
@@ -247,6 +248,8 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
if (*buf == '#')
break; /* token starts comment, skip rest of line */
+ vpr_info("start-of-word:%d '%s'\n", nwords, buf);
+
/* find `end' of word, whitespace separated or quoted */
if (*buf == '"' || *buf == '\'') {
int quote = *buf++;
@@ -257,7 +260,9 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
return -EINVAL; /* unclosed quote */
}
} else {
- for (end = buf; *end && !isspace(*end); end++)
+ for (end = buf;
+*end && *end != '=' && !isspace(*end);
+end++)
;
BUG_ON(end == buf);
}
@@ -373,30 +378,20 @@ static int ddebug_parse_query(char *words[], int nwords,
unsigned int i;
int rc = 0;
char *fline;
- char *keyword, *arg;
+ if (nwords % 2 != 0) {
+ pr_err("expecting pairs of match-spec \n");
+ return -EINVAL;
+ }
if (modname)
/* support $modname.dyndbg= */
query->module = modname;
- for (i = 0; i < nwords; i++) {
- /* accept keyword=arg */
- vpr_info("%d w:%s\n", i, words[i]);
-
- keyword = words[i];
- arg = strchr(keyword, '=');
- if (arg) {
- *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);
+ for (i = 0; i < nwords; i+=2) {
+ char *keyword = words[i];
+ char *arg = words[i+1];
+ vpr_info("key:'%s' arg:'%s'\n", keyword, arg);
if (!strcmp(keyword, "func")) {
rc = check_set(&query->function, arg, "func");
} else if (!strcmp(keyword, "file")) {
--
2.26.2
[PATCH 2/2] dyndbg: give %3u width in pr-format, cosmetic only
Specify the print-width so log entries line up nicely.
no functional changes.
Signed-off-by: Jim Cromie
---
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 1d012e597cc3..01b7d0210412 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -947,7 +947,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
list_add(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
- v2pr_info("%u debug prints in module %s\n", n, dt->mod_name);
+ v2pr_info("%3u debug prints in module %s\n", n, dt->mod_name);
return 0;
}
--
2.26.2
[PATCH 0/2] for-5.9rc1 dyndbg fixes
2 items for v5.9-rc1 fix sparse complaint about exported ddebug_exec_queries fixed width format %3u Jim Cromie (2): dyndbg: add decl for exported ddebug_exec_queries() dyndbg: give %3u width in pr-format, cosmetic only include/linux/dynamic_debug.h | 3 +++ lib/dynamic_debug.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) -- 2.26.2
[PATCH 1/2] dyndbg: add decl for exported ddebug_exec_queries()
59cf47e7df31 dyndbg: export ddebug_exec_queries elicited a sparse complaint. Add declaration to header file. I skipped the kerneldoc for now, unsure where it should go. Ive seen it in .c files, Im leaning that way. Signed-off-by: Jim Cromie --- include/linux/dynamic_debug.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h index aa9ff9e1c0b3..61eb80c726bf 100644 --- a/include/linux/dynamic_debug.h +++ b/include/linux/dynamic_debug.h @@ -6,6 +6,9 @@ #include #endif +/* exported for module authors to exercise >control */ +int ddebug_exec_queries(char *query, const char *modname); + /* * An instance of this structure is created in a special * ELF section at every dynamic debug callsite. At runtime, -- 2.26.2
Re: [dyndbg] 4397a3e7bf: BUG:sleeping_function_called_from_invalid_context_at_mm/slab.h
.705259] schedule_timeout+0x205/0x2b0 > [ 29.705722] ? try_to_wake_up+0x7d/0x860 > [ 29.706176] ? _raw_spin_unlock_irq+0x24/0x50 > [ 29.70] ? wait_for_completion+0x81/0x110 > [ 29.707155] ? wait_for_completion+0x81/0x110 > [ 29.707646] wait_for_completion+0xab/0x110 > [ 29.708130] stop_one_cpu+0x87/0xb0 > [ 29.708551] ? set_cpus_allowed_ptr+0x20/0x20 > [ 29.709044] ? _raw_spin_unlock_irqrestore+0x41/0x70 > [ 29.709581] sched_exec+0x98/0xd0 > [ 29.709989] bprm_execve+0x1d7/0x3a0 > [ 29.710416] ? rest_init+0x23e/0x23e > [ 29.710844] kernel_execve+0x135/0x1c0 > [ 29.711284] kernel_init+0x6e/0x112 > [ 29.711703] ret_from_fork+0x22/0x30 > [ 29.712298] Failed to execute /init (error -14) > [ 29.713523] Run /sbin/init as init process > [ 29.714612] with arguments: > [ 29.715484] /sbin/init > [ 29.716328] with environment: > [ 29.717235] HOME=/ > [ 29.717983] TERM=linux > [ 29.718801] user=lkp > [ 29.719587] > job=/lkp/jobs/scheduled/vm-snb-151/boot-1-aliyun-x86_64-20190626.cgz-4397a3e7bf020ef040be371dcc178db258b928b4-20200808-11882-w8z2ep-10.yaml > [ 29.721551] ARCH=x86_64 > [ 29.721986] kconfig=x86_64-rhel-7.6-kselftests > [ 29.722623] > branch=linux-review/Jim-Cromie/dyndbg-WIP-diet-plan/20200808-041343 > [ 29.723634] commit=4397a3e7bf020ef040be371dcc178db258b928b4 > [ 29.724395] > BOOT_IMAGE=/pkg/linux/x86_64-rhel-7.6-kselftests/gcc-9/4397a3e7bf020ef040be371dcc178db258b928b4/vmlinuz-5.8.0-10185-g4397a3e7bf020 > [ 29.725943] max_uptime=600 > [ 29.726406] > RESULT_ROOT=/result/boot/1/vm-snb/aliyun-x86_64-20190626.cgz/x86_64-rhel-7.6-kselftests/gcc-9/4397a3e7bf020ef040be371dcc178db258b928b4/8 > [ 29.728009] LKP_SERVER=inn > [ 29.728475] softlockup_panic=1 > [ 29.728969] prompt_ramdisk=0 > [ 29.729449] vga=normal > [ 29.729967] Starting init: /sbin/init exists but couldn't execute it > (error -14) > [ 29.730967] Run /etc/init as init process > [ 29.731526] with arguments: > [ 29.731986] /etc/init > [ 29.732410] with environment: > [ 29.732880] HOME=/ > [ 29.733277] TERM=linux > [ 29.733711] user=lkp > [ 29.734125] > job=/lkp/jobs/scheduled/vm-snb-151/boot-1-aliyun-x86_64-20190626.cgz-4397a3e7bf020ef040be371dcc178db258b928b4-20200808-11882-w8z2ep-10.yaml > [ 29.735762] ARCH=x86_64 > [ 29.736211] kconfig=x86_64-rhel-7.6-kselftests > [ 29.736844] > branch=linux-review/Jim-Cromie/dyndbg-WIP-diet-plan/20200808-041343 > [ 29.737860] commit=4397a3e7bf020ef040be371dcc178db258b928b4 > [ 29.738609] > BOOT_IMAGE=/pkg/linux/x86_64-rhel-7.6-kselftests/gcc-9/4397a3e7bf020ef040be371dcc178db258b928b4/vmlinuz-5.8.0-10185-g4397a3e7bf020 > [ 29.740172] max_uptime=600 > [ 29.740634] > RESULT_ROOT=/result/boot/1/vm-snb/aliyun-x86_64-20190626.cgz/x86_64-rhel-7.6-kselftests/gcc-9/4397a3e7bf020ef040be371dcc178db258b928b4/8 > [ 29.742238] LKP_SERVER=inn > [ 29.742699] softlockup_panic=1 > [ 29.743202] prompt_ramdisk=0 > [ 29.743680] vga=normal > [ 29.744156] Run /bin/init as init process > [ 29.744719] with arguments: > [ 29.745179] /bin/init > [ 29.745599] with environment: > [ 29.746074] HOME=/ > [ 29.746468] TERM=linux > [ 29.746894] user=lkp > [ 29.747309] > job=/lkp/jobs/scheduled/vm-snb-151/boot-1-aliyun-x86_64-20190626.cgz-4397a3e7bf020ef040be371dcc178db258b928b4-20200808-11882-w8z2ep-10.yaml > [ 29.748964] ARCH=x86_64 > [ 29.749406] kconfig=x86_64-rhel-7.6-kselftests > [ 29.750047] > branch=linux-review/Jim-Cromie/dyndbg-WIP-diet-plan/20200808-041343 > [ 29.751067] commit=4397a3e7bf020ef040be371dcc178db258b928b4 > [ 29.751818] > BOOT_IMAGE=/pkg/linux/x86_64-rhel-7.6-kselftests/gcc-9/4397a3e7bf020ef040be371dcc178db258b928b4/vmlinuz-5.8.0-10185-g4397a3e7bf020 > [ 29.753381] max_uptime=600 > [ 29.753845] > RESULT_ROOT=/result/boot/1/vm-snb/aliyun-x86_64-20190626.cgz/x86_64-rhel-7.6-kselftests/gcc-9/4397a3e7bf020ef040be371dcc178db258b928b4/8 > [ 29.755455] LKP_SERVER=inn > [ 29.755923] softlockup_panic=1 > [ 29.756425] prompt_ramdisk=0 > [ 29.756905] vga=normal > [ 29.757375] Run /bin/sh as init process > [ 29.757921] with arguments: > [ 29.758380] /bin/sh > [ 29.758782] with environment: > [ 29.759256] HOME=/ > [ 29.759650] TERM=linux > [ 29.760089] user=lkp > [ 29.760501] > job=/lkp/jobs/scheduled/vm-snb-151/boot-1-aliyun-x86_64-20190626.cgz-4397a3e7bf020ef040be371dcc178db258b928b4-20200808-11882-w8z2ep-10.yaml > [ 29.76
Re: [PATCH 3/7] dyndbg: select ZPOOL in Kconfig.debug
On Sat, Aug 8, 2020 at 11:06 PM Randy Dunlap wrote: > > On 8/7/20 1:09 PM, Jim Cromie wrote: > > dyndbg will next need zs_malloc and friends, so add config reqs now, > > to avoid touching make-deps late in a patch-set. > > > > I used select in order not to hide dyndbg inadvertently. > > I want to say recommends, since it could be an optional feature. > > Whats the best way ? > > Hi Jim, > Can you elaborate on what/why/when it could be an optional feature? > hi Randy, I dont think making it optional adds any real value. if ZPOOL/ZRAM/ZSWAP is not included, we dont get any of that sweet sweet compression. or the off-lining of >1/2 the memory. I've got 46 callsites enabled atm, which is more than average, ~3100 callsites offlined. This is the payoff for the added complexity and memory (the site pointer) fwiw, Im not entirely clear on which of ZPOOL/ZRAM/ZSWAP is the correct dependency/ies. ZSWAP feels like the best destination for the data, especially if the data can be pushed aggressively into it.
[PATCH 0/7] dyndbg: WIP diet plan
dynamic-debug metadata is bloated; the __dyndbg linker section is effectively an array of struct _ddebugs, and its 1st 3 members are highly repetetive, with 90%, 84%, 45% repeats. Total reported usage ~150kb for ~2600 callsites on my laptop config. This patchset is one diet plan. it all holds together nicely until the "cache" commit, when it blows up starting init (or right after freeing unused kernel image, which Im hoping to do...). last commit log has the BUG trace from a LOCKDEP build, which reports stuff I dont quite undertand, except that it looks bad. Jim Cromie (7): dyndbg: give %3u width in pr-format, cosmetic only dyndbg: motivate a diet plan dyndbg: select ZPOOL in Kconfig.debug dyndbg: split struct _ddebug in 2, creating _ddebug_callsite dyndbg: WIP replace __dyndbg_callsite section with a zs-pool copy. dyndbg: add locking around zpool-add loop in zpool-init dyndbg: enable 'cache' of active pr_debug callsites include/asm-generic/vmlinux.lds.h | 4 + include/linux/dynamic_debug.h | 36 +++-- lib/Kconfig.debug | 1 + lib/dynamic_debug.c | 220 +- 4 files changed, 218 insertions(+), 43 deletions(-) -- 2.26.2
[PATCH 4/7] dyndbg: split struct _ddebug in 2, creating _ddebug_callsite
Split the struct into 2 linked parts (head & body) so that next,
struct _ddebug_callsite can be off-lined to zram, and only mapped in
as needed. The split increases overall memory use by 1 pointer per
callsite, but 4 pointers and a short are now 99% likely to be off-line
(once implemented), and compressed.
dyndbg: 264 modules, 2541 entries and 10560 bytes in ddebug tables,\
81312 bytes in __dyndbg section, 101640 bytes in __dyndbg_callsites section
zram should give us some compression on the _ddebug_callsite data, and
if it doesn't, a simple run-length-encoder would be effective enough,
given the repetition on (modname,filename,function) of (90%,84%,45%)
and the highly ordered contents of the __ddebug_callsites section.
I wanted to also put the key field into _ddebug_callsite, since its
use is super rare; it is only used when enabling/disabling a callsite,
ie only when >control is being actively exersized. But that blew up
with asm goto errors, so I punted.
So struct _ddebug_callsite is all const, RO data. Maybe thats
advantageous later, but with key there too, we can shrink the online
struct _ddebug further.
details of how:
dynamic_debug.h:
I literally cut struct _ddebug in half, renamed top-half, kept
__align(8) on both, added forward decl for unified comment (not needed
by compiler), and a site pointer from _ddebug to _ddebug_callsite, and
a few "->site" additions.
DECLARE_DYNAMIC_DEBUG_METADATA does the rest; it declares and
initializes both static struct vars together, and refs one to the
other. Note: this may conflict with new guards against cross-linked
sections, but __init work on __initdata seems a fair loophole.
dynamic_debug.c:
dynamic_debug_init() gets "->site" added everywhere needed.
Other 3 runtime users get a new _ddebug_callsite *dc pointer,
initialized with a single dp->site deref, and s/dp/dc/ as needed.
These once-per-func dp->site derefs are a setup for the next commit.
vmlinux.lds.h:
add __ddebug_callsites section, with the same align(8) and KEEP as
used in the __ddebug section. I suspect keep may go as a part of
effecting the section reclaim.
Signed-off-by: Jim Cromie
---
include/asm-generic/vmlinux.lds.h | 4 +++
include/linux/dynamic_debug.h | 35 +++--
lib/dynamic_debug.c | 52 +--
3 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index c78629ec79b6..c874216c01f5 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -320,6 +320,10 @@
*(__tracepoints)\
/* implement dynamic printk debug */\
. = ALIGN(8); \
+ __start___dyndbg_callsites = .; \
+ KEEP(*(__dyndbg_callsites)) \
+ __stop___dyndbg_callsites = .; \
+ . = ALIGN(8); \
__start___dyndbg = .; \
KEEP(*(__dyndbg)) \
__stop___dyndbg = .;\
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa9ff9e1c0b3..bbb06a44c0cf 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -7,11 +7,14 @@
#endif
/*
- * An instance of this structure is created in a special
- * ELF section at every dynamic debug callsite. At runtime,
- * the special section is treated as an array of these.
+ * a pair of these structs are created in 2 special ELF sections
+ * (__dyndbg, __dyndbg_callsites) for every dynamic debug callsite.
+ * During init, __dyndbg_callsites is copied to zram, and links to
+ * them in _ddebug are updated. At runtime, the __dyndbg section is
+ * treated as an array of struct _ddebugs.
*/
-struct _ddebug {
+struct _ddebug;
+struct _ddebug_callsite {
/*
* These fields are used to drive the user interface
* for selecting and displaying debug callsites.
@@ -20,7 +23,11 @@ struct _ddebug {
const char *function;
const char *filename;
const char *format;
- unsigned int lineno:18;
+ const unsigned int lineno:16;
+} __aligned(8);
+
+struct _ddebug {
+ struct _ddebug_callsite *site;
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -32,20 +39,26 @@ 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_MAPPED (1<<7) /* reserved */
#if defin
[PATCH 2/7] dyndbg: motivate a diet plan
this throwaway patch demonstrates the extra weight:
dyndbg: 2605 entries. repeated entries: 2369 module 2231 file 1147 func
Thats (91%, 86%, 44%) repeated values in those pointers/columns.
This simple test also shows that a similarly simple run-length encoder
on those 3 columns would compress this table dramatically.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 01b7d0210412..691e79826fc2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1059,11 +1059,12 @@ static int __init dynamic_debug_init_control(void)
static int __init dynamic_debug_init(void)
{
- struct _ddebug *iter, *iter_start;
+ struct _ddebug *iter, *iter_start, *prev = 0;
const char *modname = NULL;
char *cmdline;
int ret = 0;
int n = 0, entries = 0, modct = 0;
+ int modreps = 0, funcreps = 0, filereps = 0;
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1077,7 +1078,16 @@ static int __init dynamic_debug_init(void)
iter = __start___dyndbg;
modname = iter->modname;
iter_start = iter;
- for (; iter < __stop___dyndbg; iter++) {
+ for (prev = iter; iter < __stop___dyndbg; iter++) {
+ if (entries) {
+ if (prev->site->modname == iter->site->modname)
+ modreps++;
+ if (prev->site->function == iter->site->function)
+ funcreps++;
+ if (prev->site->filename == iter->site->filename)
+ filereps++;
+ prev++; /* one behind iter */
+ }
entries++;
if (strcmp(modname, iter->modname)) {
modct++;
@@ -1099,6 +1109,9 @@ static int __init dynamic_debug_init(void)
modct, entries, (int)(modct * sizeof(struct ddebug_table)),
(int)(entries * sizeof(struct _ddebug)));
+ vpr_info("%d entries. repeated entries: %d module %d file %d func\n",
+entries, modreps, filereps, funcreps);
+
/* apply ddebug_query boot param, dont unload tables on err */
if (ddebug_setup_string[0] != '\0') {
pr_warn("ddebug_query param name is deprecated, change it to
dyndbg\n");
--
2.26.2
[PATCH 3/7] dyndbg: select ZPOOL in Kconfig.debug
dyndbg will next need zs_malloc and friends, so add config reqs now, to avoid touching make-deps late in a patch-set. I used select in order not to hide dyndbg inadvertently. I want to say recommends, since it could be an optional feature. Whats the best way ? Signed-off-by: Jim Cromie --- lib/Kconfig.debug | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 9ad9210d70a1..a7973063baf0 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -100,6 +100,7 @@ config DYNAMIC_DEBUG depends on PRINTK depends on (DEBUG_FS || PROC_FS) select DYNAMIC_DEBUG_CORE + select ZPOOL help Compiles debug level messages into the kernel, which would not -- 2.26.2
[PATCH 6/7] dyndbg: add locking around zpool-add loop in zpool-init
This commit doesnt improve things, last commit was working, next one
still breaks, despite this "fix". I keep it separate for the
following review, which now stated, will be refuted as needed. ;-)
Locking review:
ddebug_zpool_init(), like other *_init() routines, does no locking
itself. Unlike them, it runs later, at late_init. This patch doesnt
fix the kernel panic that HEAD+1 does.
ddebug_callsite_get/put() are called as a pair under mutex-lock for
all 3 callsite users, 2: ddebug_change(), dynamic_emit_prefix(), do
their own ABBA-ish LGPU (Lock-Get-Put-Unlock).
ddebug_proc_show() does the GP part, and is wrapped by
ddebug_proc_start|stop() with LU.
ddebug_add_module() does LU to protect list_add(), HEAD~1 added
ddebug_zpool_add() loop inside that protection.
This commit adds locking to ddebug_zpool_init(), around the loop of
ddebug_zpool_add(), to match the locking in ddebug_add_module().
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fdf52a26a504..701d3d1fb7e7 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1182,8 +1182,10 @@ static void __init ddebug_zpool_init(void)
}
/* add-module normally does this, but not in time for builtins */
+ mutex_lock(&ddebug_lock);
for (iter = __start___dyndbg; iter < __stop___dyndbg; iter++)
ddebug_zpool_add(iter);
+ mutex_unlock(&ddebug_lock);
v2pr_info("total pages: %lu compaction: %lu\n",
zs_get_total_pages(dd_callsite_zpool),
@@ -1280,4 +1282,3 @@ early_initcall(dynamic_debug_init);
/* Debugfs setup must be done later */
fs_initcall(dynamic_debug_init_control);
-
--
2.26.2
[PATCH 5/7] dyndbg: WIP replace __dyndbg_callsite section with a zs-pool copy.
HEAD~1 split struct _ddebugs into heads & bodies, linked accross 2 ELF sections. Lets now store copies of the bodies into a zs_pool, and relink head to the new body. This should allow recycling the section soon. The strategy is to let a compression algo handle the repetition, and map individual records in when needed. If that works, we only need one record at a time, so the 3:1 zram compression applys to 99.9% of callsites, ie to 98120 bytes below: dyndbg: 259 modules, 2453 entries and 10360 bytes in ddebug tables, \ 98120 bytes in __dyndbg section, 98120 bytes in __dyndbg_callsites section dyndbg: 2453 entries. repeated entries: 2193 module 2058 file 981 func Since dynamic_debug is nice to have but rarely used, paying more for active logging to save in-kernel memory might be a worthwhile tradeoff. This is how: ddebug_zpool_init(), at late_initcall time, creates the zpool. Per dmesg, this is immediately after zswap is ready. Existing ddebug_add_module() now also calls ddebug_zpool_add(in a loop), which copies the _ddebug_callsite record into the zpool, once it is ready. For builtins, added early, the pool is not ready yet. So ddebug_zpool_init() also calls ddebug_zpool_add() for all those builtin modules already _add()ed. ddebug_zpool_add() does, foreach _ddebug: - zs_mallocs and saves callsite to .zhandle, - zs_maps and saves .site to it - zs_unmaps it, triggering write to zram - .site=0, detaching __dyndbg_callsites[item] which later triggers zs_map (of the copy) ddebug_zpool_remove() undoes ddebug_zpool_add(). We call it from ddebug_remove_module(). So we leave late-init with a full zram copy, and __dyndbg_callsites ready to reclaim. run-time access for the 3 existing users of struct _ddebug is entirely via 2 helpers; ddebug_callsite_get(), ddebug_callsite_put(). These 3 uses are all with ddebug_lock held. _get() returns .site 1st, or maps zrec to it and returns it. _put() always unmaps, minimizing online ram (see 1 below) ddebug_change() also gets s/continue/goto skipsite/g to cleanly unmap the record at the bottom of the loop. This much works. Next steps: 1. dont always unmap in _put(), leave enabled pr_debugs mapped. this is sticking point atm. This would make the set of zs_mapped recs work like a cache, reducing zs_mappings to a trickle, and only for <>control. 2. adapt struct _ddebug to unionize .site and .zhandle We currently use !!.site and !!.zhandle tests to keep state, we have bits available in flags to replace these tests, making union possible. A crazy alt is to create a "restricted" zpool, which limits pool size to 2**N handles, and returned handles to an (N+crc)-bit number, which would fit beside our 8-bit flags, inside an 8-byte pointer. This would be cool, but isnt necessary, since union looks practical. 3. move key from struct _ddebug into _ddebug_callsite key doesnt exactly fit semantically (and its not RO), but its use is super rare, and is never needed without also needing all the other callsite data. Getting it out of head saves on-line ram. But it broke in HEAD~1 when I tried it, with asm goto errors. 4. linker question DECLARE_DYNAMIC_DEBUG_METADATA macro fills 2 sections simultaneously. If we can rely on the linker to keep those records in identical order, we can 'index' both _dyndbg & _callsites (& keys) with [N-opaque] and drop the .site pointer. 5. improve compression ? Im seeing 3:1 pages_per_zspage: # cut -c1-12,40-77,85-90 $SKD/zsmalloc/dyndbg_callsites/classes | head -n3 class sizebj_allocated obj_used pages_used pagzspage 032 0 0 0 1 1482816 2605 33 3 Since the __dyndbg_callsite section is well sorted, a simple run-length encoding on 3 RO members (modname, file, function) could reclaim most of the (90%, 84%, 45%) repeated values. That said, a standard compressor should be able to compete, and there are several to try. Lastly, this whole zram block is RO after late-init (maybe, if we split into 2 zpools, for builtins and loadable modules). So compression can be biased towards costly but excellent compression (esp for RO zpool) and good small block decompression for random-ish access. 6. we could add a ram_reclaim(), to walk the dt-list, check each _ddebug, and unmap the mapped ones. This presumes 1. is done, and there are sometimes memory pressures to respond to. Potential benefits: - convert in-kernel mem to zram/etc - zram compression - eventually swap it out entirely - map in the enabled callsites only Signed-off-by: Jim Cromie --- include/linux/dynamic_debug.h | 1 + lib/dynamic_debug.c | 156 +++--- 2 files changed, 145 insertions(+), 12 deletions(-) diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h index bbb06a44c0cf..cae82e046685 100644 --- a/include/linux/dynamic_debug.h +++ b/include/linux/dynamic_debug.h @@
[PATCH 1/7] dyndbg: give %3u width in pr-format, cosmetic only
Specify the print-width so log entries line up nicely.
no functional changes.
Signed-off-by: Jim Cromie
---
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 1d012e597cc3..01b7d0210412 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -947,7 +947,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
list_add(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
- v2pr_info("%u debug prints in module %s\n", n, dt->mod_name);
+ v2pr_info("%3u debug prints in module %s\n", n, dt->mod_name);
return 0;
}
--
2.26.2
[PATCH 6/8] dyndbg: ddebug_zpool_remove
add ddebug_zpool_remove() to undo ddebug_zpool_add(), and call it from
ddebug_remove_module().
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 049299027fb3..102f47b2a439 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1007,6 +1007,18 @@ static void ddebug_zpool_add(struct _ddebug *dp)
zs_unmap_object(dd_callsite_zpool, handle);
}
+static void ddebug_zpool_remove(struct _ddebug *dp)
+{
+ if (dp->site) {
+ pr_warn("zhandle shouldnt be mapped now\n");
+ zs_unmap_object(dd_callsite_zpool, dp->zhandle);
+ }
+ if (!dp->zhandle)
+ pr_err("zhandle already cleared !\n");
+ else
+ zs_free(dd_callsite_zpool, dp->zhandle);
+}
+
/*
* Allocate a new ddebug_table for the given module
* and add it to the global list.
@@ -1104,6 +1116,13 @@ int ddebug_remove_module(const char *mod_name)
mutex_lock(&ddebug_lock);
list_for_each_entry_safe(dt, nextdt, &ddebug_tables, link) {
if (dt->mod_name == mod_name) {
+
+ unsigned int n = dt->num_ddebugs;
+ struct _ddebug *dp = dt->ddebugs;
+
+ for (; n; n--, dp++)
+ ddebug_zpool_remove(dp);
+
ddebug_table_free(dt);
ret = 0;
break;
--
2.26.2
[PATCH 7/7] dyndbg: enable 'cache' of active pr_debug callsites
in ddebug_zpool_put() dont zs_unmap the callsite, if it is enabled for printing. This will eliminate possibly repeated un-maps then re-maps of enabled and invoked pr-debug callsites, and will promptly retire all other uses. But this causes kernel to BUG [1.364303] BUG: sleeping function called from invalid context at mm/slab.h:567 [jimc@frodo build-v2]$ krun -a main.dyndbg=+pmf -q=-s -q=-S ./.virtme_mods/lib/modules/0.0.0 /usr/bin/qemu-system-x86_64 -fsdev local,id=virtfs1,path=/,security_model=none,readonly,multidevs=remap -device virtio-9p-pci,fsdev=virtfs1,mount_tag=/dev/root -fsdev local,id=virtfs5,path=/usr/local/lib/python3.8/site-packages/virtme-0.1.1-py3.8.egg/virtme/guest,security_model=none,readonly,multidevs=remap -device virtio-9p-pci,fsdev=virtfs5,mount_tag=virtme.guesttools -machine accel=kvm:tcg -watchdog i6300esb -cpu host -parallel none -net none -echr 1 -serial none -chardev stdio,id=console,signal=off,mux=on -serial chardev:console -mon chardev=console -vga none -display none -kernel ./arch/x86/boot/bzImage -append 'virtme_link_mods=/home/jimc/projects/lx/linux.git/build-v2/.virtme_mods/lib/modules/0.0.0 earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps "virtme_stty_con=rows 25 cols 102 iutf8" TERM=xterm-256color rootfstype=9p rootflags=version=9p2000.L,trans=virtio,access=any raid=noautodetect ro nokaslr dynamic_debug.verbose=3 module.dyndbg=+pm main.dyndbg=+pmf init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o ro,version=9p2000.L,trans=virtio,access=any virtme.guesttools /run/virtme/guesttools;exec /run/virtme/guesttools/virtme-init"' -s -S Wrong EFI loader signature. early console in extract_kernel input_data: 0x033373a8 input_len: 0x00aba748 output: 0x0100 output_len: 0x025f9e28 kernel_total_size: 0x02e2c000 needed_size: 0x0300 trampoline_32bit: 0x0009d000 KASLR disabled: 'nokaslr' on cmdline. Decompressing Linux... Parsing ELF... No relocation needed... done. Booting the kernel. [0.00] Linux version 5.8.0-00025-g4e76f4427bf8 (jimc@frodo) (gcc (GCC) 10.2.1 20200723 (Red Hat 10.2.1-1), GNU ld version 2.34-4.fc32) #30 SMP Thu Aug 6 16:39:03 MDT 2020 [0.00] Command line: virtme_link_mods=/home/jimc/projects/lx/linux.git/build-v2/.virtme_mods/lib/modules/0.0.0 earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps "virtme_stty_con=rows 25 cols 102 iutf8" TERM=xterm-256color rootfstype=9p rootflags=version=9p2000.L,trans=virtio,access=any raid=noautodetect ro nokaslr dynamic_debug.verbose=3 module.dyndbg=+pm main.dyndbg=+pmf init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o ro,version=9p2000.L,trans=virtio,access=any virtme.guesttools /run/virtme/guesttools;exec /run/virtme/guesttools/virtme-init" [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers' [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' [0.00] x86/fpu: Supporting XSAVE feature 0x008: 'MPX bounds registers' [0.00] x86/fpu: Supporting XSAVE feature 0x010: 'MPX CSR' [0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256 [0.00] x86/fpu: xstate_offset[3]: 832, xstate_sizes[3]: 64 [0.00] x86/fpu: xstate_offset[4]: 896, xstate_sizes[4]: 64 [0.00] x86/fpu: Enabled xstate features 0x1f, context size is 960 bytes, using 'compacted' format. [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: [mem 0x-0x0009fbff] usable [0.00] BIOS-e820: [mem 0x0009fc00-0x0009] reserved [0.00] BIOS-e820: [mem 0x000f-0x000f] reserved [0.00] BIOS-e820: [mem 0x0010-0x07fd] usable [0.00] BIOS-e820: [mem 0x07fe-0x07ff] reserved [0.00] BIOS-e820: [mem 0xfeffc000-0xfeff] reserved [0.00] BIOS-e820: [mem 0xfffc-0x] reserved [0.00] printk: bootconsole [earlyser0] enabled [0.00] NX (Execute Disable) protection: active [0.00] SMBIOS 2.8 present. [0.00] DMI: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-2.fc32 04/01/2014 [0.00] Hypervisor detected: KVM [0.00] kvm-clock: Using msrs 4b564d01 and 4b564d00 [0.00] kvm-clock: cpu 0, msr 3141001, primary cpu clock [0.00] kvm-clock: using sched offset of 218135524 cycles [0.000900] clocksource: kvm-clock: mask: 0x max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns [0.003580] tsc: Detected 2591.998 MHz processor [0.004611] last_pfn = 0x7fe0 max_arch_pfn = 0x4 [0.005518] x86/PAT: Configuration [0-7]: WB WC UC- UC WB WP UC- WT [0.013334] found SMP MP-table
[PATCH 8/8] dyndbg: add locking around zpool-add loop in zpool-init
summary: no fix here.
Locking review:
ddebug_zpool_init(), like other *_init() routines, does not run under
a lock (that we control). Unlike them, it runs later, at late_init.
I dont know whether this is pertinent to the kernel panic.
ddebug_callsite_get/put() are called as a pair under mutex-lock for
all 3 callsite users; dynamic_emit_prefix() does its own ABBA-ish LGPU
(Lock-Get-Put-Unlock), as does ddebug_change().
ddebug_proc_show() does GP, ddebug_proc_start|stop() wrap it with LU.
ddebug_add_module() does LU to protect list_add(), HEAD~x added
ddebug_zpool_add() inside that protection.
This commit adds locking to ddebug_zpool_init(), around the loop of
ddebug_zpool_add(), to match the locking in ddebug_add_module().
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9c51f24a9c66..716231a98910 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1187,8 +1187,10 @@ static void __init ddebug_zpool_init(void)
}
/* add-module normally does this, but not in time for builtins */
+ mutex_lock(&ddebug_lock);
for (iter = __start___dyndbg; iter < __stop___dyndbg; iter++)
ddebug_zpool_add(iter);
+ mutex_unlock(&ddebug_lock);
v2pr_info("total pages: %lu compaction: %lu\n",
zs_get_total_pages(dd_callsite_zpool),
@@ -1285,4 +1287,3 @@ early_initcall(dynamic_debug_init);
/* Debugfs setup must be done later */
fs_initcall(dynamic_debug_init_control);
-
--
2.26.2
[PATCH v5 07/18] dyndbg: fix a BUG_ON in ddebug_describe_flags
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
---
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
[PATCH v5 14/18] dyndbg: accept query terms like file=bar and module=foo
Current code expects "keyword" "arg" as 2 words, space separated.
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
---
.../admin-guide/dynamic-debug-howto.rst | 1 +
lib/dynamic_debug.c | 53 ---
2 files changed, 34 insertions(+), 20 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..fad6c34c930d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -341,7 +341,8 @@ static int check_set(const char **dest, char *src, char
*name)
/*
* Parse words[] as a ddebug query specification, which is a series
- * of (keyword, value) pairs chosen from these possibilities:
+ * of (keyword, value) pairs or combined keyword=value terms,
+ * chosen from these possibilities:
*
* func
* file
@@ -360,22 +361,34 @@ 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 \n");
- return -EINVAL;
- }
+ char *keyword, *arg;
if (modname)
/* support $modname.dyndbg= */
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];
+ arg = strchr(keyword, '=');
+ if (arg) {
+ *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
[PATCH v5 11/18] dyndbg: use gcc ?: to reduce word count
reduce word count via gcc ?: extension, no actual code change.
Signed-off-by: Jim Cromie
---
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
[PATCH v5 08/18] dyndbg: fix pr_err with empty string
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
---
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
[PATCH v5 12/18] dyndbg: refactor parse_linerange out of ddebug_parse_query
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, +pr_debug.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 63 ++---
1 file changed, 36 insertions(+), 27 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 6d0159075308..7a66d5e07f41 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -291,6 +291,41 @@ 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 - */
+ 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;
+ }
+ vpr_info("parsed line %d-%d\n", query->first_lineno,
+query->last_lineno);
+ return 0;
+}
+
static int check_set(const char **dest, char *src, char *name)
{
int rc = 0;
@@ -348,34 +383,8 @@ static int ddebug_parse_query(char *words[], int nwords,
UNESCAPE_SPECIAL);
rc = check_set(&query->format, words[i+1], "format");
} else if (!strcmp(words[i], "line")) {
- char *first = words[i+1];
- char *last = strchr(first, '-');
- if (query->first_lineno || query->last_lineno) {
- pr_err("match-spec: line used 2x\n");
- return -EINVAL;
- }
- if (last)
- *last++ = '\0';
- if (parse_lineno(first, &query->first_lineno) < 0)
+ if (parse_linerange(query, words[i+1]))
return -EINVAL;
- if (last) {
- /* range - */
- 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
[PATCH v5 18/18] dyndbg: export ddebug_exec_queries
s 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 via >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 standard 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`. Queries
submitted via export will typically have module specified, which
dramatically cuts the scan by ddebug_change vs "module=* +p".
ISTM this proposed export presents no locking problems.
TLDR;
It would be interesting to see how drm.dyndbg=$QUERY and
drm.debug=$HEXY would interact; it might be order dependent, as
if given as modprobe args or in /etc/modprobe.d/
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index e87b630bd793..1d012e597cc3 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -525,7 +525,7 @@ static int ddebug_exec_query(char *query_string, const char
*modname)
last error or number of matching callsites. Module name is either
in param (for boot arg) or perhaps in query string.
*/
-static int ddebug_exec_queries(char *query, const char *modname)
+int ddebug_exec_queries(char *query, const char *modname)
{
char *split;
int i, errs = 0, exitcode = 0, rc, nfound = 0;
@@ -557,6 +557,7 @@ static int ddebug_exec_queries(char *query, const char
*modname)
return exitcode;
return nfound;
}
+EXPORT_SYMBOL_GPL(ddebug_exec_queries);
#define PREFIX_SIZE 64
--
2.26.2
[PATCH v5 17/18] dyndbg: shorten our logging prefix, drop __func__
For log-message output, reduce column space consumed by current pr_fmt by dropping __func__ and shortening "dynamic_debug" to "dyndbg". This improves readability on narrow consoles, and better matches other kernel boot info messages. Signed-off-by: Jim Cromie --- 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 c72bb4d2eb7e..e87b630bd793 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -11,7 +11,7 @@ * Copyright (C) 2013 Du, Changbin */ -#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ +#define pr_fmt(fmt) "dyndbg: " fmt #include #include -- 2.26.2
[PATCH v5 16/18] dyndbg: allow anchored match on format query term
This should work:
echo module=amd* format=^[IF_TRACE]: +p >/proc/dynamic_debug/control
consider drivers/gpu/drm/amd/display/include/logger_types.h:
It has 11 defines like:
#define DC_LOG_IF_TRACE(...) pr_debug("[IF_TRACE]:"__VA_ARGS__)
These defines are used 804 times at recent count; they are a good use
case to evaluate existing format-message based classifications of
*pr_debug*. Those macros prefix the supplied format with a fixed
string, I'd expect most existing message classification schemes to do
something similar.
Hence we want to be able to anchor our match to the beginning of the
format string, allowing easy construction of clear and precise
queries, leveraging the existing classification scheme to enable and
disable those callsites.
Note that unlike other search terms, formats are implicitly floating
substring matches, without the need for explicit wildcards.
This makes no attempt at wider regex features, just the one we need.
TLDR: Using the anchor also means the []s are less helpful for
disamiguating the prefix from a random in-message occurrence, allowing
shorter prefixes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c983049e986d..c72bb4d2eb7e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -181,9 +181,16 @@ static int ddebug_change(const struct ddebug_query *query,
continue;
/* match against the format */
- if (query->format &&
- !strstr(dp->format, query->format))
- continue;
+ if (query->format) {
+ if (*query->format == '^') {
+ char *p;
+ /* anchored search. match must be at
beginning */
+ p = strstr(dp->format, query->format+1);
+ if (p != dp->format)
+ continue;
+ } else if (!strstr(dp->format, query->format))
+ continue;
+ }
/* match against the line number range */
if (query->first_lineno &&
--
2.26.2
[PATCH v5 06/18] dyndbg: fix overcounting of ram used by dyndbg
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
---
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
[PATCH v5 15/18] dyndbg: combine flags & mask into a struct, simplify with it
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 - declares query and flag-settings,
calls other 2, passing flags
- 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
no functional changes
Signed-off-by: Jim Cromie
---
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 fad6c34c930d..c983049e986d 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 = {};
#def
[PATCH v5 09/18] dyndbg: prefer declarative init in caller, to memset in callee
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
---
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 \n");
return -EINVAL;
}
- memset(query, 0, sizeof(*query));
if (modname)
/* support $modname.dyndbg= */
@@ -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
[PATCH v5 04/18] dyndbg: refine debug verbosity; 1 is basic, 2 more chatty
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 doesn't 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
---
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
[PATCH v5 03/18] dyndbg: drop obsolete comment on ddebug_proc_open
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
---
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 /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
[PATCH v5 13/18] dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'
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-5 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
---
.../admin-guide/dynamic-debug-howto.rst| 5 +
lib/dynamic_debug.c| 18 +-
2 files changed, 22 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 7a66d5e07f41..7eb963b1bd11 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -359,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) {
@@ -374,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
[PATCH v5 10/18] dyndbg: make ddebug_tables list LIFO for add/remove_module
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
---
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
[PATCH v5 05/18] dyndbg: rename __verbose section to __dyndbg
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
---
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 aa183c9ac0a2..e7b4ff7e4fd0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3237,7 +3237,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
-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_
[PATCH v5 01/18] dyndbg-docs: eschew file /full/path query in docs
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
---
.../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 /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 != "=_"' /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
[PATCH v5 02/18] dyndbg-docs: initialization is done early, not arch
since cf964976484 in 2012, initialization is done with early_initcall, update the Docs, which still say arch_initcall. Signed-off-by: Jim Cromie --- 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
[PATCH v5 00/18] dynamic_debug fixes, cleanups, features, export
this is v5, changes from previous: - moved a chunk from patch 13 to 12, per Jason - shorten logging prefix to "dyndbg", drop __func__ - now with more commit-log advocacy - shuffle EXPORT_GPL(ddebug_exec_queries) last. - v4+ series Acked-by: jba...@akamai.com v4: https://lore.kernel.org/lkml/20200620180643.887546-1-jim.cro...@gmail.com/ v3: https://lore.kernel.org/lkml/20200617162536.611386-1-jim.cro...@gmail.com/ v2: https://lore.kernel.org/lkml/20200613155738.2249399-1-jim.cro...@gmail.com/ v1: https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cro...@gmail.com/ Patchset starts with cleanups; - change section name from vague "__verbose" to specific "__dyndbg" - cleaner docs, drop obsolete comment & useless debug prints, - refine verbose/debug logging - fix a BUG_ON, ram reporting miscounts. etc.. Then adds query parsing conveniences - allow "file inode.c:100-200" # combined file & line-range - allow "file inode.c:start_*" # file & function - accept "module=foo" query form internal improvement - combine flags & mask in a struct, clean 3 func interfaces with it. make precise format queries easier - accept "format=^ClassString" anchored query finally, EXPORT_GPL(ddebug_exec_queries) This gives module authors complete run-time control over all their *pr_debug* callsites (when CONFIG_DYNAMIC_DEBUG=y). Following the drm.debug UI model, drm.debug_chan2 could be wired to a callback which invokes ddebug_exec_queries to toggle arbitary groups of pr_debug callsites. Useful callsite groups would exploit existing message-prefix classifcation schemes: "format=^[IF_TRACE]: +p; format=^[SURFACE]: +p" >control Jim Cromie (18): 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 dyndbg: refactor parse_linerange out of ddebug_parse_query dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100' dyndbg: accept query terms like file=bar and module=foo dyndbg: combine flags & mask into a struct, simplify with it dyndbg: allow anchored match on format query term dyndbg: shorten our logging prefix, drop __func__ dyndbg: export ddebug_exec_queries .../admin-guide/dynamic-debug-howto.rst | 29 +- include/asm-generic/vmlinux.lds.h | 6 +- include/linux/dynamic_debug.h | 4 +- kernel/module.c | 2 +- lib/dynamic_debug.c | 269 ++ 5 files changed, 173 insertions(+), 137 deletions(-) -- 2.26.2
Re: [PATCH v4 13/17] dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'
> > @@ -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;
> > }
>
> This bit seems like its unrelated to this patch and makes more sense in the
> previous patch, or as separate patch...
>
> Thanks,
>
> -Jason
>
ok, I'll split it out, maybe merge with prior.
Any other tweaks ?
maybe move export last in series ?
how do you feel about changing the pr_fmt
to just mod-name "dynamic_debug" or "dyndbg"
Jim
[PATCH v2] kernel/module: add name size info to pr_debug() calls
When the kernel is booted with arg: module.dyndbg=+p, dmesg gets
volumes of info about layout details of loaded modules. This adds
module & symbol names to those messages, and sizes where pertinent.
Since these are debug messages, I thought sizes might be useful to
somebody, without adding clutter or or too much info.
Now a reader can know which module's info they're looking at, or which
2 modules they are comparing.
--
v2 has:
better commit message
s/:%s/%s:/
s/final/Final/
Signed-off-by: Jim Cromie
---
kernel/module.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..809490145b77 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2294,8 +2294,8 @@ static int simplify_symbols(struct module *mod, const
struct load_info *info)
case SHN_ABS:
/* Don't need to do anything */
- pr_debug("Absolute symbol: 0x%08lx\n",
- (long)sym[i].st_value);
+ pr_debug("Absolute symbol: 0x%08lx %s\n",
+(long)sym[i].st_value, name);
break;
case SHN_LIVEPATCH:
@@ -2409,7 +2409,7 @@ static void layout_sections(struct module *mod, struct
load_info *info)
for (i = 0; i < info->hdr->e_shnum; i++)
info->sechdrs[i].sh_entsize = ~0UL;
- pr_debug("Core section allocation order:\n");
+ pr_debug("Core section allocation order for %s:\n", mod->name);
for (m = 0; m < ARRAY_SIZE(masks); ++m) {
for (i = 0; i < info->hdr->e_shnum; ++i) {
Elf_Shdr *s = &info->sechdrs[i];
@@ -2442,7 +2442,7 @@ static void layout_sections(struct module *mod, struct
load_info *info)
}
}
- pr_debug("Init section allocation order:\n");
+ pr_debug("Init section allocation order for %s:\n", mod->name);
for (m = 0; m < ARRAY_SIZE(masks); ++m) {
for (i = 0; i < info->hdr->e_shnum; ++i) {
Elf_Shdr *s = &info->sechdrs[i];
@@ -3278,7 +3278,7 @@ static int move_module(struct module *mod, struct
load_info *info)
mod->init_layout.base = NULL;
/* Transfer each section which specifies SHF_ALLOC */
- pr_debug("final section addresses:\n");
+ pr_debug("Final section addresses for %s:\n", mod->name);
for (i = 0; i < info->hdr->e_shnum; i++) {
void *dest;
Elf_Shdr *shdr = &info->sechdrs[i];
@@ -3296,8 +3296,8 @@ static int move_module(struct module *mod, struct
load_info *info)
memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
/* Update sh_addr to point to copy in image. */
shdr->sh_addr = (unsigned long)dest;
- pr_debug("\t0x%lx %s\n",
-(long)shdr->sh_addr, info->secstrings + shdr->sh_name);
+ pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
+(long)shdr->sh_size, info->secstrings + shdr->sh_name);
}
return 0;
--
2.26.2
Re: [PATCH] kernel/module: add name size info to pr_debug() calls
On Tue, Jun 30, 2020 at 3:37 AM Jessica Yu wrote:
>
> +++ Jim Cromie [11/06/20 08:20 -0600]:
> >when booted with arg: module.dyndbg=+p
> >dmesg gets volumes of info about loaded modules.
> >This adds module & symbol names, and sizes where pertinent.
> >Now I can know which module's info Im looking at.
>
> Hi,
>
> Could you please fix the changelog formatting according to
> Documentation/process/submitting-patches.rst? More specifically,
> complete sentences, line wrapped at 75 columns, and a Signed-off-by:
> line at the end. There are plenty of examples if you look through the
> lkml mailing list.
>
ack. less casual next time. and with -s
> >---
> > kernel/module.c | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> >diff --git a/kernel/module.c b/kernel/module.c
> >index e8a198588f26..d871d9cee9eb 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -2294,8 +2294,8 @@ static int simplify_symbols(struct module *mod, const
> >struct load_info *info)
> >
> > case SHN_ABS:
> > /* Don't need to do anything */
> >- pr_debug("Absolute symbol: 0x%08lx\n",
> >- (long)sym[i].st_value);
> >+ pr_debug("Absolute symbol: 0x%08lx %s\n",
> >+ (long)sym[i].st_value, name);
>
> I would prefer "Absolute symbol %s:" rather than putting the symbol
> name at the end.
i chose this for a more tabular appearance,
names vary in length, so value afterwards looks cluttered.
plus its more consistent with preceding messages
[3.458007] 0xc0023d40 0x0030 .bss
[3.458008] 0xc0028000 0x1158 .symtab
[3.458010] 0xc0029158 0x0af5 .strtab
[3.458021] Absolute symbol: 0x rapl.mod.c
[3.458022] Absolute symbol: 0x rapl.c
>
> > break;
> >
> > case SHN_LIVEPATCH:
> >@@ -2409,7 +2409,7 @@ static void layout_sections(struct module *mod, struct
> >load_info *info)
> > for (i = 0; i < info->hdr->e_shnum; i++)
> > info->sechdrs[i].sh_entsize = ~0UL;
> >
> >- pr_debug("Core section allocation order:\n");
> >+ pr_debug("Core section allocation order for: %s\n", mod->name);
>
> I would slightly prefer "Core section allocation order for %s:", but
> it's a matter of taste.
>
done everywhere you noted.
> > for (m = 0; m < ARRAY_SIZE(masks); ++m) {
> > for (i = 0; i < info->hdr->e_shnum; ++i) {
> > Elf_Shdr *s = &info->sechdrs[i];
> >@@ -2442,7 +2442,7 @@ static void layout_sections(struct module *mod, struct
> >load_info *info)
> > }
> > }
> >
> >- pr_debug("Init section allocation order:\n");
> >+ pr_debug("Init section allocation order for: %s\n", mod->name);
>
> Same here.
>
> > for (m = 0; m < ARRAY_SIZE(masks); ++m) {
> > for (i = 0; i < info->hdr->e_shnum; ++i) {
> > Elf_Shdr *s = &info->sechdrs[i];
> >@@ -3276,7 +3276,7 @@ static int move_module(struct module *mod, struct
> >load_info *info)
> > mod->init_layout.base = NULL;
> >
> > /* Transfer each section which specifies SHF_ALLOC */
> >- pr_debug("final section addresses:\n");
> >+ pr_debug("final section addresses for: %s\n", mod->name);
>
> Let's capitalize the "f" in "final section addresses" to be consistent
> with the other two above.
>
> > for (i = 0; i < info->hdr->e_shnum; i++) {
> > void *dest;
> > Elf_Shdr *shdr = &info->sechdrs[i];
> >@@ -3294,8 +3294,8 @@ static int move_module(struct module *mod, struct
> >load_info *info)
> > memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
> > /* Update sh_addr to point to copy in image. */
> > shdr->sh_addr = (unsigned long)dest;
> >- pr_debug("\t0x%lx %s\n",
> >- (long)shdr->sh_addr, info->secstrings +
> >shdr->sh_name);
> >+ pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
> >+ (long)shdr->sh_size, info->secstrings +
> >shdr->sh_name);
>
> Any reason why you added sh_size here? Is it really need
[PATCH v4 06/17] dyndbg: fix overcounting of ram used by dyndbg
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
---
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
[PATCH v4 10/17] dyndbg: make ddebug_tables list LIFO for add/remove_module
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
---
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
[PATCH v4 11/17] dyndbg: use gcc ?: to reduce word count
reduce word count via gcc ?: extension, no actual code change.
Signed-off-by: Jim Cromie
---
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
[PATCH v4 08/17] dyndbg: fix pr_err with empty string
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
---
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
[PATCH v4 02/17] dyndbg-docs: initialization is done early, not arch
since cf964976484 in 2012, initialization is done with early_initcall, update the Docs, which still say arch_initcall. Signed-off-by: Jim Cromie --- 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
[PATCH v4 04/17] dyndbg: refine debug verbosity; 1 is basic, 2 more chatty
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
---
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
[PATCH v4 03/17] dyndbg: drop obsolete comment on ddebug_proc_open
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
---
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 /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
[PATCH v4 14/17] dyndbg: accept query terms like file=bar and module=foo
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
---
.../admin-guide/dynamic-debug-howto.rst | 1 +
lib/dynamic_debug.c | 52 ---
2 files changed, 33 insertions(+), 20 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..65c224301509 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -341,7 +341,8 @@ static int check_set(const char **dest, char *src, char
*name)
/*
* Parse words[] as a ddebug query specification, which is a series
- * of (keyword, value) pairs chosen from these possibilities:
+ * of (keyword, value) pairs or combined keyword=value terms,
+ * chosen from these possibilities:
*
* func
* file
@@ -360,22 +361,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 \n");
- return -EINVAL;
- }
+ char *keyword, *arg;
if (modname)
/* support $modname.dyndbg= */
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 +403,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
[PATCH v4 07/17] dyndbg: fix a BUG_ON in ddebug_describe_flags
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
---
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
[PATCH v4 17/17] dyndbg: combine flags & mask into a struct, simplify with it
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 - declares query and flag-settings,
calls other 2, passing flags
- 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
no functional changes
Signed-off-by: Jim Cromie
---
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 d737c733967a..c0bc78d67b36 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;
@@ -196,14 +201,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;
@@ -436,11 +441,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 '+':
@@ -457,7 +460,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;
}
}
@@ -466,30 +469,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 = {};
#def
[PATCH v4 16/17] dyndbg: allow anchored match on format query term
This should work:
echo module=amd* format=^[IF_TRACE]: +p >/proc/dynamic_debug/control
consider drivers/gpu/drm/amd/display/include/logger_types.h:
It has 11 defines like:
#define DC_LOG_IF_TRACE(...) pr_debug("[IF_TRACE]:"__VA_ARGS__)
These defines are used 804 times at recent count; they are a perfect
test case to leverage existing format-message based classifications of
*pr_debug*.
Those macros prefix the supplied message with a fixed string, Id
expect nearly all existing classification schemes to do so. Hence we
want to be able to anchor our match to the beginning of the format
string, so ddebug_exec_queries() can be more exact. This gives easier
contruction of reliable queries to enable/disable those callsites.
This makes no attempt at wider regex features, just the one we need.
Since this is a corner-case of sorts, lets examine one more, hidden in
the example above; note the "module=amd*" query term.
// normal usage
ddebug_exec_queries("format=^[IF_TRACE]: +p", MODULE)
// wildcard 1
ddebug_exec_queries("module=amd* format=^[IF_TRACE]: +p", NULL)
// wildcard 2
ddebug_exec_queries("format=^[IF_TRACE]: +p", "amd*")
Now, I doubt that "amd*" is useful here, but I dont see a reason
to preclude it; multiple modules with coordinated classifcation
schemes is reasonable. That said, a single call can have multiple
queries, each with an exact module name.
I also see little reason to prefer either of forms 1 or 2. Its
case-by-case, judging brevity, clarity, and query specificity and
robustness.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b00f536d6d12..d737c733967a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -176,9 +176,15 @@ static int ddebug_change(const struct ddebug_query *query,
continue;
/* match against the format */
- if (query->format &&
- !strstr(dp->format, query->format))
- continue;
+ if (query->format) {
+ if (*query->format == '^') {
+ /* anchored search. match must be at
beginning */
+ char *p = strstr(dp->format,
query->format+1);
+ if (p != dp->format)
+ continue;
+ } else if (!strstr(dp->format, query->format))
+ continue;
+ }
/* match against the line number range */
if (query->first_lineno &&
--
2.26.2
[PATCH v4 09/17] dyndbg: prefer declarative init in caller, to memset in callee
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
---
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 \n");
return -EINVAL;
}
- memset(query, 0, sizeof(*query));
if (modname)
/* support $modname.dyndbg= */
@@ -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
[PATCH v4 00/17] dynamic_debug cleanups, query features, export
this is v4, changes from previous: - dropped flags extensions, one internal optimization kept - export ddebug_exec_queries() - done previously, but warrants attention - add ^anchor to format matching v3: https://lore.kernel.org/lkml/20200617162536.611386-1-jim.cro...@gmail.com/ v2: https://lore.kernel.org/lkml/20200613155738.2249399-1-jim.cro...@gmail.com/ v1: https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cro...@gmail.com/ Jim Cromie (17): 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.. 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 accept combined file:line & file:func forms file inode.c:100-200# file & line-range file inode.c:start_*# file & function dyndbg: refactor parse_linerange out of ddebug_parse_query dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100' accept keyword=value, not just "keyword value" (and not keyword:value) dyndbg: accept query terms like file=bar and module=foo dyndbg: export ddebug_exec_queries This will afford module authors complete run-time control over all the *pr_debug* callsites theyve coded. They can attach a callback to their existing debug interface (drm.debug for example), and map bits, bytes, or strings to particular queries. dyndbg: allow anchored match on format query term This makes "format=^PCI" work, which is far more specific than just "format=PCI". It is ideal for the most common convention in logging; a string prefix which classifies the log-entry in some way. dyndbg: combine flags & mask into a struct, simplify with it 1 less parameter on the stack. .../admin-guide/dynamic-debug-howto.rst | 29 +- include/asm-generic/vmlinux.lds.h | 6 +- include/linux/dynamic_debug.h | 4 +- kernel/module.c | 2 +- lib/dynamic_debug.c | 263 ++ 5 files changed, 169 insertions(+), 135 deletions(-) -- 2.26.2
[PATCH v4 13/17] dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'
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
---
.../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
[PATCH v4 15/17] dyndbg: export ddebug_exec_queries
Export ddebug_exec_queries() for use by modules.
This will allow module authors to control all their *pr_debug*s
dynamically. And since ddebug_exec_queries() is what implements
"echo $query >control", it gives the same complete control.
Virtues of this:
- simplicity. just an export.
- full control over any/all subsets of callsites.
- same "query/command-string" in code and console
- full callsite selectivity with module file line format
Format in particular deserves special attention; it is where
low-hanging fruit will be found.
Consider: drivers/gpu/drm/amd/display/include/logger_types.h:
#define DC_LOG_SURFACE(...) pr_debug("[SURFACE]:"__VA_ARGS__)
#define DC_LOG_HW_LINK_TRAINING(...)
pr_debug("[HW_LINK_TRAINING]:"__VA_ARGS__)
.. 9 more ..
Thats 11 string prefixes, used in 804 callsites across the module.
Clearly a systematized classification of those callsites. And one Id
expect to see repeated often.
Using ddebug_exec_queries(), authors can operate on those
classifications as a unitary set:
echo format="[SURFACE]:" +p >control
Trivially, those sets can be subsected with the other query terms too,
should the author see fit.
Using ddebug_exec_queries() from a callback, authors can change
*pr_debug* callstates when drm.debug is updated from userspace. They
can map bits, or strings, or whatever they want.
They could even alter [fmlt] flags, though I dont foresee why they would.
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`. Queries
submitted via export will have module specified, which dramatically
cuts matching work done by ddebug_change vs "module=* +p".
ISTM this proposed export presents no locking problems.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 65c224301509..b00f536d6d12 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -546,6 +546,7 @@ static int ddebug_exec_queries(char *query, const char
*modname)
return exitcode;
return nfound;
}
+EXPORT_SYMBOL_GPL(ddebug_exec_queries);
#define PREFIX_SIZE 64
--
2.26.2
[PATCH v4 05/17] dyndbg: rename __verbose section to __dyndbg
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
---
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
-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_
[PATCH v4 12/17] dyndbg: refactor parse_linerange out of ddebug_parse_query
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
---
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 - */
+ 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 - */
- 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
[PATCH v4 01/17] dyndbg-docs: eschew file /full/path query in docs
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
---
.../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 /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 != "=_"' /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
Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags
On Thu, Jun 18, 2020 at 4:34 PM Stanimir Varbanov
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
Re: [PATCH v3 14/21] dyndbg: accept query terms like file=bar and module=foo
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 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
> ---
> .../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
> * line - // 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 \n");
> - return -EINVAL;
> - }
> + char *keyword, *arg;
>
> if (modname)
> /* support $modname.dyndbg= */
> 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 |
>
Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags
On Thu, Jun 18, 2020 at 1:40 PM Jason Baron wrote:
>
>
>
> On 6/18/20 3:11 PM, jim.cro...@gmail.com wrote:
> > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron 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 > 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 >
> > 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
Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags
On Thu, Jun 18, 2020 at 12:17 PM Jason Baron 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 > Thanks, > > -Jason
Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags
On Thu, Jun 18, 2020 at 10:19 AM Petr Mladek 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
Re: [PATCH v3 19/21] dyndbg: extend ddebug_parse_flags to accept optional leading filter-flags
On Thu, Jun 18, 2020 at 6:44 AM Petr Mladek 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
Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags
On Wed, Jun 17, 2020 at 4:13 PM Joe Perches 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.
[PATCH v3 19/21] dyndbg: extend ddebug_parse_flags to accept optional leading filter-flags
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
---
.../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
[PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags
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 --- .../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:: lInclude line number in the printed message mInclude module name in the printed message tInclude thread ID in messages not generated from interrupt context + uuser 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 ``=`` (
[PATCH v3 13/21] dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'
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
---
.../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
[PATCH v3 07/21] dyndbg: fix a BUG_ON in ddebug_describe_flags
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
---
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
[PATCH v3 21/21] dyndbg: allow negating flag-chars in modifier flags
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
---
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
[PATCH v3 15/21] dyndbg: export ddebug_exec_queries
Exporting ddebug_exec_queries will allow module authors using dynamic debug to actually control/enable/disable their own pr-debug callsites dynamically. With it, module authors can tie any update of their internal debug variables to a corresponding ddebug_exec_queries invocation, which will modify all their selected callsites accordingly. Generally, authors would exec +p or -p on some subsets of their set of callsites, and leave fmlt flags for user to choose the appropriate amount of structural context desired in the logs. Depending upon the user needs, the module might be known, and therefore a waste of screen width, function would be valuable, file would be long and familiar to the author, etc.. That said, author could harness the message-prefix facility if they saw fit to do so. Any author preferences can be overridden with echo >control Is it safe ? ddebug_exec_queries() is currently 'exposed' to user space in several limited ways; 1 it is called from module-load callback, where it implements the $modname.dyndbg=+p "fake" parameter provided to all modules. 2 it handles query input from >control directly IOW, it is "fully" exposed to local root user; exposing the same functionality to other kernel modules is no additional risk. The other big issue to check is locking: dyndbg has a single mutex, taken by ddebug_change to handle >control, and by ddebug_proc_(start|stop) to span `cat control`. ISTM this proposed export presents no locking problems. drm use case: drm.debug=0x03 appears to be a kernel boot-arg example, setting 2 internal debug flags. Each bit is probably controlling a separate subset of all debug-prints, they may be overlapping subsets. Those subsets are *definitely* expressible as a few dyndbg queries each. Any arbitrary subset is. drm.dyndbg='file drm_gem_* +p' # gem debug drm.dyndbg='file *_gem_* +p' # *gem debug With this proposed export, drm authors could exec these examples, most likely in the callback that handles updates to the drm.debug variable. Signed-off-by: Jim Cromie --- 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
[PATCH v3 14/21] dyndbg: accept query terms like file=bar module=foo
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
---
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
* line - // 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 \n");
- return -EINVAL;
- }
+ char *keyword, *arg;
if (modname)
/* support $modname.dyndbg= */
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
[PATCH v3 02/21] dyndbg-docs: initialization is done early, not arch
since cf964976484 in 2012, initialization is done with early_initcall, update the Docs, which still say arch_initcall. Signed-off-by: Jim Cromie --- 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
[PATCH v3 04/21] dyndbg: refine debug verbosity; 1 is basic, 2 more chatty
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
---
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
[PATCH v3 12/21] dyndbg: refactor parse_linerange out of ddebug_parse_query
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
---
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 - */
+ 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 - */
- 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
[PATCH v3 17/21] dyndbg: refactor ddebug_read_flags out of ddebug_parse_flags
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
---
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
[PATCH v3 14/21] dyndbg: accept query terms like file=bar and module=foo
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
---
.../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
* line - // 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 \n");
- return -EINVAL;
- }
+ char *keyword, *arg;
if (modname)
/* support $modname.dyndbg= */
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
[PATCH v3 11/21] dyndbg: use gcc ?: to reduce word count
reduce word count via gcc ?: extension, no actual code change.
Signed-off-by: Jim Cromie
---
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
[PATCH v3 06/21] dyndbg: fix overcounting of ram used by dyndbg
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
---
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
[PATCH v3 09/21] dyndbg: prefer declarative init in caller, to memset in callee
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
---
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 \n");
return -EINVAL;
}
- memset(query, 0, sizeof(*query));
if (modname)
/* support $modname.dyndbg= */
@@ -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
[PATCH v3 10/21] dyndbg: make ddebug_tables list LIFO for add/remove_module
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
---
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
[PATCH v3 18/21] dyndbg: add filter channel to the internals
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
---
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
[PATCH v3 16/21] dyndbg: combine flags & mask into a struct, simplify with it
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
---
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(c
[PATCH v3 14/21] dyndbg: accept query terms like module:foo and file=bar
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
---
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
* line - // 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 \n");
- return -EINVAL;
- }
+ char *keyword, *arg;
if (modname)
/* support $modname.dyndbg= */
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
[PATCH v3 08/21] dyndbg: fix pr_err with empty string
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
---
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
[PATCH v3 05/21] dyndbg: rename __verbose section to __dyndbg
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
---
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
-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_
[PATCH v3 03/21] dyndbg: drop obsolete comment on ddebug_proc_open
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
---
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 /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
[PATCH v3 00/21] dynamic_debug cleanups, query features, export
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/20200613155738.2249399-1-jim.cro...@gmail.com/ v1: https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cro...@gmail.com/ 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
[PATCH v3 01/21] dyndbg-docs: eschew file /full/path query in docs
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
---
.../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 /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 != "=_"' /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
Re: [PATCH v2 19/24] dyndbg: accept query terms like module:foo and file=bar
hi Petr > You made to do some research and I was wrong. For example, getopt() > operates with options and their arguments. So, 'keyword' and 'arg' names > look good after all. > > Well, I still think that only one syntax should be supported. And it > is better to distinguish keywords and arguments, so I prefer keyword=arg. > hehe, Im gonna cite some RFC wisdom to convince you ;-) Be strict in what you emit, and permissive in what you accept. I see no potential for real ambiguity that would override that bias. thanks jimc
Re: [PATCH v2 20/24] dyndbg: WIP towards debug-print-class based callsite controls
On Wed, Jun 17, 2020 at 3:52 AM Petr Mladek wrote: > > On Wed 2020-06-17 10:31:54, Daniel Thompson wrote: > > On Tue, Jun 16, 2020 at 02:05:27PM -0700, Joe Perches wrote: > > > On Tue, 2020-06-16 at 15:45 +0200, Petr Mladek wrote: > > > > On Sat 2020-06-13 09:57:34, Jim Cromie wrote: > > > > > There are *lots* of ad-hoc debug printing solutions in kernel, > > > > > this is a 1st attempt at providing a common mechanism for many of > > > > > them. > > > > > > > > I agree that it might make sense to provide some common mechanism. > > > [] > > > > My problem with this approach is that it is too generic. Each class > > > > would have different meaning in each subsystem. > > > > > > > > It might help to replace any existing variants. But it would be hard > > > > for developers debugging the code. They would need to study/remember > > > > the meaning of these groups for particular subsystems. They would > > > > need to set different values for different messages. > > > > > > > > Could you please provide more details about the potential users? > > > > Would be possible to find some common patterns and common > > > > meaning of the groups? > > > > > > I doubt the utility of common patterns. > > > Verbosity is common but groupings are not. > > > > > > Look at the DRM patterns vs other groups. > > > > I've seen drm.debug mentioned a couple of times but the comments about > > it seem to only learn part of what is shows us. > > > > drm.debug is a form of common grouping but it acts at a sub-system level > > rather then whole system (and gives a whole sub-system enable/disable). > > This is where grouping makes most sense. > > > > The result is that drm.debug is easy to document, both in official > > kernel docs and in other resources (like the arch distro documentation). > > Having controls that are easy to document makes them easy to find and > > thus sub-system grouping leads directly to higher quality bug reports. > > Thanks a lot for explanation. > > Now, could anyone please tell me how this new dynamic debug feature > would allow to replace drm.debug option? > > I mean what steps/commands will be needed instead of, for example > drm.debug=0x3 command line option? > > Best Regards, > Petr [jimc@frodo linux.git]$ git log -1 commit 12a67ffb3e63c40027e251b44b2abc77463dc2da (HEAD -> dd-v3) Author: Jim Cromie Date: Tue Jun 16 15:36:37 2020 -0600 dyndbg: export ddebug_exec_queries Exporting ddebug_exec_queries will allow module authors using dynamic debug to actually control/enable/disable their own pr-debug callsites dynamically. With it, module authors can tie any update of their internal debug variables to a corresponding ddebug_exec_queries invocation, which will modify all their selected callsites accordingly. Generally, authors would exec +p or -p on some subsets of their set of callsites, and leave fmlt flags for user to choose the appropriate amount of structural context desired in the logs. Depending upon the user needs, the module might be known, and therefore a waste of screen width, function would be valuable, file would be long and familiar to the author, etc.. That said, author could harness the message-prefix facility if they saw fit to do so. Any author preferences can be overridden with echo >control Is it safe ? ddebug_exec_queries() is currently 'exposed' to user space in several limited ways; 1 it is called from module-load callback, where it implements the $modname.dyndbg=+p "fake" parameter provided to all modules. 2 it handles query input from >control directly IOW, it is "fully" exposed to local root user; exposing the same functionality to other kernel modules is no additional risk. The other big issue to check is locking: dyndbg has a single mutex, taken by ddebug_change to handle >control, and by ddebug_proc_(start|stop) to span `cat control`. ISTM this proposed export presents no locking problems. drm use case: drm.debug=0x03 appears to be a kernel boot-arg example, setting 2 internal debug flags. Each bit is probably controlling a separate subset of all debug-prints, they may be overlapping subsets. Those subsets are *definitely* expressible as a few dyndbg queries each. Any arbitrary subset is. drm.dyndbg='file drm_gem_* +p' # gem debug drm.dyndbg='file *_gem_* +p' # *gem debug With this proposed export, drm authors could exec these examples, most likely in the callback that handles updates to the drm.debug variable. (END) I should note that none of this needs the WIP patch
Re: [PATCH v2 20/24] dyndbg: WIP towards debug-print-class based callsite controls
> Or that meaning could be handled by merely issuing the fill-in activations. > In the module that wants debug levels > > echo module foo mflags 4 >control > auto generates same query 3 more times, with mflags 3 flags 2 mflags:1 > let me also note that just because a module might do the descending loop N..1 to implement debug levels, doesnt mean you cant then override manually using echo >control to say disable N=3 but keep 4,5, except 4,5 in file x youre in >control ;-) > > > > > > Best Regards, > > Petr
Re: [PATCH v2 20/24] dyndbg: WIP towards debug-print-class based callsite controls
hi Petr,
On Tue, Jun 16, 2020 at 7:45 AM Petr Mladek wrote:
>
> On Sat 2020-06-13 09:57:34, Jim Cromie wrote:
> > There are *lots* of ad-hoc debug printing solutions in kernel,
> > this is a 1st attempt at providing a common mechanism for many of them.
>
> I agree that it might make sense to provide some common mechanism.
>
>
> > Basically, there are 2 styles of debug printing:
> > - levels, with increasing verbosity, 1-10 forex
> > - bits/flags, independently controlling separate groups of dprints
> >
> > This patch does bits/flags only.
> >
> > proposed API:
> >
> > Usage model is for a module developer to create N exclusive subsets of
> > pr_debug()s by changing some of them to pr_debug_n(1,) .. pr_debug_n(N,).
> > Each callsite must be a single print-class, with 0 default.
> >
> > No multi-type classification ala pr_debug_M(1|2, ...) is contemplated.
> >
> > Qfoo() { echo module foo $* >/proc/dynamic_debug/control }
> > Qfoo +p # all groups, including default 0
> > Qfoo mflags 1 +p# only group 1
> > Qfoo mflags 12 +p # TBD[1]: groups 1 or 2
> > Qfoo mflags 0 +p# ignored atm TBD[2]
> > Qfoo mflags af +p # TBD[3]: groups a or f (10 or 15)
>
> My problem with this approach is that it is too generic. Each class
> would have different meaning in each subsystem.
>
I think generic is a feature.
subsystem and module are the organizational level
where print-classes would be sensibly defined.
Thats why I required a module query-term with mflags,
so that no mflags query could operate on all the callsites.
I might go further to prohibit a wildcard in the module query-term value,
so its absolutely locked in to a specific module.
maybe there would be consensus about having 1 or 2 kernel-wide print-classes,
but other than something akin to stdin, stdout, stderr, I cant think
of what it would look like.
> It might help to replace any existing variants. But it would be hard
> for developers debugging the code. They would need to study/remember
> the meaning of these groups for particular subsystems. They would
> need to set different values for different messages.
>
> Could you please provide more details about the potential users?
Ive ccd Stanimir, who wanted HI MID LOW classifications on some of his
debug prints.
Im doing the simplest possible thing that might work for him.
> Would be possible to find some common patterns and common
> meaning of the groups?
probably should start with anti-patterns.
KISAP - simple as possible
I offer exclusive classes only, no this or that.
prclass = 0 is reserved for every current use.
If module authors think they need many print-classes, rethink.
control output exposes whole structure of code,
file and function names are chosen to convey organization,
a small set of queries will recreate most arbitrary print-classes
use the echo >control interface only.
I briefly thought about checking module debug parameters,
that now sounds like madness
Any chunk of code with N pr-debug* callsites, however badly architected,
can be completely controlled by N separate queries.
for better code, its much smaller.
ISTM the only sane way to allow external modules to control their own
dynamic debug callsites is to expose ddebug_exec_queries,
and let them issue the callsite modifications they want.
then, they can map any updates to their debug-flags storage
onto a pair of on-off queries for each bit.
Lastly, Id note that exclusive classes doesnt mean levels (debug++, debug--)
cant be handled.
we could reserve pr-classes 1-9 to mean all inclusive below N.
Or that meaning could be handled by merely issuing the fill-in activations.
In the module that wants debug levels
echo module foo mflags 4 >control
auto generates same query 3 more times, with mflags 3 flags 2 mflags:1
>
> Best Regards,
> Petr
Re: [PATCH v2 19/24] dyndbg: accept query terms like module:foo and file=bar
On Tue, Jun 16, 2020 at 5:57 AM Petr Mladek wrote: > > On Sat 2020-06-13 09:57:33, Jim Cromie wrote: > > Current code expects "keyword" "arg" as 2 space separated words. > > Change to also accept "keyword:arg" and "keyword=arg" forms as well, > > and drop !(nwords%2) requirement. > > > > Then in rest of function, use new keyword,arg variables instead of > > word[i],word[i+1] > > I like the idea. But please allow only one form. IMHO, parameter=value > is a common way to pass values to commandline parameters. > I dont see a basis to prefer one over the other. we already now accept " file foo.c:func " that might argue for file=foo:func but file:foo:func is what youd expect reading left-to-right > Note that "keyword" and "arg" is strange naming, especially "arg". > I think keyword is clear in context. query_term is suitable, but no better. arg is pretty generic, without overloaded meaning like value ( like lvalue ? rvalue ?) almost as old as 'i', but generally a string (not an int) Is there an alternative you favor ? > Best Regards, > Petr
Re: [PATCH v2 13/24] dyndbg: combine flags & mask into a struct, use that
On Mon, Jun 15, 2020 at 9:14 AM Petr Mladek wrote:
>
> On Sat 2020-06-13 09:57:27, Jim Cromie wrote:
> > combine flags & mask into a struct, and replace those 2 parameters in
> > 3 functions: ddebug_change, ddebug_parse_flags, ddebug_read_flags,
> > altering the derefs in them accordingly.
> >
> > This simplifies the 3 function sigs, preparing for more changes.
> > We dont yet need mask from ddebug_read_flags, but will soon.
> > ---
> > lib/dynamic_debug.c | 46 +++--
> > 1 file changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 93c627e9c094..8dc073a6e8a4 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -62,6 +62,11 @@ struct ddebug_iter {
> > unsigned int idx;
> > };
> >
> > +struct flagsettings {
>
> Please. use underscore to help with parsing such a long names.
> I mean use: flags_settings.
>
ok
> > + unsigned int flags;
> > + unsigned int mask;
> > +};
>
> static int ddebug_change(const struct ddebug_query *query,
> > - unsigned int pflags, unsigned int mask)
> > + struct flagsettings *mods)
>
> > -static int ddebug_read_flags(const char *str, unsigned int *flags)
> > +static int ddebug_read_flags(const char *str, struct flagsettings *f)
>
> > -static int ddebug_parse_flags(const char *str, unsigned int *flagsp,
> > -unsigned int *maskp)
> > +static int ddebug_parse_flags(const char *str, struct flagsettings *mods)
>
> What "mods" stands for, please?
>
modifying_flags, or modifiers.
the original flags & mask bundled together
ie the pfmlt in
echo +pfmlt > control
> I have to say that using a parameter called "mods" in a function
> called parse_flags() is inconsistent and confusing.
>
does the above help ?
I could go with modifying_flags
keep in mind the name has to suit all + - = operations
I'll review all the new names. I recall you didnt like filterflags either,
so I wasnt sufficently clear there either.
Im mulling a better explanation.
> Best Regards,
> Petr
Re: [PATCH v2 10/24] dyndbg: refactor parse_linerange out of ddebug_parse_query
On Mon, Jun 15, 2020 at 7:37 AM Petr Mladek wrote: > > On Sat 2020-06-13 09:57:24, Jim Cromie wrote: > > make the code-block reusable to later handle "file foo.c:101-200" etc. > > > This should be a 90%+ code-move, with minimal adaptations; reindent, > > and scafolding. > > This sentence sounds like the author did some hidden > microoptimizations and potentially broke the code. > It made me nervous. > > But in fact, I do not see any real change except that the variable > "first" does not longer need to be defined. So, it is just a code move. > > In this case, I usually write: > > This patch does not change the existing behavior. I see your point. it was code move, reindent, add function wrapper, add call, compile I just dont recall if I had to touch anything else, add/move var decls etc.
Re: [PATCH v2 23/24] kset-example: add pr_debug()s for easy visibility of its operation
On Sun, Jun 14, 2020 at 12:05 AM Greg KH wrote:
>
> On Sat, Jun 13, 2020 at 09:57:37AM -0600, Jim Cromie wrote:
> > put pr_debug()s into most functions, to easily see code operate when
> > module is loaded and used.
> >
> > #> dmesg -w &
> > #> modprobe kset-example dyndbg=+pfml
> > #> cat /sys/kernel/kset-example/*/*
> > ---
> > static int __init example_init(void)
> > {
> > + pr_debug("called");
>
> Why??? If you want to do something like this, use ftrace, that is what
> it is for.
>
> thanks,
>
> greg k-h
mostly I needed an easy place to try out pr_debug_n in the next patch.
if that next patch seems like a good anti-pattern for pr_debug_n use/misuse,
then I could combine the 2, and add a 'dont do this, use ftrace' comment too.
or not, of course.
Re: [PATCH v2 07/24] dyndbg: fix a BUG_ON in ddebug_describe_flags
On Mon, Jun 15, 2020 at 7:20 AM Petr Mladek wrote:
>
> On Sat 2020-06-13 09:57:21, Jim Cromie wrote:
> In all patches is missing:
>
> Signed-off-by: Jim Cromie
right, I missed the -s invoking format-patch, v3 will have them
>
> > ---
> > lib/dynamic_debug.c | 31 +++
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 9b2445507988..aaace13d7536 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -87,22 +87,22 @@ static struct { unsigned flag:8; char opt_char; }
> > opt_array[] = {
> > { _DPRINTK_FLAGS_NONE, '_' },
> > };
> >
> > +struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
>
> This looks too complicated. What about?
>
> typedef char flags_buf[ARRAY_SIZE(opt_array) + 1];
> used as
> flags_buf fb;
>
I used the struct to give type safety.
old code passed a pointer to a string, and hoped it was big enough.
then used a BUG_ON to insist.
passing (the address of the) struct means the contained string is
known big enough.
and the addy is also that of the string itself (member offset 0), no overhead.
>
> #define FLAGS_BUF_SIZE (ARRAY_SIZE(opt_array) + 1)
> used as
> char flags_buf[FLAGS_BUF_SIZE];
>
I never needed that constant, cuz the string is filled once,
in the function just below the struct def, using the same expression (sans +1)
I would/will update the 1-line comment on ddebug_describe_flags
and add another on the struct itself, once I figure out how to say
all of this succinctly and clearly enough.
Im open to suggestions.
thanks
jimc
>
> Best Regards,
> Petr
Re: [PATCH v2 09/24] dyndbg: add maybe(str,"") macro to reduce code
On Mon, Jun 15, 2020 at 7:28 AM Petr Mladek wrote: > > On Sat 2020-06-13 09:57:23, Jim Cromie wrote: > From my POV this makes the code less readable. Open coding is much > more clear than an ambiguous macro name. > > Best Regards, > Petr Im going with Joes approach, which addresses your concerns too thanks
