[RFC PATCH v4 19/19] dyndbg: RFC add linker rules to module.lds.h
Copy the DYNAMIC_DEBUG_DATA macro, that works in vmlinux.lds.h,
into module.lds.h This does not work here
The point of the KEEPS is to pack section pairs into consecutive
memory, a property we need in order to drop the _ddebug.site pointer.
The problem (ISTM) is that for linking vmlinux, the data is linked
into .data (and the sections are subsumed), and it is is accessed by
iterating beteween __(fstart|stop)___dyndbg(_sites)? symbols. That
breaks down here cuz kernel/module.c specifically grabs the __dyndb*
sections by name.
I lack the linker-fu to sort this, so I left my commented out
attempts, to show my errors.
no-bisect: expect linker error
Signed-off-by: Jim Cromie
---
include/asm-generic/module.lds.h | 21 +
1 file changed, 21 insertions(+)
diff --git a/include/asm-generic/module.lds.h b/include/asm-generic/module.lds.h
index f210d5c1b78b..0074c5f2421b 100644
--- a/include/asm-generic/module.lds.h
+++ b/include/asm-generic/module.lds.h
@@ -7,4 +7,25 @@
* Empty for the asm-generic header.
*/
+/* implement dynamic printk debug section packing */
+#if defined(CONFIG_DYNAMIC_DEBUG) || \
+ (defined(CONFIG_DYNAMIC_DEBUG_CORE) \
+&& defined(DYNAMIC_DEBUG_MODULE))
+#define DYNAMIC_DEBUG_DATA() \
+ . = ALIGN(8); \
+ KEEP(*(__dyndbg_sites .gnu.linkonce.dyndbg_site)) \
+ KEEP(*(__dyndbg .gnu.linkonce.dyndbg))
+#else
+#define DYNAMIC_DEBUG_DATA()
+#endif
+
+SECTIONS {
+__dyndbg : { (*(__dyndbg .gnu.linkonce.dyndbg)) }
+__dyndbg_sites : { (*(__dyndbg_sites .gnu.linkonce.dyndbg_site)) }
+
+ //.data.dyndbg : { DYNAMIC_DEBUG_DATA() } // syntax ok
+ //: { DYNAMIC_DEBUG_DATA() }
+ //DYNAMIC_DEBUG_DATA()
+}
+
#endif /* __ASM_GENERIC_MODULE_LDS_H */
--
2.29.2
[RFC PATCH v4 16/19] dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE
The next patch adds DEFINE_DYNAMIC_DEBUG_TABLE(), which breaks some subtrees with special compile constraints (efi etc). Avoid this by adding a define to suppress the *remote declaration* done by DEFINE_DYNAMIC_DEBUG_TABLE(), automatically, on behalf of all possible users of pr_debug. Signed-off-by: Jim Cromie --- arch/x86/boot/compressed/Makefile | 1 + arch/x86/entry/vdso/Makefile | 3 +++ arch/x86/purgatory/Makefile | 1 + drivers/firmware/efi/libstub/Makefile | 3 ++- 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index e0bc3988c3fa..ada4eb960d95 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -31,6 +31,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \ KBUILD_CFLAGS := -m$(BITS) -O2 KBUILD_CFLAGS += -fno-strict-aliasing -fPIE KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE cflags-$(CONFIG_X86_32) := -march=i386 cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone KBUILD_CFLAGS += $(cflags-y) diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile index 05c4abc2fdfd..619878f2c427 100644 --- a/arch/x86/entry/vdso/Makefile +++ b/arch/x86/entry/vdso/Makefile @@ -29,6 +29,9 @@ vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o vobjs32-y += vdso32/vclock_gettime.o vobjs-$(CONFIG_X86_SGX)+= vsgx.o +# avoid a x86_64_RELATIVE error +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE + # files to link into kernel obj-y += vma.o extable.o KASAN_SANITIZE_vma.o := y diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 95ea17a9d20c..95ba7b18410f 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -35,6 +35,7 @@ PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0 PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING PURGATORY_CFLAGS += -fno-stack-protector +PURGATORY_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE # Default KBUILD_CFLAGS can have -pg option set when FTRACE is enabled. That # in turn leaves some undefined symbols like __fentry__ in purgatory and not diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index c23466e05e60..def8febefbd3 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -13,7 +13,8 @@ cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ \ -Wno-pointer-sign \ $(call cc-disable-warning, address-of-packed-member) \ $(call cc-disable-warning, gnu) \ - -fno-asynchronous-unwind-tables + -fno-asynchronous-unwind-tables \ + -DNO_DYNAMIC_DEBUG_TABLE # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly # disable the stackleak plugin -- 2.29.2
[RFC PATCH v4 18/19] dyndbg: shuffle ddebug_table fields
In preparation to unionize structs _ddebug & ddebug_table, shuffle
fields in latter so they match the layout of the former. This MAY
simplify initialization of the header field, in particular by
preserving *sites.
It also sets up a later conversion to a flex-array ddebugs[].
This step is mostly to isolate/prove no breakage before HEAD++
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 29 +
lib/dynamic_debug.c | 2 +-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 229cfd81ffb3..22f11218047f 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -66,6 +66,35 @@ struct _ddebug {
#endif
} __aligned(8);
+/* this pair of header structs correspond quite deeply to struct
+ * _ddebug(|_site)s above; they are also created into __dyndbg*
+ * sections (by DEFINE_DYNAMIC_DEBUG_TABLE), and should be unionized
+ * with them to reinforce this.
+ *
+ * struct _ddebug_header is the important one, it has enough space to
+ * take struct ddebug_table's job, ie: link together into a list, and
+ * keep track of the modname & _ddebug(|_sites) vectors.
+ *
+ * Its other job is handled by its placement in the front of a
+ * module's _ddebug[N] entries. Each _ddebug knows its N, so the
+ * header's address is computable, and its site pointer is available
+ * to get _ddebug_sites[N]. Then we can drop _ddebug.sites, regaining
+ * parity with original _ddebug footprint.
+ *
+ * Eventually, N will index a fetch from a compressed block, or for
+ * enabled callsites, a hash. A global hash is probably adequate, if
+ * ~5k elements doesnt degrade access time.
+ */
+struct _ddebug_site_header {
+ /* we have 24 bytes total here, all currently unused */
+} __aligned(8);
+
+struct _ddebug_header {
+ struct _ddebug_site* sites;
+ struct list_head link;
+ const char* mod_name;
+ unsigned num_ddebugs;
+} __aligned(8);
#if defined(CONFIG_DYNAMIC_DEBUG_CORE)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c1a7345277eb..5d1ce7f21c30 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -45,11 +45,11 @@ extern struct _ddebug_site __start___dyndbg_sites[];
extern struct _ddebug_site __stop___dyndbg_sites[];
struct ddebug_table {
+ struct _ddebug_site *sites;
struct list_head link;
const char *mod_name;
unsigned int num_ddebugs;
struct _ddebug *ddebugs;
- struct _ddebug_site *sites;
};
struct ddebug_query {
--
2.29.2
[RFC PATCH v4 05/19] dyndbg: hoist ->site out of ddebug_match_site
A coming change adds _get/_put abstraction on the site pointer, to
allow managing site info more flexibly. The get/put pattern is best
done at a single lexical scope, where its more obviously correct, so
hoist the ->site ref out of ddebug_match_site, and pass it in instead.
no functional changes
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index da732e0c56e3..abe3382aabd5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -143,10 +143,9 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
}
static int ddebug_match_site(const struct ddebug_query *query,
-const struct _ddebug *dp)
+const struct _ddebug *dp,
+const struct _ddebug_site *dc)
{
- struct _ddebug_site *dc;
-
/* match against the format */
if (query->format) {
if (*query->format == '^') {
@@ -167,7 +166,6 @@ static int ddebug_match_site(const struct ddebug_query
*query,
dp->lineno > query->last_lineno)
return false;
- dc = dp->site;
if (!dc) {
/* site info has been dropped, so query cannot test these
fields */
if (query->filename || query->function)
@@ -219,9 +217,9 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_site *dc;
+ struct _ddebug_site *dc = dp->site;
- if (!ddebug_match_site(query, dp))
+ if (!ddebug_match_site(query, dp, dc))
continue;
nfound++;
--
2.29.2
[RFC PATCH v4 06/19] dyndbg: accept null site in ddebug_change
fix a debug-print that includes site info, by adding an alternate
debug message that does not.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index abe3382aabd5..151e55ab6bb5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -235,10 +235,17 @@ static int ddebug_change(const struct ddebug_query *query,
static_branch_enable(>key.dd_key_true);
#endif
dp->flags = newflags;
- v2pr_info("changed %s:%d [%s]%s =%s\n",
-trim_prefix(dc->filename), dp->lineno,
-dt->mod_name, dc->function,
-ddebug_describe_flags(dp->flags, ));
+
+ if (dc)
+ v2pr_info("changed %s:%d [%s]%s =%s\n",
+ trim_prefix(dc->filename), dp->lineno,
+ dt->mod_name, dc->function,
+ ddebug_describe_flags(dp->flags,
));
+ else
+ v2pr_info("changed %s:%d =%s \"%s\"\n",
+ dt->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags,
),
+ dp->format);
}
}
mutex_unlock(_lock);
--
2.29.2
[RFC PATCH v4 08/19] dyndbg: accept null site in ddebug_proc_show
Accept a ddebug record with a null site pointer, and write abbreviated
output for that record that doesn't include site info (but does
include line-number, since that can be used in >control queries).
Also add a 2nd header line with a template for the new output.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0c535f3c2ba9..1f0cac4a66af 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -918,18 +918,27 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
if (p == SEQ_START_TOKEN) {
seq_puts(m,
-"# filename:lineno [module]function flags format\n");
+"#: filename:lineno [module]function flags format\n"
+"#| [module]:lineno flags format\n"
+ );
return 0;
}
dc = dp->site;
-
- seq_printf(m, "%s:%u [%s]%s =%s \"",
- trim_prefix(dc->filename), dp->lineno,
- iter->table->mod_name, dc->function,
- ddebug_describe_flags(dp->flags, ));
- seq_escape(m, dp->format, "\t\r\n\"");
- seq_puts(m, "\"\n");
+ if (dc) {
+ seq_printf(m, "%s:%u [%s]%s =%s \"",
+ trim_prefix(dc->filename), dp->lineno,
+ iter->table->mod_name, dc->function,
+ ddebug_describe_flags(dp->flags, ));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ } else {
+ seq_printf(m, "[%s]:%u =%s \"",
+ iter->table->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags, ));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ }
return 0;
}
--
2.29.2
[RFC PATCH v4 15/19] dyndbg: add _index to struct _ddebug
We currently use dp->site to map: &__dyndbg[N] -> &__dyndbg_sites[N].
We want to drop site, new _ddebug._index provides the N.
The mapping is done in ddebug_site_get():
For builtin modules, a _ddebug *ptr is between __start___dyndbg and
__stop___dyndbg, and we can use &__start___dyndbg_sites[N] directly.
For loadable modules, we still need work, so we print rubbish, and
just return site pointer (which is correct).
ddebug_add_module() handles _index initialization:
Its new task is to number each module consecutively, so it gets new
base arg to pass the next starting index.
To actually drop site, We need both the module's __dyndbg* section
addys, and we need their relative placement to have a base-to-base
offset.
PLAN - a table header connecting 2 tables.
- ddebug_table points to both __dyndbgs & __dyndbg_sites.
but *ddebugs & *sites are independent.
no path from ddebugs[n] -> ddebug_sites[n]
If we have a header record in-situ, which keeps the site pointer we
seek to eliminate from _ddebug, and its in element[0] of both vectors,
we can go:
ddebugs[n] -> ddebugs[0] -> containerof -> site[n]
union ddebug_table_header {
struct ddebug_table *owner;
struct _ddebug item;
}
and
struct ddebug_table_vector {
struct ddebug_table *owner;
struct _ddebug vector[];
}
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 2 ++
lib/dynamic_debug.c | 40 +--
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 7d33475d226a..18689db0e2c0 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -29,6 +29,7 @@ struct _ddebug {
/* format is always needed, lineno shares word with flags */
const char *format;
const unsigned lineno:18;
+ unsigned _index:14;
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -56,6 +57,7 @@ struct _ddebug {
#define _DPRINTK_FLAGS_DEFAULT 0
#endif
unsigned int flags:8;
+
#ifdef CONFIG_JUMP_LABEL
union {
struct static_key_true dd_key_true;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 1f59407b6a83..54737647556c 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -123,6 +123,8 @@ do {
\
#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
#define v2pr_info(fmt, ...)vnpr_info(2, fmt, ##__VA_ARGS__)
#define v3pr_info(fmt, ...)vnpr_info(3, fmt, ##__VA_ARGS__)
+#define v4pr_info(fmt, ...)vnpr_info(4, fmt, ##__VA_ARGS__)
+#define v5pr_info(fmt, ...)vnpr_info(5, fmt, ##__VA_ARGS__)
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
@@ -146,7 +148,17 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
{
- return dp->site; /* passthru abstraction */
+ v4pr_info("get %d: %s.%s.%d\n", dp->_index, dp->site->modname,
+ dp->site->function, dp->lineno);
+
+ if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
+ v4pr_info(" is builtin: %d %ld\n", dp->_index, dp -
__start___dyndbg);
+ return &__start___dyndbg_sites[ dp - __start___dyndbg ];
+ } else {
+ v3pr_info(" is loaded: %d %ld\n", dp->_index, dp -
__start___dyndbg);
+ return dp->site;
+ }
+ return dp->site;
}
static inline void ddebug_site_put(struct _ddebug *dp)
{
@@ -1034,10 +1046,12 @@ static const struct proc_ops proc_fops = {
* Allocate a new ddebug_table for the given module
* and add it to the global list.
*/
-int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
- unsigned int numdbgs, const char *modname)
+static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+ unsigned numdbgs, unsigned base,
+ const char *modname)
{
struct ddebug_table *dt;
+ int i;
dt = kzalloc(sizeof(*dt), GFP_KERNEL);
if (dt == NULL) {
@@ -1055,6 +1069,13 @@ int ddebug_add_module(struct _ddebug *tab, struct
_ddebug_site *sites,
dt->ddebugs = tab;
dt->sites = sites;
+ v3pr_info("add-module: %s\n", modname);
+ for (i = 0; i < numdbgs; i++) {
+ tab[i]._index = base++;
+ v3pr_info(" %d %d %s.%s.%d\n", i, tab[i]._index, modname,
+ tab[i].site->function, tab[i].lineno);
+ }
+
mutex_lock(_lock);
list_add(>link, _tables);
mutex_unlock(_lock);
@@ -10
[RFC PATCH v4 17/19] dyndbg: RFC - DECLARE/DEFINE_DYNAMIC_DEBUG_TABLE
V4-proto - now currently diet-3i
Simplify:
.gnu.linkonce._mod_.dyndbg -> .gnu.linkonce.dyndbg
ie drop _mod_
This puts a single header record at front of the vectors, solving 2
problems (discussed below) simultaneously:
- header in front, allowing flex-array rep of layout.
- single header, not per-module. adequate for needs, no wasted space.
this now appears to work
- get: dp range-check good on builtin, !builtin.
todo:
- _sym_##_site symbols in init/main.i still busted
- _index init in __ddebug_add_module
- cleanup commentary below.
DEFINE_DYNAMIC_DEBUG_TABLE is based on DECLARE_DYNAMIC_DEBUG_METADATA.
Like its model, it creates/allocates a pair of structs: _ddebug &
_ddebug_site, and inits them distinctively.
Its purpose is to create an in-situ module-header-pair for each
module's sub-vectors of _ddebug[], _ddebug_sites[]. Once the
header-pair is reliably linked into the vectors, code can get from
_ddebug[N] to the header-pair, then to _ddebug_sites[N] at runtime by
saving N into _ddebug._index during init. With this, we can drop the
site pointer in struct _ddebug.
Eventually, this module-header-pair can be adapted to be an in-situ
replacement for the separately allocated struct ddebug_tables, and be
linked into the ddebug_tables list.
RFC, NOTES:
DYNAMIC_DEBUG is a 'transparent' facility, in that uses of pr_debug
get extra features without additional api. Because of this,
DEFINE_DYNAMIC_DEBUG_TABLE is 'transparently' invoked by
dynamic_debug.h on behalf of all its (indirect) users, including
printk.h.
IOW this has wide effects; it results in multiple redundant
declarations of header records, even single object files may get
multiple copies. Using .gnu.linkonce._mod_.dyndbg(|_site) section
names and "__used __weak " seems to resolve the redundancies. I
havent tried with clang.
In vmlinux-lds.h, the 2 KEEPs are modified to append those 2 new
header sections to their respective existing __dyndbg* sections, in an
interleaved manner. This places the header records immediately after
the modules' block of _ddebug*s, in a knowable offset from &__dyndbg[0].
scripts/Makefile.lib gets a new -DKBUILD_MODSYM defn, with a value
like KBUILD_MODNAME, except that its not __stringified. It is used to
create a pair of module-ish named variables: _sym_##_dyndbg_base.
For some non-obvious reason, the substitution doesnt work, resulting
in per-module symbol names like KBUILD_MODSYM_dyndbg_base. This
subtly alters the header initialization and is_dyndbg_header_pair(),
which is used in __init to find the headers adjacent to each modules'
block of _ddebug records. This isnt fatal to the plan; we just need
the storage reserved where its accessible by known offset from the
_ddebug[N] record of an enabled pr-debug. But it would be nice to
have the symbol names consistent with the intent. I looked for a
MODULE_SINGLETON(name_suffix) to use, similarly to how UNIQUE_ID is
used to construct names, but found nothing.
The .gnu.linkonce._mod_. works to eliminate all the extra headers in
each module, but a problem remains; it adds unneeded headers for
modules with zero pr-debugs. So Im seeing ~1500 extra headers.
I tried several flavors of conditional linking, now think I want/need
a linker-script language extension:
KEEP ( *(__dyndbg) AND *(.gnu.linkonce.dyndbg) ) # my need
KEEP ( *(foo_tbl) OR *(.gnu.linkonce.foo_alt) ) # for completeness
I've managed to alter ld's grammar, but its only compile tested, and
is missing the conditional linking pieces. Since this patchset's
value proposition is a memory shrink, ~1500 extra headers is fatal,
and this patchset must be a slow-cook.
V4 todo:
Time to simplify, drop _mod_, and change _ddebug.module_index to
._index, indicating that its no longer keyed to module, but to the
whole compilation unit, which for the kernel includes all the builtin
modules. loadable modules should get their own. tbt.
this could obsolete all the above problems.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/i915_drv.c | 2 +
include/asm-generic/vmlinux.lds.h | 27 +---
include/linux/dynamic_debug.h | 100 +-
lib/dynamic_debug.c | 27 +---
scripts/Makefile.lib | 2 +
5 files changed, 139 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8e9cb44e66e5..51163e0d40cf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -91,6 +91,8 @@
static const struct drm_driver driver;
+//DEFINE_DYNAMIC_DEBUG_TABLE();
+
static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
{
int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 4f2af9de2f03..482d94078785 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -335,6 +335,22 @@
[RFC PATCH v4 14/19] dyndbg: add ddebug_site(_get|_put) abstraction
Replace direct ->site refs with _get(),_put() internal API. Right
now, _get() just returns ->site and _put() does nothing. Later we can
replace that implementation with one using ->module_index to fetch
then forget site data dynamically.
Several approaches are possible:
A: !site -> fill from backing store
1st try at this is/was using zram. At init, it copied each callsite
into a zs-allocation, and all site-> refs afterward went thru
_get/_put to zs-map on demand, and zs-unmap the site info. This
worked until I tried to keep callsites mapped while they're enabled,
when it gave lockdep warns/panics. IIRC theres a zram patchset doing
something with locking; I need to retry this approach, even if other
options are better, this might be a validating use case.
B: block store
Another approach is to compress the new linker section, using some
algorithm thats good at indexed decompression. I probed this
approach, using objcopy, unsuccessfully:
objcopy --dump-section __dyndbg=dd \
--dump-section __dyndbg_sites=ddsites $IMG
>From vmlinux.o dumps were mostly empty (pre-link/reloc data?)
and vmlinux didnt have the section.
C: callsite composed from __dyndbg[N] & __dyndbg_site[N]
We know _ddebug records are in a vector, either in the builtin
__dyndbg linker section, or the same from a modprobed one. The
builtin section has all builtin module sub-sections catenated
dogether.
At init, we iterate over the section, and "parse it" by creating a
ddebug_table for each module with prdebugs. ddebug_table.num_debugs
remembers the size of each modules' vector of prdebugs.
We need a few things:
- _ddebug.index field, which knows offset to start of this sub-vector.
this new field will be "free" because the struct has padding.
it can be initialized during init, then RO.
- a back-pointer at the beginning of the sub-vector, to the
ddebug_table "owning" (but not containing) this sub-vector of
prdebugs.
If we had both, we could get from the ddebug element to its vector
root, back up to the owning ddebug_table, then down to the _callsite
vector, and index to the right element. While slower than a pointer
deref, this is a cold path, and it allows elimination of the
per-callsite pointer member, thus greater density of the sections, and
still can support sparse site info.
That back-pointer feels tricky. It needs to be 1st in the sub-vector
D: (C1?) add a header record to each sub-vector
If we can insert a header record into each modules' __dyndbg* section
sub-vectors, we can simplify the cold path above; a single sites*
pointer in the header can give us access to __dyndbg_sites[N]
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index be2e97ae2da9..1f59407b6a83 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -144,6 +144,14 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
+{
+ return dp->site; /* passthru abstraction */
+}
+static inline void ddebug_site_put(struct _ddebug *dp)
+{
+}
+
static int ddebug_match_site(const struct ddebug_query *query,
const struct _ddebug *dp,
const struct _ddebug_site *dc)
@@ -239,16 +247,18 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_site *dc = dp->site;
+ struct _ddebug_site *dc;
+
+ dc = ddebug_site_get(dp);
if (!ddebug_match_site(query, dp, dc))
- continue;
+ goto skipsite;
nfound++;
newflags = (dp->flags & modifiers->mask) |
modifiers->flags;
if (newflags == dp->flags)
- continue;
+ goto skipsite;
ddebug_alter_site(dp, modifiers);
@@ -264,6 +274,9 @@ static int ddebug_change(const struct ddebug_query *query,
dt->mod_name, dp->lineno,
ddebug_describe_flags(dp->flags,
),
dp->format);
+
+ skipsite:
+ ddebug_site_put(dp);
}
}
mutex_unlock(_lock);
@@ -633,11 +646,11 @@ static int remaining(int wrote)
return 0;
}
-static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
+static char *__dynamic_emit_prefix(s
[RFC PATCH v4 13/19] dyndbg+module: expose ddebug_sites to modules
In order to drop the pointer connecting _ddebug's to _ddebug_sites,
we need to elevate the latter; we need to track it in (internal)
ddebug_tables, and set it in ddebug_add_module. That last part
exposes it by interface to module.c, so we add a field to load_info,
and adjust load_module to initialize it from the elf section.
Its possible that this closes a "hole" created when __dyndbg_sites
section was added, in that the section wasn't "managed" directly, and
could conceivably get lost later. I never saw any misbehavior loading
i915.ko into a vm, but still..
TBD/RFC:
these 2 vectors should be in a single struct. if this struct can have
ddebugs[], ie a flex-array, then container_of can get us to the
sites*, yielding _sites[N], and allowing to drop _ddebug.site
The trouble with this is that ddebugs* now points to someone elses
memory, and we cant just steal it and stomp on the memory just in
front of it (for the sites ptr).
rename n to numdbgs
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 4 ++--
kernel/module-internal.h | 1 +
kernel/module.c | 9 ++---
lib/dynamic_debug.c | 20
4 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index d811273c8484..7d33475d226a 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -70,8 +70,8 @@ struct _ddebug {
/* 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);
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+ unsigned int numdbgs, const char *modname);
extern int ddebug_remove_module(const char *mod_name);
extern __printf(2, 3)
void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 33783abc377b..fb61eb2f8032 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -18,6 +18,7 @@ struct load_info {
char *secstrings, *strtab;
unsigned long symoffs, stroffs, init_typeoffs, core_typeoffs;
struct _ddebug *debug;
+ struct _ddebug_site *sites;
unsigned int num_debug;
bool sig_ok;
#ifdef CONFIG_KALLSYMS
diff --git a/kernel/module.c b/kernel/module.c
index 30479355ab85..792903230acd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2780,11 +2780,12 @@ static void add_kallsyms(struct module *mod, const
struct load_info *info)
}
#endif /* CONFIG_KALLSYMS */
-static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
unsigned int num)
+static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
+ struct _ddebug_site *sites, unsigned int num)
{
if (!debug)
return;
- ddebug_add_module(debug, num, mod->name);
+ ddebug_add_module(debug, sites, num, mod->name);
}
static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
@@ -,6 +3334,8 @@ static int find_module_sections(struct module *mod,
struct load_info *info)
info->debug = section_objs(info, "__dyndbg",
sizeof(*info->debug), >num_debug);
+ info->sites = section_objs(info, "__dyndbg_sites",
+ sizeof(*info->sites), >num_debug);
return 0;
}
@@ -4004,7 +4007,7 @@ static int load_module(struct load_info *info, const char
__user *uargs,
goto free_arch_cleanup;
}
- dynamic_debug_setup(mod, info->debug, info->num_debug);
+ dynamic_debug_setup(mod, info->debug, info->sites, info->num_debug);
/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
ftrace_module_init(mod);
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fdc2d0e15731..be2e97ae2da9 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -49,6 +49,7 @@ struct ddebug_table {
const char *mod_name;
unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct _ddebug_site *sites;
};
struct ddebug_query {
@@ -1014,14 +1015,14 @@ static const struct proc_ops proc_fops = {
* Allocate a new ddebug_table for the given module
* and add it to the global list.
*/
-int ddebug_add_module(struct _ddebug *tab, unsigned int n,
-const char *name)
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+ unsigned int numdbgs, const char *modname)
{
struct ddebug_table *dt;
dt = kzalloc(sizeof(*dt), GFP_KERNEL);
if (dt == NULL) {
- pr_err("error adding module: %s\n", name);
[RFC PATCH v4 12/19] dyndbg: allow deleting site info via control interface
Allow users & subsystems to selectively delete callsite info for
pr-debug callsites. Hopefully, this can lead to actual recovery of
memory.
DRM is a potential user which would drop the sites:
- has distinct categories for logging, and can map them over to a
format prefix, like: "drm:core:", "drm:kms:", etc.
- are happy with group control of all the callsites in a class/cateory.
individual control is still possible using queries including line numbers
- don't need dynamic "module:function:line:" prefixes in log messages
- don't care about loss of context in /proc/dynamic_debug/control
before:
init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed
data\012"
init/main.c:1337 [main]run_init_process =pm "%s\012"
init/main.c:1335 [main]run_init_process =pm " with environment:\012"
init/main.c:1334 [main]run_init_process =pm "%s\012"
init/main.c:1332 [main]run_init_process =pm " with arguments:\012"
init/main.c:1121 [main]initcall_blacklisted =pm "initcall %s blacklisted\012"
init/main.c:1082 [main]initcall_blacklist =pm "blacklisting initcall %s\012"
then:
bash-5.0# echo file init/main.c +D > /proc/dynamic_debug/control
after:
init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed
data\012"
[main]:1337 =pmD "%s\012"
[main]:1335 =pmD " with environment:\012"
[main]:1334 =pmD "%s\012"
[main]:1332 =pmD " with arguments:\012"
[main]:1121 =pmD "initcall %s blacklisted\012"
[main]:1082 =pmD "blacklisting initcall %s\012"
Notes:
If Drm adopted dyndbg, i915 + drm* would add ~1600 prdebugs, amdgpu +
drm* would add ~3200 callsites, so the additional memory costs are
substantial. In trade, drm and drivers would avoid lots of calls to
drm_debug_enabled(). This patch should reduce the costs.
Using this interface, drm could drop site info for all categories /
prefixes controlled by bits in drm.debug, while preserving site info
and individual selectivity for any uncategorized prdebugs, and for all
other modules.
Lastly, because lineno field was not moved into _ddebug_callsite, it
can be used to modify a single[*] callsite even if drm has dropped all
the callsite data:
echo module $mod format ^$prefix line $line +p >control
Dropping site info is a one-way, information losing operation, so
minor misuse is possible. Worst case is maybe (depending upon
previous settings) some loss of logging context/decorations.
echo +D > /proc/dynamic_debug/control
[*] amdgpu has some macros invoking clusters of pr_debugs; each use of
them creates a cluster of pr-debugs with the same line number.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 9 +
2 files changed, 10 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa4a3f5d8d35..d811273c8484 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -40,6 +40,7 @@ struct _ddebug {
#define _DPRINTK_FLAGS_INCL_FUNCNAME (1<<2)
#define _DPRINTK_FLAGS_INCL_LINENO (1<<3)
#define _DPRINTK_FLAGS_INCL_TID(1<<4)
+#define _DPRINTK_FLAGS_DELETE_SITE (1<<7) /* drop site info to save ram */
#define _DPRINTK_FLAGS_INCL_ANYSITE\
(_DPRINTK_FLAGS_INCL_MODNAME\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 6fbd9099c2fa..fdc2d0e15731 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -92,6 +92,7 @@ static struct { unsigned flag:8; char opt_char; } opt_array[]
= {
{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
{ _DPRINTK_FLAGS_INCL_TID, 't' },
{ _DPRINTK_FLAGS_NONE, '_' },
+ { _DPRINTK_FLAGS_DELETE_SITE, 'D' },
};
struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
@@ -201,6 +202,14 @@ static void ddebug_alter_site(struct _ddebug *dp,
} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(>key.dd_key_true);
#endif
+ /* delete site info for this callsite */
+ if (modifiers->flags & _DPRINTK_FLAGS_DELETE_SITE) {
+ if (dp->site) {
+ vpr_info("dropping site info %s.%s.%d\n",
dp->site->filename,
+ dp->site->function, dp->lineno);
+ dp->site = NULL;
+ }
+ }
}
/*
--
2.29.2
[RFC PATCH v4 11/19] dyndbg: refactor ddebug_alter_site out of ddebug_change
Move the JUMP_LABEL/static-key code to a separate function.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2d011ac3308d..6fbd9099c2fa 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -191,6 +191,18 @@ static int ddebug_match_site(const struct ddebug_query
*query,
return true;
}
+static void ddebug_alter_site(struct _ddebug *dp,
+ struct flag_settings *modifiers)
+{
+#ifdef CONFIG_JUMP_LABEL
+ if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+ if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
+ static_branch_disable(>key.dd_key_true);
+ } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
+ static_branch_enable(>key.dd_key_true);
+#endif
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -227,13 +239,9 @@ static int ddebug_change(const struct ddebug_query *query,
newflags = (dp->flags & modifiers->mask) |
modifiers->flags;
if (newflags == dp->flags)
continue;
-#ifdef CONFIG_JUMP_LABEL
- if (dp->flags & _DPRINTK_FLAGS_PRINT) {
- if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
-
static_branch_disable(>key.dd_key_true);
- } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
- static_branch_enable(>key.dd_key_true);
-#endif
+
+ ddebug_alter_site(dp, modifiers);
+
dp->flags = newflags;
if (dc)
--
2.29.2
[RFC PATCH v4 09/19] dyndbg: optimize ddebug_emit_prefix
Add early return if no callsite info is specified in site-flags.
This avoids fetching site info that isn't going to be printed.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 9 +
lib/dynamic_debug.c | 3 +++
2 files changed, 12 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index bc8027292c02..aa4a3f5d8d35 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -40,6 +40,15 @@ 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_INCL_ANYSITE\
+ (_DPRINTK_FLAGS_INCL_MODNAME\
+| _DPRINTK_FLAGS_INCL_FUNCNAME \
+| _DPRINTK_FLAGS_INCL_LINENO)
+#define _DPRINTK_FLAGS_INCL_ANY\
+ (_DPRINTK_FLAGS_INCL_ANYSITE\
+| _DPRINTK_FLAGS_INCL_TID)
+
#if defined DEBUG
#define _DPRINTK_FLAGS_DEFAULT _DPRINTK_FLAGS_PRINT
#else
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 1f0cac4a66af..af9cf97f869b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -632,6 +632,9 @@ static char *dynamic_emit_prefix(const struct _ddebug *dp,
char *buf)
}
pos_after_tid = pos;
+ if (!(dp->flags & _DPRINTK_FLAGS_INCL_ANYSITE))
+ return buf;
+
if (desc) {
if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
--
2.29.2
[RFC PATCH v4 10/19] dyndbg: avoid calling dyndbg_emit_prefix when it has no work
Wrap function in a static-inline one, which checks flags to avoid
calling the function unnecessarily.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index af9cf97f869b..2d011ac3308d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -615,7 +615,7 @@ static int remaining(int wrote)
return 0;
}
-static char *dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
+static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
{
int pos_after_tid;
int pos = 0;
@@ -655,6 +655,13 @@ static char *dynamic_emit_prefix(const struct _ddebug *dp,
char *buf)
return buf;
}
+static inline char *dynamic_emit_prefix(struct _ddebug *dp, char *buf)
+{
+ if (unlikely(dp->flags & _DPRINTK_FLAGS_INCL_ANY))
+ return __dynamic_emit_prefix(dp, buf);
+ return buf;
+}
+
void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
{
va_list args;
--
2.29.2
[RFC PATCH v4 07/19] dyndbg: accept null site in dynamic_emit_prefix
2 prints use site->member, protect them with if site.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 151e55ab6bb5..0c535f3c2ba9 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -631,15 +631,19 @@ static char *dynamic_emit_prefix(const struct _ddebug
*dp, char *buf)
task_pid_vnr(current));
}
pos_after_tid = pos;
- if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
- pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->modname);
- if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
- pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->function);
+
+ if (desc) {
+ if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+ pos += snprintf(buf + pos, remaining(pos), "%s:",
+ desc->modname);
+ if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+ pos += snprintf(buf + pos, remaining(pos), "%s:",
+ desc->function);
+ }
if (dp->flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
dp->lineno);
+
if (pos - pos_after_tid)
pos += snprintf(buf + pos, remaining(pos), " ");
if (pos >= PREFIX_SIZE)
--
2.29.2
[RFC PATCH v4 04/19] dyndbg: accept null site in ddebug_match_site
basically, reorder the elements to minimize data fetches.
1- move format and line-number check code to the top of the function,
since they don't use/check site info.
2- test site pointer:
If its null, we return early, skipping 3:
If the query tests against missing site info, fail the match.
otherwize site matches.
3- rest of function (checking site vs query) is unchanged.
ddebug_match_site ignores module, because it's tested already
by the caller, where it is known from debug_tables.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 41 +
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9cff9db15937..da732e0c56e3 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -145,21 +145,7 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
static int ddebug_match_site(const struct ddebug_query *query,
const struct _ddebug *dp)
{
- struct _ddebug_site *dc = dp->site;
-
- /* match against the source filename */
- if (query->filename &&
- !match_wildcard(query->filename, dc->filename) &&
- !match_wildcard(query->filename,
- kbasename(dc->filename)) &&
- !match_wildcard(query->filename,
- trim_prefix(dc->filename)))
- return false;
-
- /* match against the function */
- if (query->function &&
- !match_wildcard(query->function, dc->function))
- return false;
+ struct _ddebug_site *dc;
/* match against the format */
if (query->format) {
@@ -181,6 +167,29 @@ static int ddebug_match_site(const struct ddebug_query
*query,
dp->lineno > query->last_lineno)
return false;
+ dc = dp->site;
+ if (!dc) {
+ /* site info has been dropped, so query cannot test these
fields */
+ if (query->filename || query->function)
+ return false;
+ else
+ return true;
+ }
+
+ /* match against the source filename */
+ if (query->filename &&
+ !match_wildcard(query->filename, dc->filename) &&
+ !match_wildcard(query->filename,
+ kbasename(dc->filename)) &&
+ !match_wildcard(query->filename,
+ trim_prefix(dc->filename)))
+ return false;
+
+ /* match against the function */
+ if (query->function &&
+ !match_wildcard(query->function, dc->function))
+ return false;
+
return true;
}
@@ -210,7 +219,7 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_site *dc = dp->site;
+ struct _ddebug_site *dc;
if (!ddebug_match_site(query, dp))
continue;
--
2.29.2
[RFC PATCH v4 02/19] dyndbg: __init iterate over __dyndbg & __dyndbg_site in parallel
In dynamic_debug_init(), rework for-loop; add 2nd 'site' var, and
iterate over both __dyndbg* sections in parallel. Replace uses of
iter->site with the new 'site' iter, add a BUG_ON to enforce the
invariant given by HEAD~1's DECLARE_DYNAMIC_DEBUG_METADATA base->site
initialization.
0- declare the new elf section start/stop, named in vmlinux.lds.h
I disregarded a checkpatch warning about externs in c-files, stuck
with current practice.
1- clean up use of 4 iterators for clarity:
(iter, site), and ((iter, site)_mod_start) block markers.
2- iterate over __dyndbg_sites in parallel with __dyndbg
s/iter->site/site/g;
3- add BUG_ON(iter->site != site)
DECLARE_DYNAMIC_DEBUG_METADATA + linker insure this now.
Maybe we can drop pointer, still get order.
4- var rename n to site_ct
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 738c4ce28046..c3c35dcc6a59 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -41,6 +41,8 @@
extern struct _ddebug __start___dyndbg[];
extern struct _ddebug __stop___dyndbg[];
+extern struct _ddebug_site __start___dyndbg_sites[];
+extern struct _ddebug_site __stop___dyndbg_sites[];
struct ddebug_table {
struct list_head link;
@@ -118,6 +120,7 @@ do {
\
#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
#define v2pr_info(fmt, ...)vnpr_info(2, fmt, ##__VA_ARGS__)
+#define v3pr_info(fmt, ...)vnpr_info(3, fmt, ##__VA_ARGS__)
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
@@ -1082,11 +1085,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_mod_start;
+ struct _ddebug_site *site, *site_mod_start;
const char *modname = NULL;
char *cmdline;
int ret = 0;
- int n = 0, entries = 0, modct = 0;
+ int site_ct = 0, entries = 0, modct = 0;
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1097,23 +1101,29 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
return 0;
}
- iter = __start___dyndbg;
- modname = iter->site->modname;
- iter_start = iter;
- for (; iter < __stop___dyndbg; iter++) {
+
+ iter = iter_mod_start = __start___dyndbg;
+ site = site_mod_start = __start___dyndbg_sites;
+ modname = site->modname;
+
+ for (; iter < __stop___dyndbg; iter++, site++) {
+
+ BUG_ON(site != iter->site);
entries++;
- if (strcmp(modname, iter->site->modname)) {
+
+ if (strcmp(modname, site->modname)) {
modct++;
- ret = ddebug_add_module(iter_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, site_ct,
modname);
if (ret)
goto out_err;
- n = 0;
- modname = iter->site->modname;
- iter_start = iter;
+ site_ct = 0;
+ modname = site->modname;
+ iter_mod_start = iter;
+ site_mod_start = site;
}
- n++;
+ site_ct++;
}
- ret = ddebug_add_module(iter_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, site_ct, modname);
if (ret)
goto out_err;
--
2.29.2
[RFC PATCH v4 03/19] dyndbg: refactor part of ddebug_change to ddebug_match_site
Move all the site-match logic into a separate function, reindent the
code, and replace the continues with return falses.
No functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 75 ++---
1 file changed, 43 insertions(+), 32 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c3c35dcc6a59..9cff9db15937 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -142,6 +142,48 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+static int ddebug_match_site(const struct ddebug_query *query,
+const struct _ddebug *dp)
+{
+ struct _ddebug_site *dc = dp->site;
+
+ /* match against the source filename */
+ if (query->filename &&
+ !match_wildcard(query->filename, dc->filename) &&
+ !match_wildcard(query->filename,
+ kbasename(dc->filename)) &&
+ !match_wildcard(query->filename,
+ trim_prefix(dc->filename)))
+ return false;
+
+ /* match against the function */
+ if (query->function &&
+ !match_wildcard(query->function, dc->function))
+ return false;
+
+ /* match against the format */
+ 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)
+ return false;
+ } else if (!strstr(dp->format, query->format))
+ return false;
+ }
+
+ /* match against the line number range */
+ if (query->first_lineno &&
+ dp->lineno < query->first_lineno)
+ return false;
+ if (query->last_lineno &&
+ dp->lineno > query->last_lineno)
+ return false;
+
+ return true;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -170,38 +212,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct _ddebug *dp = >ddebugs[i];
struct _ddebug_site *dc = dp->site;
- /* match against the source filename */
- if (query->filename &&
- !match_wildcard(query->filename, dc->filename) &&
- !match_wildcard(query->filename,
- kbasename(dc->filename)) &&
- !match_wildcard(query->filename,
- trim_prefix(dc->filename)))
- continue;
-
- /* match against the function */
- if (query->function &&
- !match_wildcard(query->function, dc->function))
- continue;
-
- /* match against the format */
- 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 &&
- dp->lineno < query->first_lineno)
- continue;
- if (query->last_lineno &&
- dp->lineno > query->last_lineno)
+ if (!ddebug_match_site(query, dp))
continue;
nfound++;
--
2.29.2
[RFC PATCH v4 00/19] dynamic debug diet plan
v4 fixes 'series grooming errors',
Reported-by: kernel test robot
CONFIG_DYNAMIC_DEBUG creates a struct _ddebug (56 bytes) for each
callsite, which includes 3 pointers to: module, filename, function.
These are repetetive, and compressible, this patch series goes about
doing that, it:
- splits struct _ddebug and __dyndbg[] section/array into 2
creating struct _ddebug_site and __dyndbg_sites[]
temporary _ddebug.site connects them.
- makes _ddebug_site data optional
- minor optimizations
- makes _ddebug_site data deleteable
not necessary, proof of optionality
The RFC stuff comes at the end:
- attach __dyndbg_sites[] to module-info, like __dyndbg[]
- add index to struct _ddebug, use it for builtins
- add ddebug_site(_get|_put) abstraction to hide _ddebug.site
At this point, its practical to: compress __dyndbg_sites[] & replace
the section, then expand it on-demand to serve ddebug_site_get()
calls. For enabled callsites (with decorations flags), the retrieved
records can be cached in a hash after theyre 1st needed/used (when
callsite is actually executed).
Whats my (ideal?) decompressing interface ?
And whats the name of the api call ? I couldnt suss it out yet.
For any control read, a simple block decompress and cache is close to
ideal; all site data is then available to iterate over, and each
ddebug_site_get() just indexes into it. A stream of decompressed site
records would also work well, with less lumpy memory allocs and frees.
Actually dropping _ddebug.site is not yet possible. While we could
drop it for builtin modules, thats cuz we know __start___dyndbg_sites.
For loaded modules, I need the elf section: __dyndbg_sites. This is
in load-info, but I dont have a path to it. In:
- add _ddebug_header/table
That adds a single header entry pair (structs _ddebug*s with special
initialization) into the arrays, and it links to the front of the
array, where its useful. But creating this header entry only works
for vmlinux itself (because of vmlinux.ld.h patch), not for loadable
modules.
- add linker rules to module.lds.h
I tried to re-use the DYMAMIC_DEBUG_DATA macro added above for
modules, this failed, commit msg guesses the cause. This breaks with
a linker script syntax error
TODO
Presuming the fixed header can be linked reliably in front doing
something like I tried, it can be recast as a ddebug_header and
unionized with struct _ddebug, and the _ddebugs[] will fit nicely as a
flex-array:
struct ddebug_table2 {
struct ddebug_header foo;
struct _ddebug ddebugs[];
}
A header would have 40 bytes, room to contain most/all of struct
ddebug_table's fields, a pointer to the __dyndbg_sites[] table, and a
list-head too, meaning it supports essential and nice-to-have
properties:
- the mapping: __dyndbg[N] --> __dyndbg_sites[N] # NEEDed to drop .site
using container_of_flex() for flex-arrays
- we can add them directly to ddebug_tables list # freebie
ie avoid the kzalloc in ddebug_add_module()
If not all fields fit in the space available in __dyndbg[0], there is
space available in __dyndbg_sites[0].
Additionally, at the end of __init, ddebug_tables list is composed of
memory entirely in __dyndbg[], which could then be make readonly (by
means I dont know). If this breaks insertions of loadable modules, we
can easily a 2nd list for that.
Jim Cromie (19):
dyndbg: split struct _ddebug, move display fields to new _ddebug_site
dyndbg: __init iterate over __dyndbg & __dyndbg_site in parallel
dyndbg: refactor part of ddebug_change to ddebug_match_site
dyndbg: accept null site in ddebug_match_site
dyndbg: hoist ->site out of ddebug_match_site
dyndbg: accept null site in ddebug_change
dyndbg: accept null site in dynamic_emit_prefix
dyndbg: accept null site in ddebug_proc_show
dyndbg: optimize ddebug_emit_prefix
dyndbg: avoid calling dyndbg_emit_prefix when it has no work
dyndbg: refactor ddebug_alter_site out of ddebug_change
dyndbg: allow deleting site info via control interface
dyndbg+module: expose ddebug_sites to modules
dyndbg: add ddebug_site(_get|_put) abstraction
dyndbg: add _index to struct _ddebug
dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE
dyndbg: RFC - DECLARE/DEFINE_DYNAMIC_DEBUG_TABLE
dyndbg: shuffle ddebug_table fields
dyndbg: RFC add linker rules to module.lds.h
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/entry/vdso/Makefile | 3 +
arch/x86/purgatory/Makefile | 1 +
drivers/firmware/efi/libstub/Makefile | 3 +-
drivers/gpu/drm/i915/i915_drv.c | 2 +
include/asm-generic/module.lds.h | 21 ++
include/asm-generic/vmlinux.lds.h | 24 +-
include/linux/dynamic_debug.h | 180 +--
kernel/module-internal.h | 1 +
kernel/module.c | 9 +-
lib/dynamic_debug.c | 313 +++---
scripts/Makefile.lib | 2 +
12 files changed,
[RFC PATCH v4 01/19] dyndbg: split struct _ddebug, move display fields to new _ddebug_site
struct _ddebug has 2 flavors of fields: essential and optional. Split
the 3 optional fields: module, function, file into a new struct
_ddebug_site, and add pointer to it from _ddebug.
These fields are optional in that they are primarily used to generate
the optional "module:func:line" log prefix. They're also used to
select callsites to >control. lineno is arguably optional too, but
leaving it uses spare bytes in struct _ddebug.
The new ptr increases memory footprint by 1 ptr per pr_debug, but I
think its temporary, and the indirection gives several advantages:
- we can drop sites and their storage opportunistically.
this reduces per-site mem by 24/64.
Subsystems may not need/want "module:func:line:" in their logs.
If they already use format-prefixes such as "drm:kms:",
they can select on those, and don't need the site info for that.
forex:
#> echo module drm format "^drm:kms: " +p >control
ie: dynamic_debug_exec_queries("format '^drm:kms: '", "drm");
- the moved display fields are inherently hierarchical, and the linker
section is ordered; so (module, file, function) have repeating
values (90%, 85%, 45%). This is readily compressible, even with a
simple field-wise run length encoding. Since I'm splitting the struct,
I also reordered the fields to match the hierarchy.
- the separate linker section sets up naturally for block compression.
IFF we can on-demand map: ddebugs[N] -> ddebug_sites[N]
- we can compress __dyndbg_sites during __init, and mark section __initdata
- can decompress on-demand, say for `cat control`
- can save chunks of decompressed buffer for enabled callsites
- free chunks on site disable, or on memory pressure.
Whats actually done here is rather mechanical, and preparatory.
dynamic_debug.h:
I cut struct _ddebug in half, renamed optional top-half to
_ddebug_site, kept __align(8) for both halves. I added a forward decl
for a unified comment for both head & body, and added head.site to
point at body.
DECLARE_DYNAMIC_DEBUG_METADATA does the core of the work; it declares
and initializes both static struct variables together, and refs one to
the other.
dynamic_debug.c:
dynamic_debug_init() mem-usage now also counts sites.
dynamic_emit_prefix() & ddebug_change() use those moved fields; they
get a new initialized auto-var, and the field refs get adjusted as
needed to follow the move from one struct to the other.
struct _ddebug_site *dc = dp->site;
ddebug_proc_show() differs slightly; it assigns to (not initializes)
the autovar, to avoid a panic when p == SEQ_START_TOKEN.
vmlinux.lds.h:
add __dyndbg_sites section, with the same align(8) and KEEP as
used in the __dyndbg section.
Signed-off-by: Jim Cromie
---
include/asm-generic/vmlinux.lds.h | 3 ++
include/linux/dynamic_debug.h | 37 -
lib/dynamic_debug.c | 46 +--
3 files changed, 53 insertions(+), 33 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 0331d5d49551..4f2af9de2f03 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -353,6 +353,9 @@
*(__tracepoints)\
/* implement dynamic printk debug */\
. = ALIGN(8); \
+ __start___dyndbg_sites = .; \
+ KEEP(*(__dyndbg_sites)) \
+ __stop___dyndbg_sites = .; \
__start___dyndbg = .; \
KEEP(*(__dyndbg)) \
__stop___dyndbg = .;\
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index a57ee75342cf..bc8027292c02 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -7,20 +7,28 @@
#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_sites) for every dynamic debug callsite.
+ * At runtime, the sections are treated as arrays.
*/
-struct _ddebug {
+struct _ddebug;
+struct _ddebug_site {
/*
-* These fields are used to drive the user interface
-* for selecting and displaying debug callsites.
+* These fields (and lineno) are used to:
+* - decorate log messages per site flags
+* - select callsites for modification via >control
+* - display callsites & settings in `cat control`
*/
const char *modname;
- const char *function;
[RFC PATCH v3 18/18] dyndbg: shuffle ddebug_table fields
In preparation to unionize structs _ddebug & ddebug_table, shuffle
fields in latter so they match the layout of the former. This MAY
simplify initialization of the header field, in particular by
preserving *sites.
It also sets up a later conversion to a flex-array ddebugs[].
This step is mostly to isolate/prove no breakage before HEAD++
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 29 +
lib/dynamic_debug.c | 2 +-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 229cfd81ffb3..22f11218047f 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -66,6 +66,35 @@ struct _ddebug {
#endif
} __aligned(8);
+/* this pair of header structs correspond quite deeply to struct
+ * _ddebug(|_site)s above; they are also created into __dyndbg*
+ * sections (by DEFINE_DYNAMIC_DEBUG_TABLE), and should be unionized
+ * with them to reinforce this.
+ *
+ * struct _ddebug_header is the important one, it has enough space to
+ * take struct ddebug_table's job, ie: link together into a list, and
+ * keep track of the modname & _ddebug(|_sites) vectors.
+ *
+ * Its other job is handled by its placement in the front of a
+ * module's _ddebug[N] entries. Each _ddebug knows its N, so the
+ * header's address is computable, and its site pointer is available
+ * to get _ddebug_sites[N]. Then we can drop _ddebug.sites, regaining
+ * parity with original _ddebug footprint.
+ *
+ * Eventually, N will index a fetch from a compressed block, or for
+ * enabled callsites, a hash. A global hash is probably adequate, if
+ * ~5k elements doesnt degrade access time.
+ */
+struct _ddebug_site_header {
+ /* we have 24 bytes total here, all currently unused */
+} __aligned(8);
+
+struct _ddebug_header {
+ struct _ddebug_site* sites;
+ struct list_head link;
+ const char* mod_name;
+ unsigned num_ddebugs;
+} __aligned(8);
#if defined(CONFIG_DYNAMIC_DEBUG_CORE)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c1a7345277eb..5d1ce7f21c30 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -45,11 +45,11 @@ extern struct _ddebug_site __start___dyndbg_sites[];
extern struct _ddebug_site __stop___dyndbg_sites[];
struct ddebug_table {
+ struct _ddebug_site *sites;
struct list_head link;
const char *mod_name;
unsigned int num_ddebugs;
struct _ddebug *ddebugs;
- struct _ddebug_site *sites;
};
struct ddebug_query {
--
2.29.2
[RFC PATCH v3 17/18] dyndbg: RFC - DECLARE/DEFINE_DYNAMIC_DEBUG_TABLE
V4-proto - now currently diet-3i
Simplify:
.gnu.linkonce._mod_.dyndbg -> .gnu.linkonce.dyndbg
ie drop _mod_
This puts a single header record at front of the vectors, solving 2
problems (discussed below) simultaneously:
- header in front, allowing flex-array rep of layout.
- single header, not per-module. adequate for needs, no wasted space.
this now appears to work
- get: dp range-check good on builtin, !builtin.
todo:
- _sym_##_site symbols in init/main.i still busted
- _index init in __ddebug_add_module
- cleanup commentary below.
DEFINE_DYNAMIC_DEBUG_TABLE is based on DECLARE_DYNAMIC_DEBUG_METADATA.
Like its model, it creates/allocates a pair of structs: _ddebug &
_ddebug_site, and inits them distinctively.
Its purpose is to create an in-situ module-header-pair for each
module's sub-vectors of _ddebug[], _ddebug_sites[]. Once the
header-pair is reliably linked into the vectors, code can get from
_ddebug[N] to the header-pair, then to _ddebug_sites[N] at runtime by
saving N into _ddebug._index during init. With this, we can drop the
site pointer in struct _ddebug.
Eventually, this module-header-pair can be adapted to be an in-situ
replacement for the separately allocated struct ddebug_tables, and be
linked into the ddebug_tables list.
RFC, NOTES:
DYNAMIC_DEBUG is a 'transparent' facility, in that uses of pr_debug
get extra features without additional api. Because of this,
DEFINE_DYNAMIC_DEBUG_TABLE is 'transparently' invoked by
dynamic_debug.h on behalf of all its (indirect) users, including
printk.h.
IOW this has wide effects; it results in multiple redundant
declarations of header records, even single object files may get
multiple copies. Using .gnu.linkonce._mod_.dyndbg(|_site) section
names and "__used __weak " seems to resolve the redundancies. I
havent tried with clang.
In vmlinux-lds.h, the 2 KEEPs are modified to append those 2 new
header sections to their respective existing __dyndbg* sections, in an
interleaved manner. This places the header records immediately after
the modules' block of _ddebug*s, in a knowable offset from &__dyndbg[0].
scripts/Makefile.lib gets a new -DKBUILD_MODSYM defn, with a value
like KBUILD_MODNAME, except that its not __stringified. It is used to
create a pair of module-ish named variables: _sym_##_dyndbg_base.
For some non-obvious reason, the substitution doesnt work, resulting
in per-module symbol names like KBUILD_MODSYM_dyndbg_base. This
subtly alters the header initialization and is_dyndbg_header_pair(),
which is used in __init to find the headers adjacent to each modules'
block of _ddebug records. This isnt fatal to the plan; we just need
the storage reserved where its accessible by known offset from the
_ddebug[N] record of an enabled pr-debug. But it would be nice to
have the symbol names consistent with the intent. I looked for a
MODULE_SINGLETON(name_suffix) to use, similarly to how UNIQUE_ID is
used to construct names, but found nothing.
The .gnu.linkonce._mod_. works to eliminate all the extra headers in
each module, but a problem remains; it adds unneeded headers for
modules with zero pr-debugs. So Im seeing ~1500 extra headers.
I tried several flavors of conditional linking, now think I want/need
a linker-script language extension:
KEEP ( *(__dyndbg) AND *(.gnu.linkonce.dyndbg) ) # my need
KEEP ( *(foo_tbl) OR *(.gnu.linkonce.foo_alt) ) # for completeness
I've managed to alter ld's grammar, but its only compile tested, and
is missing the conditional linking pieces. Since this patchset's
value proposition is a memory shrink, ~1500 extra headers is fatal,
and this patchset must be a slow-cook.
V4 todo:
Time to simplify, drop _mod_, and change _ddebug.module_index to
._index, indicating that its no longer keyed to module, but to the
whole compilation unit, which for the kernel includes all the builtin
modules. loadable modules should get their own. tbt.
this could obsolete all the above problems.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/i915_drv.c | 2 +
include/asm-generic/vmlinux.lds.h | 27 +---
include/linux/dynamic_debug.h | 100 +-
lib/dynamic_debug.c | 28 ++---
scripts/Makefile.lib | 2 +
5 files changed, 139 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8e9cb44e66e5..51163e0d40cf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -91,6 +91,8 @@
static const struct drm_driver driver;
+//DEFINE_DYNAMIC_DEBUG_TABLE();
+
static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
{
int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 4f2af9de2f03..482d94078785 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -335,6 +335,22 @@
[RFC PATCH v3 16/18] dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE
The next patch adds DEFINE_DYNAMIC_DEBUG_TABLE(), which breaks some subtrees with special compile constraints (efi etc). Avoid this by adding a define to suppress the *remote declaration* done by DEFINE_DYNAMIC_DEBUG_TABLE(), automatically, on behalf of all possible users of pr_debug. Signed-off-by: Jim Cromie --- arch/x86/boot/compressed/Makefile | 1 + arch/x86/entry/vdso/Makefile | 3 +++ arch/x86/purgatory/Makefile | 1 + drivers/firmware/efi/libstub/Makefile | 3 ++- 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index e0bc3988c3fa..ada4eb960d95 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -31,6 +31,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \ KBUILD_CFLAGS := -m$(BITS) -O2 KBUILD_CFLAGS += -fno-strict-aliasing -fPIE KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE cflags-$(CONFIG_X86_32) := -march=i386 cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone KBUILD_CFLAGS += $(cflags-y) diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile index 05c4abc2fdfd..619878f2c427 100644 --- a/arch/x86/entry/vdso/Makefile +++ b/arch/x86/entry/vdso/Makefile @@ -29,6 +29,9 @@ vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o vobjs32-y += vdso32/vclock_gettime.o vobjs-$(CONFIG_X86_SGX)+= vsgx.o +# avoid a x86_64_RELATIVE error +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE + # files to link into kernel obj-y += vma.o extable.o KASAN_SANITIZE_vma.o := y diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 95ea17a9d20c..95ba7b18410f 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -35,6 +35,7 @@ PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0 PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING PURGATORY_CFLAGS += -fno-stack-protector +PURGATORY_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE # Default KBUILD_CFLAGS can have -pg option set when FTRACE is enabled. That # in turn leaves some undefined symbols like __fentry__ in purgatory and not diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index c23466e05e60..def8febefbd3 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -13,7 +13,8 @@ cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ \ -Wno-pointer-sign \ $(call cc-disable-warning, address-of-packed-member) \ $(call cc-disable-warning, gnu) \ - -fno-asynchronous-unwind-tables + -fno-asynchronous-unwind-tables \ + -DNO_DYNAMIC_DEBUG_TABLE # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly # disable the stackleak plugin -- 2.29.2
[RFC PATCH v3 13/18] dyndbg+module: expose ddebug_sites to modules
In order to drop the pointer connecting _ddebug's to _ddebug_sites,
we need to elevate the latter; we need to track it in (internal)
ddebug_tables, and set it in ddebug_add_module. That last part
exposes it by interface to module.c, so we add a field to load_info,
and adjust load_module to initialize it from the elf section.
Its possible that this closes a "hole" created when __dyndbg_sites
section was added, in that the section wasn't "managed" directly, and
could conceivably get lost later. I never saw any misbehavior loading
i915.ko into a vm, but still..
TBD/RFC:
these 2 vectors should be in a single struct. if this struct can have
ddebugs[], ie a flex-array, then container_of can get us to the
sites*, yielding _sites[N], and allowing to drop _ddebug.site
The trouble with this is that ddebugs* now points to someone elses
memory, and we cant just steal it and stomp on the memory just in
front of it (for the sites ptr).
rename n to numdbgs
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 4 ++--
kernel/module-internal.h | 1 +
kernel/module.c | 9 ++---
lib/dynamic_debug.c | 18 +++---
4 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index d811273c8484..7d33475d226a 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -70,8 +70,8 @@ struct _ddebug {
/* 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);
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+ unsigned int numdbgs, const char *modname);
extern int ddebug_remove_module(const char *mod_name);
extern __printf(2, 3)
void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 33783abc377b..fb61eb2f8032 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -18,6 +18,7 @@ struct load_info {
char *secstrings, *strtab;
unsigned long symoffs, stroffs, init_typeoffs, core_typeoffs;
struct _ddebug *debug;
+ struct _ddebug_site *sites;
unsigned int num_debug;
bool sig_ok;
#ifdef CONFIG_KALLSYMS
diff --git a/kernel/module.c b/kernel/module.c
index 30479355ab85..792903230acd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2780,11 +2780,12 @@ static void add_kallsyms(struct module *mod, const
struct load_info *info)
}
#endif /* CONFIG_KALLSYMS */
-static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
unsigned int num)
+static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
+ struct _ddebug_site *sites, unsigned int num)
{
if (!debug)
return;
- ddebug_add_module(debug, num, mod->name);
+ ddebug_add_module(debug, sites, num, mod->name);
}
static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
@@ -,6 +3334,8 @@ static int find_module_sections(struct module *mod,
struct load_info *info)
info->debug = section_objs(info, "__dyndbg",
sizeof(*info->debug), >num_debug);
+ info->sites = section_objs(info, "__dyndbg_sites",
+ sizeof(*info->sites), >num_debug);
return 0;
}
@@ -4004,7 +4007,7 @@ static int load_module(struct load_info *info, const char
__user *uargs,
goto free_arch_cleanup;
}
- dynamic_debug_setup(mod, info->debug, info->num_debug);
+ dynamic_debug_setup(mod, info->debug, info->sites, info->num_debug);
/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
ftrace_module_init(mod);
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fdc2d0e15731..5529b17461ae 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -49,6 +49,7 @@ struct ddebug_table {
const char *mod_name;
unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct _ddebug_site *sites;
};
struct ddebug_query {
@@ -1014,8 +1015,8 @@ static const struct proc_ops proc_fops = {
* Allocate a new ddebug_table for the given module
* and add it to the global list.
*/
-int ddebug_add_module(struct _ddebug *tab, unsigned int n,
-const char *name)
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+ unsigned int numdbgs, const char *modname)
{
struct ddebug_table *dt;
@@ -1030,15 +1031,16 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int
n,
* member of struct module, which lives at least as long as
[RFC PATCH v3 09/18] dyndbg: optimize ddebug_emit_prefix
Add early return if no callsite info is specified in site-flags.
This avoids fetching site info that isn't going to be printed.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 9 +
lib/dynamic_debug.c | 3 +++
2 files changed, 12 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index bc8027292c02..aa4a3f5d8d35 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -40,6 +40,15 @@ 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_INCL_ANYSITE\
+ (_DPRINTK_FLAGS_INCL_MODNAME\
+| _DPRINTK_FLAGS_INCL_FUNCNAME \
+| _DPRINTK_FLAGS_INCL_LINENO)
+#define _DPRINTK_FLAGS_INCL_ANY\
+ (_DPRINTK_FLAGS_INCL_ANYSITE\
+| _DPRINTK_FLAGS_INCL_TID)
+
#if defined DEBUG
#define _DPRINTK_FLAGS_DEFAULT _DPRINTK_FLAGS_PRINT
#else
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 1f0cac4a66af..af9cf97f869b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -632,6 +632,9 @@ static char *dynamic_emit_prefix(const struct _ddebug *dp,
char *buf)
}
pos_after_tid = pos;
+ if (!(dp->flags & _DPRINTK_FLAGS_INCL_ANYSITE))
+ return buf;
+
if (desc) {
if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
--
2.29.2
[RFC PATCH v3 14/18] dyndbg: add ddebug_site(_get|_put) abstraction
Replace direct ->site refs with _get(),_put() internal API. Right
now, _get() just returns ->site and _put() does nothing. Later we can
replace that implementation with one using ->module_index to fetch
then forget site data dynamically.
Several approaches are possible:
A: !site -> fill from backing store
1st try at this is/was using zram. At init, it copied each callsite
into a zs-allocation, and all site-> refs afterward went thru
_get/_put to zs-map on demand, and zs-unmap the site info. This
worked until I tried to keep callsites mapped while they're enabled,
when it gave lockdep warns/panics. IIRC theres a zram patchset doing
something with locking; I need to retry this approach, even if other
options are better, this might be a validating use case.
B: block store
Another approach is to compress the new linker section, using some
algorithm thats good at indexed decompression. I probed this
approach, using objcopy, unsuccessfully:
objcopy --dump-section __dyndbg=dd \
--dump-section __dyndbg_sites=ddsites $IMG
>From vmlinux.o dumps were mostly empty (pre-link/reloc data?)
and vmlinux didnt have the section.
C: callsite composed from __dyndbg[N] & __dyndbg_site[N]
We know _ddebug records are in a vector, either in the builtin
__dyndbg linker section, or the same from a modprobed one. The
builtin section has all builtin module sub-sections catenated
dogether.
At init, we iterate over the section, and "parse it" by creating a
ddebug_table for each module with prdebugs. ddebug_table.num_debugs
remembers the size of each modules' vector of prdebugs.
We need a few things:
- _ddebug.index field, which knows offset to start of this sub-vector.
this new field will be "free" because the struct has padding.
it can be initialized during init, then RO.
- a back-pointer at the beginning of the sub-vector, to the
ddebug_table "owning" (but not containing) this sub-vector of
prdebugs.
If we had both, we could get from the ddebug element to its vector
root, back up to the owning ddebug_table, then down to the _callsite
vector, and index to the right element. While slower than a pointer
deref, this is a cold path, and it allows elimination of the
per-callsite pointer member, thus greater density of the sections, and
still can support sparse site info.
That back-pointer feels tricky. It needs to be 1st in the sub-vector
D: (C1?) add a header record to each sub-vector
If we can insert a header record into each modules' __dyndbg* section
sub-vectors, we can simplify the cold path above; a single sites*
pointer in the header can give us access to __dyndbg_sites[N]
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 31 +--
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 5529b17461ae..34329e323ed5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -144,6 +144,14 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
+{
+ return dp->site; /* passthru abstraction */
+}
+static inline void ddebug_site_put(struct _ddebug *dp)
+{
+}
+
static int ddebug_match_site(const struct ddebug_query *query,
const struct _ddebug *dp,
const struct _ddebug_site *dc)
@@ -239,16 +247,18 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_site *dc = dp->site;
+ struct _ddebug_site *dc;
+
+ dc = ddebug_site_get(dp);
if (!ddebug_match_site(query, dp, dc))
- continue;
+ goto skipsite;
nfound++;
newflags = (dp->flags & modifiers->mask) |
modifiers->flags;
if (newflags == dp->flags)
- continue;
+ goto skipsite;
ddebug_alter_site(dp, modifiers);
@@ -264,6 +274,9 @@ static int ddebug_change(const struct ddebug_query *query,
dt->mod_name, dp->lineno,
ddebug_describe_flags(dp->flags,
),
dp->format);
+
+ skipsite:
+ ddebug_site_put(dp);
}
}
mutex_unlock(_lock);
@@ -633,11 +646,11 @@ static int remaining(int wrote)
return 0;
}
-static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
+static char *__dynamic_emit_prefix(s
[RFC PATCH v3 15/18] dyndbg: add _index to struct _ddebug
We currently use dp->site to map: &__dyndbg[N] -> &__dyndbg_sites[N].
We want to drop site, new _ddebug._index provides the N.
The mapping is done in ddebug_site_get():
For builtin modules, a _ddebug *ptr is between __start___dyndbg and
__stop___dyndbg, and we can use &__start___dyndbg_sites[N] directly.
For loadable modules, we still need work, so we print rubbish, and
just return site pointer (which is correct).
ddebug_add_module() handles _index initialization:
Its new task is to number each module consecutively, so it gets new
base arg to pass the next starting index.
To actually drop site, We need both the module's __dyndbg* section
addys, and we need their relative placement to have a base-to-base
offset.
PLAN - a table header connecting 2 tables.
- ddebug_table points to both __dyndbgs & __dyndbg_sites.
but *ddebugs & *sites are independent.
no path from ddebugs[n] -> ddebug_sites[n]
If we have a header record in-situ, which keeps the site pointer we
seek to eliminate from _ddebug, and its in element[0] of both vectors,
we can go:
ddebugs[n] -> ddebugs[0] -> containerof -> site[n]
union ddebug_table_header {
struct ddebug_table *owner;
struct _ddebug item;
}
and
struct ddebug_table_vector {
struct ddebug_table *owner;
struct _ddebug vector[];
}
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 2 ++
lib/dynamic_debug.c | 43 +--
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 7d33475d226a..18689db0e2c0 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -29,6 +29,7 @@ struct _ddebug {
/* format is always needed, lineno shares word with flags */
const char *format;
const unsigned lineno:18;
+ unsigned _index:14;
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -56,6 +57,7 @@ struct _ddebug {
#define _DPRINTK_FLAGS_DEFAULT 0
#endif
unsigned int flags:8;
+
#ifdef CONFIG_JUMP_LABEL
union {
struct static_key_true dd_key_true;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 34329e323ed5..3b53035d63d6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -123,6 +123,8 @@ do {
\
#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
#define v2pr_info(fmt, ...)vnpr_info(2, fmt, ##__VA_ARGS__)
#define v3pr_info(fmt, ...)vnpr_info(3, fmt, ##__VA_ARGS__)
+#define v4pr_info(fmt, ...)vnpr_info(4, fmt, ##__VA_ARGS__)
+#define v5pr_info(fmt, ...)vnpr_info(5, fmt, ##__VA_ARGS__)
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
@@ -146,7 +148,17 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
{
- return dp->site; /* passthru abstraction */
+ v4pr_info("get %d: %s.%s.%d\n", dp->_index, dp->site->modname,
+ dp->site->function, dp->lineno);
+
+ if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
+ v4pr_info(" is builtin: %d %ld\n", dp->_index, dp -
__start___dyndbg);
+ return &__start___dyndbg_sites[ dp - __start___dyndbg ];
+ } else {
+ v3pr_info(" is loaded: %d %ld\n", dp->_index, dp -
__start___dyndbg);
+ return dp->site;
+ }
+ return dp->site;
}
static inline void ddebug_site_put(struct _ddebug *dp)
{
@@ -1034,14 +1046,16 @@ static const struct proc_ops proc_fops = {
* Allocate a new ddebug_table for the given module
* and add it to the global list.
*/
-int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
- unsigned int numdbgs, const char *modname)
+static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+ unsigned numdbgs, unsigned base,
+ const char *modname)
{
struct ddebug_table *dt;
+ int i;
dt = kzalloc(sizeof(*dt), GFP_KERNEL);
if (dt == NULL) {
- pr_err("error adding module: %s\n", name);
+ pr_err("error adding module: %s\n", modname);
return -ENOMEM;
}
/*
@@ -1055,6 +1069,13 @@ int ddebug_add_module(struct _ddebug *tab, struct
_ddebug_site *sites,
dt->ddebugs = tab;
dt->sites = sites;
+ v3pr_info("add-module: %s\n", modname);
+ for (i = 0; i < numdbgs; i++) {
+ tab[i]._index = base++;
+ v3pr_info(" %d %d %s.%s.%d\n&q
[RFC PATCH v3 08/18] dyndbg: accept null site in ddebug_proc_show
Accept a ddebug record with a null site pointer, and write abbreviated
output for that record that doesn't include site info (but does
include line-number, since that can be used in >control queries).
Also add a 2nd header line with a template for the new output.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0c535f3c2ba9..1f0cac4a66af 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -918,18 +918,27 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
if (p == SEQ_START_TOKEN) {
seq_puts(m,
-"# filename:lineno [module]function flags format\n");
+"#: filename:lineno [module]function flags format\n"
+"#| [module]:lineno flags format\n"
+ );
return 0;
}
dc = dp->site;
-
- seq_printf(m, "%s:%u [%s]%s =%s \"",
- trim_prefix(dc->filename), dp->lineno,
- iter->table->mod_name, dc->function,
- ddebug_describe_flags(dp->flags, ));
- seq_escape(m, dp->format, "\t\r\n\"");
- seq_puts(m, "\"\n");
+ if (dc) {
+ seq_printf(m, "%s:%u [%s]%s =%s \"",
+ trim_prefix(dc->filename), dp->lineno,
+ iter->table->mod_name, dc->function,
+ ddebug_describe_flags(dp->flags, ));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ } else {
+ seq_printf(m, "[%s]:%u =%s \"",
+ iter->table->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags, ));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ }
return 0;
}
--
2.29.2
[RFC PATCH v3 12/18] dyndbg: allow deleting site info via control interface
Allow users & subsystems to selectively delete callsite info for
pr-debug callsites. Hopefully, this can lead to actual recovery of
memory.
DRM is a potential user which would drop the sites:
- has distinct categories for logging, and can map them over to a
format prefix, like: "drm:core:", "drm:kms:", etc.
- are happy with group control of all the callsites in a class/cateory.
individual control is still possible using queries including line numbers
- don't need dynamic "module:function:line:" prefixes in log messages
- don't care about loss of context in /proc/dynamic_debug/control
before:
init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed
data\012"
init/main.c:1337 [main]run_init_process =pm "%s\012"
init/main.c:1335 [main]run_init_process =pm " with environment:\012"
init/main.c:1334 [main]run_init_process =pm "%s\012"
init/main.c:1332 [main]run_init_process =pm " with arguments:\012"
init/main.c:1121 [main]initcall_blacklisted =pm "initcall %s blacklisted\012"
init/main.c:1082 [main]initcall_blacklist =pm "blacklisting initcall %s\012"
then:
bash-5.0# echo file init/main.c +D > /proc/dynamic_debug/control
after:
init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed
data\012"
[main]:1337 =pmD "%s\012"
[main]:1335 =pmD " with environment:\012"
[main]:1334 =pmD "%s\012"
[main]:1332 =pmD " with arguments:\012"
[main]:1121 =pmD "initcall %s blacklisted\012"
[main]:1082 =pmD "blacklisting initcall %s\012"
Notes:
If Drm adopted dyndbg, i915 + drm* would add ~1600 prdebugs, amdgpu +
drm* would add ~3200 callsites, so the additional memory costs are
substantial. In trade, drm and drivers would avoid lots of calls to
drm_debug_enabled(). This patch should reduce the costs.
Using this interface, drm could drop site info for all categories /
prefixes controlled by bits in drm.debug, while preserving site info
and individual selectivity for any uncategorized prdebugs, and for all
other modules.
Lastly, because lineno field was not moved into _ddebug_callsite, it
can be used to modify a single[*] callsite even if drm has dropped all
the callsite data:
echo module $mod format ^$prefix line $line +p >control
Dropping site info is a one-way, information losing operation, so
minor misuse is possible. Worst case is maybe (depending upon
previous settings) some loss of logging context/decorations.
echo +D > /proc/dynamic_debug/control
[*] amdgpu has some macros invoking clusters of pr_debugs; each use of
them creates a cluster of pr-debugs with the same line number.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 9 +
2 files changed, 10 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa4a3f5d8d35..d811273c8484 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -40,6 +40,7 @@ struct _ddebug {
#define _DPRINTK_FLAGS_INCL_FUNCNAME (1<<2)
#define _DPRINTK_FLAGS_INCL_LINENO (1<<3)
#define _DPRINTK_FLAGS_INCL_TID(1<<4)
+#define _DPRINTK_FLAGS_DELETE_SITE (1<<7) /* drop site info to save ram */
#define _DPRINTK_FLAGS_INCL_ANYSITE\
(_DPRINTK_FLAGS_INCL_MODNAME\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 6fbd9099c2fa..fdc2d0e15731 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -92,6 +92,7 @@ static struct { unsigned flag:8; char opt_char; } opt_array[]
= {
{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
{ _DPRINTK_FLAGS_INCL_TID, 't' },
{ _DPRINTK_FLAGS_NONE, '_' },
+ { _DPRINTK_FLAGS_DELETE_SITE, 'D' },
};
struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
@@ -201,6 +202,14 @@ static void ddebug_alter_site(struct _ddebug *dp,
} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(>key.dd_key_true);
#endif
+ /* delete site info for this callsite */
+ if (modifiers->flags & _DPRINTK_FLAGS_DELETE_SITE) {
+ if (dp->site) {
+ vpr_info("dropping site info %s.%s.%d\n",
dp->site->filename,
+ dp->site->function, dp->lineno);
+ dp->site = NULL;
+ }
+ }
}
/*
--
2.29.2
[RFC PATCH v3 11/18] dyndbg: refactor ddebug_alter_site out of ddebug_change
Move the JUMP_LABEL/static-key code to a separate function.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2d011ac3308d..6fbd9099c2fa 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -191,6 +191,18 @@ static int ddebug_match_site(const struct ddebug_query
*query,
return true;
}
+static void ddebug_alter_site(struct _ddebug *dp,
+ struct flag_settings *modifiers)
+{
+#ifdef CONFIG_JUMP_LABEL
+ if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+ if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
+ static_branch_disable(>key.dd_key_true);
+ } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
+ static_branch_enable(>key.dd_key_true);
+#endif
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -227,13 +239,9 @@ static int ddebug_change(const struct ddebug_query *query,
newflags = (dp->flags & modifiers->mask) |
modifiers->flags;
if (newflags == dp->flags)
continue;
-#ifdef CONFIG_JUMP_LABEL
- if (dp->flags & _DPRINTK_FLAGS_PRINT) {
- if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
-
static_branch_disable(>key.dd_key_true);
- } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
- static_branch_enable(>key.dd_key_true);
-#endif
+
+ ddebug_alter_site(dp, modifiers);
+
dp->flags = newflags;
if (dc)
--
2.29.2
[RFC PATCH v3 10/18] dyndbg: avoid calling dyndbg_emit_prefix when it has no work
Wrap function in a static-inline one, which checks flags to avoid
calling the function unnecessarily.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index af9cf97f869b..2d011ac3308d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -615,7 +615,7 @@ static int remaining(int wrote)
return 0;
}
-static char *dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
+static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
{
int pos_after_tid;
int pos = 0;
@@ -655,6 +655,13 @@ static char *dynamic_emit_prefix(const struct _ddebug *dp,
char *buf)
return buf;
}
+static inline char *dynamic_emit_prefix(struct _ddebug *dp, char *buf)
+{
+ if (unlikely(dp->flags & _DPRINTK_FLAGS_INCL_ANY))
+ return __dynamic_emit_prefix(dp, buf);
+ return buf;
+}
+
void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
{
va_list args;
--
2.29.2
[RFC PATCH v3 03/18] dyndbg: refactor part of ddebug_change to ddebug_match_site
Move all the site-match logic into a separate function, reindent the
code, and replace the continues with return falses.
No functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 75 ++---
1 file changed, 43 insertions(+), 32 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c3c35dcc6a59..9cff9db15937 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -142,6 +142,48 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+static int ddebug_match_site(const struct ddebug_query *query,
+const struct _ddebug *dp)
+{
+ struct _ddebug_site *dc = dp->site;
+
+ /* match against the source filename */
+ if (query->filename &&
+ !match_wildcard(query->filename, dc->filename) &&
+ !match_wildcard(query->filename,
+ kbasename(dc->filename)) &&
+ !match_wildcard(query->filename,
+ trim_prefix(dc->filename)))
+ return false;
+
+ /* match against the function */
+ if (query->function &&
+ !match_wildcard(query->function, dc->function))
+ return false;
+
+ /* match against the format */
+ 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)
+ return false;
+ } else if (!strstr(dp->format, query->format))
+ return false;
+ }
+
+ /* match against the line number range */
+ if (query->first_lineno &&
+ dp->lineno < query->first_lineno)
+ return false;
+ if (query->last_lineno &&
+ dp->lineno > query->last_lineno)
+ return false;
+
+ return true;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -170,38 +212,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct _ddebug *dp = >ddebugs[i];
struct _ddebug_site *dc = dp->site;
- /* match against the source filename */
- if (query->filename &&
- !match_wildcard(query->filename, dc->filename) &&
- !match_wildcard(query->filename,
- kbasename(dc->filename)) &&
- !match_wildcard(query->filename,
- trim_prefix(dc->filename)))
- continue;
-
- /* match against the function */
- if (query->function &&
- !match_wildcard(query->function, dc->function))
- continue;
-
- /* match against the format */
- 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 &&
- dp->lineno < query->first_lineno)
- continue;
- if (query->last_lineno &&
- dp->lineno > query->last_lineno)
+ if (!ddebug_match_site(query, dp))
continue;
nfound++;
--
2.29.2
[RFC PATCH v3 07/18] dyndbg: accept null site in dynamic_emit_prefix
2 prints use site->member, protect them with if site.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 151e55ab6bb5..0c535f3c2ba9 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -631,15 +631,19 @@ static char *dynamic_emit_prefix(const struct _ddebug
*dp, char *buf)
task_pid_vnr(current));
}
pos_after_tid = pos;
- if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
- pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->modname);
- if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
- pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->function);
+
+ if (desc) {
+ if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+ pos += snprintf(buf + pos, remaining(pos), "%s:",
+ desc->modname);
+ if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+ pos += snprintf(buf + pos, remaining(pos), "%s:",
+ desc->function);
+ }
if (dp->flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
dp->lineno);
+
if (pos - pos_after_tid)
pos += snprintf(buf + pos, remaining(pos), " ");
if (pos >= PREFIX_SIZE)
--
2.29.2
[RFC PATCH v3 05/18] dyndbg: hoist ->site out of ddebug_match_site
A coming change adds _get/_put abstraction on the site pointer, to
allow managing site info more flexibly. The get/put pattern is best
done at a single lexical scope, where its more obviously correct, so
hoist the ->site ref out of ddebug_match_site, and pass it in instead.
no functional changes
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index da732e0c56e3..abe3382aabd5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -143,10 +143,9 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
}
static int ddebug_match_site(const struct ddebug_query *query,
-const struct _ddebug *dp)
+const struct _ddebug *dp,
+const struct _ddebug_site *dc)
{
- struct _ddebug_site *dc;
-
/* match against the format */
if (query->format) {
if (*query->format == '^') {
@@ -167,7 +166,6 @@ static int ddebug_match_site(const struct ddebug_query
*query,
dp->lineno > query->last_lineno)
return false;
- dc = dp->site;
if (!dc) {
/* site info has been dropped, so query cannot test these
fields */
if (query->filename || query->function)
@@ -219,9 +217,9 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_site *dc;
+ struct _ddebug_site *dc = dp->site;
- if (!ddebug_match_site(query, dp))
+ if (!ddebug_match_site(query, dp, dc))
continue;
nfound++;
--
2.29.2
[RFC PATCH v3 04/18] dyndbg: accept null site in ddebug_match_site
basically, reorder the elements to minimize data fetches.
1- move format and line-number check code to the top of the function,
since they don't use/check site info.
2- test site pointer:
If its null, we return early, skipping 3:
If the query tests against missing site info, fail the match.
otherwize site matches.
3- rest of function (checking site vs query) is unchanged.
ddebug_match_site ignores module, because it's tested already
by the caller, where it is known from debug_tables.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 41 +
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9cff9db15937..da732e0c56e3 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -145,21 +145,7 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
static int ddebug_match_site(const struct ddebug_query *query,
const struct _ddebug *dp)
{
- struct _ddebug_site *dc = dp->site;
-
- /* match against the source filename */
- if (query->filename &&
- !match_wildcard(query->filename, dc->filename) &&
- !match_wildcard(query->filename,
- kbasename(dc->filename)) &&
- !match_wildcard(query->filename,
- trim_prefix(dc->filename)))
- return false;
-
- /* match against the function */
- if (query->function &&
- !match_wildcard(query->function, dc->function))
- return false;
+ struct _ddebug_site *dc;
/* match against the format */
if (query->format) {
@@ -181,6 +167,29 @@ static int ddebug_match_site(const struct ddebug_query
*query,
dp->lineno > query->last_lineno)
return false;
+ dc = dp->site;
+ if (!dc) {
+ /* site info has been dropped, so query cannot test these
fields */
+ if (query->filename || query->function)
+ return false;
+ else
+ return true;
+ }
+
+ /* match against the source filename */
+ if (query->filename &&
+ !match_wildcard(query->filename, dc->filename) &&
+ !match_wildcard(query->filename,
+ kbasename(dc->filename)) &&
+ !match_wildcard(query->filename,
+ trim_prefix(dc->filename)))
+ return false;
+
+ /* match against the function */
+ if (query->function &&
+ !match_wildcard(query->function, dc->function))
+ return false;
+
return true;
}
@@ -210,7 +219,7 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_site *dc = dp->site;
+ struct _ddebug_site *dc;
if (!ddebug_match_site(query, dp))
continue;
--
2.29.2
[RFC PATCH v3 06/18] dyndbg: accept null site in ddebug_change
fix a debug-print that includes site info, by adding an alternate
debug message that does not.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index abe3382aabd5..151e55ab6bb5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -235,10 +235,17 @@ static int ddebug_change(const struct ddebug_query *query,
static_branch_enable(>key.dd_key_true);
#endif
dp->flags = newflags;
- v2pr_info("changed %s:%d [%s]%s =%s\n",
-trim_prefix(dc->filename), dp->lineno,
-dt->mod_name, dc->function,
-ddebug_describe_flags(dp->flags, ));
+
+ if (dc)
+ v2pr_info("changed %s:%d [%s]%s =%s\n",
+ trim_prefix(dc->filename), dp->lineno,
+ dt->mod_name, dc->function,
+ ddebug_describe_flags(dp->flags,
));
+ else
+ v2pr_info("changed %s:%d =%s \"%s\"\n",
+ dt->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags,
),
+ dp->format);
}
}
mutex_unlock(_lock);
--
2.29.2
[RFC PATCH v3 02/18] dyndbg: __init iterate over __dyndbg & __dyndbg_site in parallel
In dynamic_debug_init(), rework for-loop; add 2nd 'site' var, and
iterate over both __dyndbg* sections in parallel. Replace uses of
iter->site with the new 'site' iter, add a BUG_ON to enforce the
invariant given by HEAD~1's DECLARE_DYNAMIC_DEBUG_METADATA base->site
initialization.
0- declare the new elf section start/stop, named in vmlinux.lds.h
I disregarded a checkpatch warning about externs in c-files, stuck
with current practice.
1- clean up use of 4 iterators for clarity:
(iter, site), and ((iter, site)_mod_start) block markers.
2- iterate over __dyndbg_sites in parallel with __dyndbg
s/iter->site/site/g;
3- add BUG_ON(iter->site != site)
DECLARE_DYNAMIC_DEBUG_METADATA + linker insure this now.
Maybe we can drop pointer, still get order.
4- var rename n to site_ct
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 738c4ce28046..c3c35dcc6a59 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -41,6 +41,8 @@
extern struct _ddebug __start___dyndbg[];
extern struct _ddebug __stop___dyndbg[];
+extern struct _ddebug_site __start___dyndbg_sites[];
+extern struct _ddebug_site __stop___dyndbg_sites[];
struct ddebug_table {
struct list_head link;
@@ -118,6 +120,7 @@ do {
\
#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
#define v2pr_info(fmt, ...)vnpr_info(2, fmt, ##__VA_ARGS__)
+#define v3pr_info(fmt, ...)vnpr_info(3, fmt, ##__VA_ARGS__)
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
@@ -1082,11 +1085,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_mod_start;
+ struct _ddebug_site *site, *site_mod_start;
const char *modname = NULL;
char *cmdline;
int ret = 0;
- int n = 0, entries = 0, modct = 0;
+ int site_ct = 0, entries = 0, modct = 0;
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1097,23 +1101,29 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
return 0;
}
- iter = __start___dyndbg;
- modname = iter->site->modname;
- iter_start = iter;
- for (; iter < __stop___dyndbg; iter++) {
+
+ iter = iter_mod_start = __start___dyndbg;
+ site = site_mod_start = __start___dyndbg_sites;
+ modname = site->modname;
+
+ for (; iter < __stop___dyndbg; iter++, site++) {
+
+ BUG_ON(site != iter->site);
entries++;
- if (strcmp(modname, iter->site->modname)) {
+
+ if (strcmp(modname, site->modname)) {
modct++;
- ret = ddebug_add_module(iter_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, site_ct,
modname);
if (ret)
goto out_err;
- n = 0;
- modname = iter->site->modname;
- iter_start = iter;
+ site_ct = 0;
+ modname = site->modname;
+ iter_mod_start = iter;
+ site_mod_start = site;
}
- n++;
+ site_ct++;
}
- ret = ddebug_add_module(iter_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, site_ct, modname);
if (ret)
goto out_err;
--
2.29.2
[RFC PATCH v3 01/18] dyndbg: split struct _ddebug, move display fields to new _ddebug_site
struct _ddebug has 2 flavors of fields: essential and optional. Split
the 3 optional fields: module, function, file into a new struct
_ddebug_site, and add pointer to it from _ddebug.
These fields are optional in that they are primarily used to generate
the optional "module:func:line" log prefix. They're also used to
select callsites to >control. lineno is arguably optional too, but
leaving it uses spare bytes in struct _ddebug.
The new ptr increases memory footprint by 1 ptr per pr_debug, but I
think its temporary, and the indirection gives several advantages:
- we can drop sites and their storage opportunistically.
this reduces per-site mem by 24/64.
Subsystems may not need/want "module:func:line:" in their logs.
If they already use format-prefixes such as "drm:kms:",
they can select on those, and don't need the site info for that.
forex:
#> echo module drm format "^drm:kms: " +p >control
ie: dynamic_debug_exec_queries("format '^drm:kms: '", "drm");
- the moved display fields are inherently hierarchical, and the linker
section is ordered; so (module, file, function) have repeating
values (90%, 85%, 45%). This is readily compressible, even with a
simple field-wise run length encoding. Since I'm splitting the struct,
I also reordered the fields to match the hierarchy.
- the separate linker section sets up naturally for block compression.
IFF we can on-demand map: ddebugs[N] -> ddebug_sites[N]
- we can compress __dyndbg_sites during __init, and mark section __initdata
- can decompress on-demand, say for `cat control`
- can save chunks of decompressed buffer for enabled callsites
- free chunks on site disable, or on memory pressure.
Whats actually done here is rather mechanical, and preparatory.
dynamic_debug.h:
I cut struct _ddebug in half, renamed optional top-half to
_ddebug_site, kept __align(8) for both halves. I added a forward decl
for a unified comment for both head & body, and added head.site to
point at body.
DECLARE_DYNAMIC_DEBUG_METADATA does the core of the work; it declares
and initializes both static struct variables together, and refs one to
the other.
dynamic_debug.c:
dynamic_debug_init() mem-usage now also counts sites.
dynamic_emit_prefix() & ddebug_change() use those moved fields; they
get a new initialized auto-var, and the field refs get adjusted as
needed to follow the move from one struct to the other.
struct _ddebug_site *dc = dp->site;
ddebug_proc_show() differs slightly; it assigns to (not initializes)
the autovar, to avoid a panic when p == SEQ_START_TOKEN.
vmlinux.lds.h:
add __dyndbg_sites section, with the same align(8) and KEEP as
used in the __dyndbg section.
Signed-off-by: Jim Cromie
---
include/asm-generic/vmlinux.lds.h | 3 ++
include/linux/dynamic_debug.h | 37 -
lib/dynamic_debug.c | 46 +--
3 files changed, 53 insertions(+), 33 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 0331d5d49551..4f2af9de2f03 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -353,6 +353,9 @@
*(__tracepoints)\
/* implement dynamic printk debug */\
. = ALIGN(8); \
+ __start___dyndbg_sites = .; \
+ KEEP(*(__dyndbg_sites)) \
+ __stop___dyndbg_sites = .; \
__start___dyndbg = .; \
KEEP(*(__dyndbg)) \
__stop___dyndbg = .;\
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index a57ee75342cf..bc8027292c02 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -7,20 +7,28 @@
#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_sites) for every dynamic debug callsite.
+ * At runtime, the sections are treated as arrays.
*/
-struct _ddebug {
+struct _ddebug;
+struct _ddebug_site {
/*
-* These fields are used to drive the user interface
-* for selecting and displaying debug callsites.
+* These fields (and lineno) are used to:
+* - decorate log messages per site flags
+* - select callsites for modification via >control
+* - display callsites & settings in `cat control`
*/
const char *modname;
- const char *function;
[RFC PATCH v3 00/18] dynamic debug diet plan
CONFIG_DYNAMIC_DEBUG creates a struct _ddebug (56 bytes) for each
callsite, which includes 3 pointers to: module, filename, function.
These are repetetive, and compressible, this patch series goes about
doing that, it:
- splits struct _ddebug and __dyndbg[] section/array into 2
creating struct _ddebug_site and __dyndbg_sites[]
temporary _ddebug.site connects them.
- makes _ddebug_site data optional
- minor optimizations
- makes _ddebug_site data deleteable
not necessary, proof of optionality
The RFC stuff comes at the end:
- attach __dyndbg_sites[] to module-info, like __dyndbg[]
- add index to struct _ddebug, use it for builtins
- add ddebug_site(_get|_put) abstraction to hide _ddebug.site
At this point, actually compressing __dyndbg_sites[] and using that is
doable: ddebug_site_get() has the info (compressed-table-ref, N) to do
the decompress / lookup, and could stick it (the retrieved records)
into a hash if the site is enabled for printing with the prefix.
Whats my (ideal?) decompressing interface ?
And whats the name of the api call ? I couldnt suss it out yet.
For any control read, a simple block decompress and cache is close to
ideal; all site data is then available to iterate over, and each
ddebug_site_get() just indexes into it. A stream of decompressed site
records would also work well, with less lumpy memory allocs and frees,
or maybe none at all.
Actually dropping _ddebug.site is not yet possible. While we could
drop it for builtin modules, thats cuz we know __start___dyndbg_sites.
For loaded modules, I need the elf section: __dyndbg_sites. This is
in load-info, but I dont have a path to it. In:
- add _ddebug_header/table
I managed to add a single header entry (a struct _ddebug with special
initialization) to the array, and it links to the front of the array,
where its useful. But creating this header entry only works for
vmlinux itself (because of vmlinux.ld.h patch), not for loadable
modules. Some breakout & reuse of the macro I added to vmxlinux.ld.h
might be the ticket. Please signal agreement, and suggest how.
Presuming the fixed header can be linked reliably in front doing
something like I tried, it can be recast as a ddebug_header and
unionized with struct _ddebug, and the _ddebugs[] will fit nicely as a
flex-array:
struct ddebug_table2 {
struct ddebug_header foo;
struct _ddebug ddebugs[];
}
A header would have 40 bytes, room to contain most/all of struct
ddebug_table's fields, a pointer to the __dyndbg_sites[] table, and a
list-head too, meaning it supports essential and nice-to-have
properties:
- the mapping: __dyndbg[N] --> __dyndbg_sites[N] # we NEED this
- we can enlist them directly in ddebug_tables. # freebie
ie avoid the kzalloc in ddebug_add_module()
And if not all fields fit in the space available in __dyndbg[0], there
is space available in __dyndbg_sites[0].
Additionally, at the end of __init, ddebug_tables list is composed of
memory entirely in __dyndbg[], which can then be make readonly (by
means I dont know). If this breaks insertions of loadable modules, we
can easily a 2nd list for that.
Jim Cromie (18):
dyndbg: split struct _ddebug, move display fields to new _ddebug_site
dyndbg: __init iterate over __dyndbg & __dyndbg_site in parallelg
dyndbg: refactor part of ddebug_change to ddebug_match_site
dyndbg: accept null site in ddebug_match_site
dyndbg: hoist ->site out of ddebug_match_site
dyndbg: accept null site in ddebug_change
dyndbg: accept null site in dynamic_emit_prefix
dyndbg: accept null site in ddebug_proc_show
dyndbg: optimize ddebug_emit_prefix
dyndbg: avoid calling dyndbg_emit_prefix when it has no work
dyndbg: refactor ddebug_alter_site out of ddebug_change
dyndbg: allow deleting site info via control interface
dyndbg+module: expose ddebug_sites to modules
dyndbg: add ddebug_site(_get|_put) abstraction
dyndbg: add _index to struct _ddebug
dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE
dyndbg: RFC - DECLARE/DEFINE_DYNAMIC_DEBUG_TABLE
dyndbg: shuffle ddebug_table fields
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/entry/vdso/Makefile | 3 +
arch/x86/purgatory/Makefile | 1 +
drivers/firmware/efi/libstub/Makefile | 3 +-
drivers/gpu/drm/i915/i915_drv.c | 2 +
include/asm-generic/vmlinux.lds.h | 24 +-
include/linux/dynamic_debug.h | 180 +--
kernel/module-internal.h | 1 +
kernel/module.c | 9 +-
lib/dynamic_debug.c | 313 +++---
scripts/Makefile.lib | 2 +
11 files changed, 428 insertions(+), 111 deletions(-)
--
2.29.2
[PATCH 19/20] dyndbg: try conditional linker expression in KEEP - RFC
This is the last patch in v3 of patchest Ive sent previously:
v2: https://lore.kernel.org/lkml/?q=Cromie+v2+00%2F19+2020-12-25+-Re
It isolates my only issue now, Id appreciate advice, and dont want to
distract you with the 18 previous commits.
Im trying to use ? : inside a KEEP(*(expression)) to only include
A_header when A has content.
IE:
KEEP(*( A ? A_header : ))
It fails with inscrutable linker error.
GEN modules.builtin
LD .tmp_vmlinux.kallsyms1
ld:./arch/x86/kernel/vmlinux.lds:46: syntax error
Is this possible by other modes of expression ?
I tried inserting {} 1st, that failed, appearing to foreclose any
foreach-like construct. I also tried () around each term, and a
preceding, embedded "_loc=.;" statement to test the parser.
I didnt try doing this with 2 separate KEEPs; while it would be
simple, it defeats the adjacency guaranteed by *(.text .rdata), which
is the point of this.
If this were to be possible, it opens up interesting options to
statically construct table headers, and possibly even tree structures
in the linker script. Id use it to add 1 header for each module, and
strip a column out of the table.
Ive pulled binutils to take a look at the source; having never done
anything bison-ish, I anticipate a long study without some focused
primer knowledge.
Signed-off-by: Jim Cromie
---
include/asm-generic/vmlinux.lds.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 87868c5a980a..6198cc850f9b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -329,10 +329,10 @@
#define DYNAMIC_DEBUG_DATA() \
. = ALIGN(8); \
__start___dyndbg_sites = .; \
- KEEP(*(__dyndbg_sites .gnu.linkonce.*.dyndbg_site))
\
+ KEEP(*(__dyndbg_sites ? .gnu.linkonce.*.dyndbg_site : ))\
__stop___dyndbg_sites = .; \
__start___dyndbg = .; \
- KEEP(*(__dyndbg .gnu.linkonce.*.dyndbg))
\
+ KEEP(*(__dyndbg ? .gnu.linkonce.*.dyndbg : )) \
__stop___dyndbg = .;
#else
#define DYNAMIC_DEBUG_DATA()
--
2.29.2
Re: [RFC PATCH v2 00/19] dynamic debug diet plan
On Tue, Dec 29, 2020 at 11:56 AM Joe Perches wrote: > > On Fri, 2020-12-25 at 13:19 -0700, Jim Cromie wrote: > > Well, we're mostly overeating, but we can all look forward to a diet > > in January. And more exersize. > > > > dyndbg's compiled-in data-table currently uses 56 bytes per prdebug; > > this includes 3 pointers to hierarchical "decorator" data, which is > > primarily for adding "module:function:line:" prefixes to prdebug > > messages, and for enabling and modifying those prdebugs selectively. > > > > This patchset decouples "decorator" data, and makes it optional, and > > disposable. By separating that data, it opens up possiblities to > > compress it, swap it out, map it selectively, etc. > > While this may be somewhat useful, what debugging does it really help? > Are there really memory limited platforms that enable dynamic debug? > > hi Joe, happy new year! Who wants to drop 5 lbs of weight for free ? You dont even have to put down the turkey leg. Seriously, I cant point to any particular use case that suddenly becomes possible. and there are no powerful new debugging features here either. but dynamic_debug: add an option to enable dynamic debug for modules only Recently reduced dyndbg's system footprint, surely to open up new use cases, users. This is an orthogonal (and more involved) approach to dropping more weight, and improving the coefficients in a user's cost-benefit equation. I tried out DRM as a user https://lore.kernel.org/lkml/20201204035318.332419-1-jim.cro...@gmail.com/ it works, but I got the impression Ville is inclined to use static-keys directly to replace drm_debug_enabled(), avoiding dyndbg overheads. The possible in-memory savings here are asymptotically 24/64 (56 maybe) of the footprint, which is easy if subsystems dont need the decorators/selectors, DRM has that option. Possible savings in dyndbg aside, a static-key takes 16 bytes. I think I can get struct _ddebug down to 32 bytes (RFC on 18,19 particularly) so Im still playing catch-up wrt what a minimal static-keys drm update could do. Theres also a vector of jump-labels form of static-keys that Ville may be able to exploit too. IOW, drm is not my ace card. but memory savings is still nice. Where Id like to RFC: (patch-19) DEFINE_DYNAMIC_DEBUG_TABLE(i915) worked. it adds a pair of header records into the 2 elf sections, It will let me drop the site pointer currently needed to get each site's decorations, when needed. But I had to code it in manually, as a test. Its not a general solution. I'd like to figure out how to have it defined in module scope automatically, and weakly, and maybe-unused, so that if the module does not have any pr_debugs, the header record is silently excluded, and that module's sections are left empty. When the header is linked in, as with my hacked i915.ko, It becomes possible to finally lose the _ddebug.site pointer. .module_index and container-of can replace it: it gets us from struct _ddebug *p back up to the header, then we could follow a header.sites_vector[.module_index] to the right decorators/selectors. Its a modest cost increase for a rarely used path, to shave 8/40 off our minimum footprint Then the total footprint reduces back to 56 bytes/callsite, but now with 24 optional, and manageable.. module_index would be a fine lookup to a compressed RO table of callsites, and a good-enough key to a hashtable of active/enabled pr-debug callsites. I played with zs-pool to store callsite data. Though it had problems, I did see 3/1 pages/zs-page, which is a decent (slightly pessimistic) proxy for what could be had with another (block) compression choice. Once compressed callsites works, we can drop and recycle the __dyndbg_callsites section. other pertinences: the 2 section relative ordering may be a consequence of : - natural ordering of compilation & lexical placement of the paired declarations - OR the site pointer, and its initialization between the 2 records. I suspect former. if 2nd, dropping site may lose the constraint between 2 sections. I havent tried yet to test the drop to see what happens, I cannot use the current BUG_ON (site_iter != iter->site) construct. I tried invoking TABLE from METADATA, hoping that __weak and __maybe_unused would allow redundant definitions it errored, something about "local" and "section" mumble. I now believe that initialization in TABLE is part of the problem, I tried : $ objcopy --dump-section __dyndbg_callsites=dd_callsites --dump-section __dyndbg=dd vmlinux.o I got mostly null data, as if some final linking wasnt yet done. [jimc@frodo local-i915m]$ od -c dd_callsites 000 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 \0 * 0205620 trying it on vmlinux doesnt work; objcopy: vmlinux: can't dump section '__dyndbg' - it does not exist: file format not recog
[RFC PATCH v2 01/19] dyndbg: fix use before null check
In commit a2d375eda771 ("dyndbg: refine export, rename to
dynamic_debug_exec_queries()"), a string is copied before checking it
isn't NULL. Fix this, report a usage/interface error, and return the
proper error code.
Fixes: a2d375eda771 ("dyndbg: refine export, rename to
dynamic_debug_exec_queries()")
Cc: sta...@vger.kernel.org
--
-v2 drop comment tweak, improve commit message
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index bd7b3aaa93c3..c70d6347afa2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -561,9 +561,14 @@ static int ddebug_exec_queries(char *query, const char
*modname)
int dynamic_debug_exec_queries(const char *query, const char *modname)
{
int rc;
- char *qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ char *qry; /* writable copy of query */
- if (!query)
+ if (!query) {
+ pr_err("non-null query/command string expected\n");
+ return -EINVAL;
+ }
+ qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ if (!qry)
return -ENOMEM;
rc = ddebug_exec_queries(qry, modname);
--
2.29.2
[RFC PATCH v2 17/19] dyndbg: rearrange struct ddebug_callsites
move static-key field to top of struct. It is the biggest field, and
most alignment constrained (I believe), so this improves ambient
pahole conditions.
It doesn't actually improve the packing, it only simplifies and
shrinks the pahole reporting.
probably drop during rebase, cleanup.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 0fcbe96736f3..e7b5e7664e51 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -25,7 +25,14 @@ struct _ddebug_callsite {
};
struct _ddebug {
+#ifdef CONFIG_JUMP_LABEL
+ union {
+ struct static_key_true dd_key_true;
+ struct static_key_false dd_key_false;
+ } key;
+#endif
struct _ddebug_callsite *site;
+
/* format is always needed, lineno shares word with flags */
const char *format;
const unsigned lineno:18;
@@ -56,12 +63,6 @@ struct _ddebug {
#define _DPRINTK_FLAGS_DEFAULT 0
#endif
unsigned int flags:8;
-#ifdef CONFIG_JUMP_LABEL
- union {
- struct static_key_true dd_key_true;
- struct static_key_false dd_key_false;
- } key;
-#endif
} __aligned(8);
--
2.29.2
[RFC PATCH v2 18/19] dyndbg: add module_index to struct _ddebug
We seek to drop the pointer added when we split struct _ddebug. This
would restore our footprint to parity worst case, with all the upsides
of callsite overhead management.
we will rely upon existing structure (and modify it):
__dyndbg[], __dyndbg_callsites[] are parallel arrays, packed in order
by the linker [*]. Together the pairs compose each prdebug's state.
Each _ddebug[].site points at the corresponding _ddebug_callsite[],
visually like a ladder.
At init we (already) iterate over __dyndbg[] & __dyndbg_callsites[],
and parse them into sub-vectors reffered/owned by ddebug_table's that
we create and collect in a list. We also prove via BUG_ON(site_iter
!= iter->site) that the ladder is straight.
IFF we can recreate site_iter, we can drop the .site pointer. But
site_iter is really just &__dyndbg_callsites[module_index], so we add
module_index here and now.
This doesn't finish the job.
With module_index, we can find the vector root. Now we need a
backlink up to the sponsor/owner ddebug_table record.
This has a few steps:
1 claim space in the linkage, for a module header.
DECLARE_DYNAMIC_DEBUG_TABLE
a special variety of DECLARE_DYNAMIC_DEBUG_METADATA,
sounds like it fits.
ideally its transparent, registers with module somehow.
linker needs to put this 1st in module's sub-vector
2 define that header - maybe something like:
union ddebug_table_header {
struct ddebug_table *owner;
struct _ddebug item;
}
OR
struct ddebug_table_vector {
struct ddebug_table *owner;
struct _ddebug vector[];
}
So with module_index, we can go from our _ddebug element in our
sub-vector to the root, and container_of() to get the backlink up to
the ddebug_table, then down to the sites[N]
The backlink is sort-of a poor-mans list, so why not just use one ?
- we only have 2 vectors in the list
- they never change size
- we only go up one leg, and down other
- list rotation would just confuse.
Given the amount of space in a struct _ddebug (40 bytes), we might be
able to pack part or all of ddebug_table into it, share it in a union,
and avoid kmalloc'g them at all.
if DECLARE_DYNAMIC_DEBUG_TABLE can be forced into the linkage at
the front of each modules sub-section, we can probably initialize
module_index statically, and not have to do it during _init().
add DEFINE_DYNAMIC_DEBUG_TABLE(module) as a special case of:
DEFINE_DYNAMIC_DEBUG_METADATA(module, "00-1st-in-subsection")
It's just a "callsite" we expect not to use, except as a 0th element
we can compute an offset from, to initialize each ddebug.module_index.
The next step is to get one into a module by brute force, see if it
compiles, and places this header record where we need it.
If that doesnt work, we need to fix it.
Later, this macro will be adapted to initialize a pair of alternate
structures; analogous to structs _ddebug & _ddebug_callsite, equal or
smaller in size, and implemented as structs unionized with them. This
pair of alt-structs is big enough to contain all of ddebug_table's
fields. For reference, the respective sizes:
A: struct _ddebug_callsite {
/* size: 24, cachelines: 1, members: 3 */
/* last cacheline: 24 bytes */
B: struct _ddebug {
/* XXX 6 bits hole, try to pack */
/* size: 40, cachelines: 1, members: 6 */
/* sum members: 36 */
/* sum bitfield members: 26 bits, bit holes: 1, sum bit holes: 6 bits */
/* last cacheline: 40 bytes */
C: struct ddebug_table {
struct list_head link; /* 016 */
const char * mod_name; /*16 8 */
unsigned int num_ddebugs; /*24 4 */
/* XXX 4 bytes hole, try to pack */
struct _ddebug * ddebugs; /*32 8 */
struct _ddebug_callsite * sites;/*40 8 */
/* size: 48, cachelines: 1, members: 5 */
/* sum members: 44, holes: 1, sum holes: 4 */
/* last cacheline: 48 bytes */
Clearly theres enough room in A + B to hold the contents of C. We
will keep the pointer in A" -> B", it will get us to all the contents
there, most importantly the sites vector.
It will be interesting to see just how much can be done by linker and
static initialization. Id be tickled if the linked list init could be
done statically, but it hardly matters; __ro_after_init (however its
spelled) would probably suffice for a solid security guarantee.
I presume its ok to have a list which is partly of items in RO memory,
but is extended at runtime, in our case when modules are (un)loaded.
If not, we can keep 2 lists, for builtin and dynamic-loaded modules
respectively.
[1] DECLARE_DYNAMIC_DEBUG_METADATA may be why 2 linker sections are
in-order; it links head->body as it "allocates" them. If we drop the
pointer, we lose the constraint on the relative or
[RFC PATCH v2 16/19] dyndbg: ddebug_site_get/put api commentary
Paths forward: (not mutually exclusive)
A: !site -> fill from backing store
1st try at this is/was using zram. At init, it copied each callsite
into a zs-allocation, and all site-> refs afterward went thru
_get/_put to zs-map on demand, and zs-unmap the site info. This
worked until I tried to keep callsites mapped while they're enabled.
Another approach is to compress the new linker section, using some
algo thats good at indexed decompression. I tried to test this a bit,
using objcopy, unsuccessfully:
objcopy --dump-section __dyndbg_callsites=dd_callsites vmlinux.o
>From vmlinux.o it was mostly empty, vmlinux didnt have the section.
B: callsite as a set of property vectors, indexed by 'N'
We know dp is in a vector, either in the builtin __dyndbg_callsite
linker section, or the same from a modprobed one. The builtin section
has all builtin module sub-sections catenated dogether.
At init, we iterate over the section, and "parse it" by creating a
ddebug_table for each module with prdebugs. ddebug_table.num_debugs
remembers the size of each modules' vector of prdebugs.
We need a few things:
- _ddebug.index field, which knows offset to start of this sub-vector.
this new field will be "free" because the struct has padding.
it can be initialized during init, then RO.
- a back-pointer at the beginning of the sub-vector, to the
ddebug_table "owning" (but not containing) this sub-vector of
prdebugs.
If we had both, we could get from the ddebug element to its vector
root, back up to the owning ddebug_table, then down to the _callsite
vector, and index to the right element. While slower than a pointer
deref, this is a cold path, and it allows elimination of the
per-callsite pointer member, thus greater density of the sections, and
still can support sparse site info.
That back-pointer feels tricky. It needs to be 1st in the sub-vector
is to reserve the 0th slot
ing its spot at the
front of
each module's ddebug sub-vector.
getting space in the
section for it. Initializing it is easy.
prdebugs are added to section when DECLARE_DYAMIC_DEBUG_METADATA is
compiled; its unclear whether they are intrinsically sorted during
compile, or whether thats a linker thing.
Ideally, a MODULE-mumble declaration can be coaxed into declaring a
module singleton in the section(s), either naturally at the top (or
bottom) or sorted into place.
If that doesn't work, a "preload if module is different" strategy
could maybe work, but I dont know how to do that in macros.
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 25f49515c235..ec28c113a361 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -146,7 +146,7 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
static struct _ddebug_callsite *ddebug_site_get(struct _ddebug *dp)
{
- return dp->site;
+ return dp->site; /* passthru abstraction */
}
static inline void ddebug_site_put(struct _ddebug *dp)
{
--
2.29.2
[RFC PATCH v2 12/19] dyndbg: allow deleting site info via control interface
Allow users & subsystems to selectively delete callsite info for
individual pr-debug callsites, or groups of them.
Its purpose is for subsystems such as DRM which:
- use distinct categories for logging, and can map them over to a
format prefix, like: "drm:core:", "drm:kms:", etc.
- are happy with group control of all the callsites in a class/cateory.
individual control is still possible using queries including line numbers
- don't need dynamic "module:function:line:" prefixes in log messages
- don't care about loss of context in /proc/dynamic_debug/control
before:
init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed
data\012"
init/main.c:1337 [main]run_init_process =pm "%s\012"
init/main.c:1335 [main]run_init_process =pm " with environment:\012"
init/main.c:1334 [main]run_init_process =pm "%s\012"
init/main.c:1332 [main]run_init_process =pm " with arguments:\012"
init/main.c:1121 [main]initcall_blacklisted =pm "initcall %s blacklisted\012"
init/main.c:1082 [main]initcall_blacklist =pm "blacklisting initcall %s\012"
then:
bash-5.0# echo file init/main.c +D > /proc/dynamic_debug/control
after:
init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed
data\012"
[main]:1337 =pmD "%s\012"
[main]:1335 =pmD " with environment:\012"
[main]:1334 =pmD "%s\012"
[main]:1332 =pmD " with arguments:\012"
[main]:1121 =pmD "initcall %s blacklisted\012"
[main]:1082 =pmD "blacklisting initcall %s\012"
Notes:
If Drm adopted dyndbg, i915 + drm* would add ~1600 prdebugs, amdgpu +
drm* would add ~3200 callsites, so the additional memory costs are
substantial. In trade, drm and drivers would avoid lots of calls to
drm_debug_enabled(). This patch should reduce the costs.
Using this interface, drm could drop site info for all categories /
prefixes controlled by bits in drm.debug, while preserving site info
and individual selectivity for any uncategorized prdebugs.
Lastly, because lineno field was not moved into _ddebug_callsite, it
can be used to modify a single[*] callsite even if drm has dropped all
the callsite data:
echo module $mod format ^$prefix line $line +p >control
Dropping a _callsite a one-way, information losing operation, so minor
misuse is possible. Worst case is maybe (depending upon previous
settings) some loss of logging context/decorations.
echo +D > /proc/dynamic_debug/control
[*] amdgpu has some macros invoking clusters of pr_debugs; each use of
them creates a cluster of pr-debugs with the same line number.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 9 +
2 files changed, 10 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index ea07a91a43bc..49fa1390d1f8 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -40,6 +40,7 @@ struct _ddebug {
#define _DPRINTK_FLAGS_INCL_FUNCNAME (1<<2)
#define _DPRINTK_FLAGS_INCL_LINENO (1<<3)
#define _DPRINTK_FLAGS_INCL_TID(1<<4)
+#define _DPRINTK_FLAGS_DELETE_SITE (1<<7) /* drop site info to save ram */
#define _DPRINTK_FLAGS_INCL_ANYSITE\
(_DPRINTK_FLAGS_INCL_MODNAME\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 6203a6ad1706..2d10fc1e16cd 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -90,6 +90,7 @@ static struct { unsigned flag:8; char opt_char; } opt_array[]
= {
{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
{ _DPRINTK_FLAGS_INCL_TID, 't' },
{ _DPRINTK_FLAGS_NONE, '_' },
+ { _DPRINTK_FLAGS_DELETE_SITE, 'D' },
};
struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
@@ -198,6 +199,14 @@ static inline void ddebug_alter_site(struct _ddebug *dp,
} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(>key.dd_key_true);
#endif
+ /* delete site info for this callsite */
+ if (modifiers->flags & _DPRINTK_FLAGS_DELETE_SITE) {
+ if (dp->site) {
+ vpr_info("dropping site info %s.%s.%d\n",
dp->site->filename,
+ dp->site->function, dp->lineno);
+ dp->site = NULL;
+ }
+ }
}
/*
--
2.29.2
[RFC PATCH v2 15/19] dyndbg: add ddebug_site_get/put api with pass-thru impl
Now that site info is optional, abstract it so we can manage it more
flexibly later. Change all site users to use ddebug_site_get(p)
instead, which just returns ->site. ddebug_site_put is called to
balance gets, it currently does nothing.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8ad9be28f38e..25f49515c235 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -144,6 +144,14 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+static struct _ddebug_callsite *ddebug_site_get(struct _ddebug *dp)
+{
+ return dp->site;
+}
+static inline void ddebug_site_put(struct _ddebug *dp)
+{
+}
+
static int ddebug_match_site(const struct ddebug_query *query,
const struct _ddebug *dp,
const struct _ddebug_callsite *dc)
@@ -242,13 +250,13 @@ static int ddebug_change(const struct ddebug_query *query,
struct _ddebug_callsite *dc = dp->site;
if (!ddebug_match_site(query, dp, dc))
- continue;
+ goto skipsite;
nfound++;
newflags = (dp->flags & modifiers->mask) |
modifiers->flags;
if (newflags == dp->flags)
- continue;
+ goto skipsite;
ddebug_alter_site(dp, modifiers);
@@ -264,6 +272,9 @@ static int ddebug_change(const struct ddebug_query *query,
dt->mod_name, dp->lineno,
ddebug_describe_flags(dp->flags,
),
dp->format);
+
+ skipsite:
+ ddebug_site_put(dp);
}
}
mutex_unlock(_lock);
@@ -633,11 +644,11 @@ static int remaining(int wrote)
return 0;
}
-static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
+static char *__dynamic_emit_prefix(struct _ddebug *dp, char *buf)
{
int pos_after_tid;
int pos = 0;
- const struct _ddebug_callsite *desc = dp->site;
+ const struct _ddebug_callsite *desc;
*buf = '\0';
@@ -653,6 +664,7 @@ static char *__dynamic_emit_prefix(const struct _ddebug
*dp, char *buf)
if (!(dp->flags & _DPRINTK_FLAGS_INCL_ANYSITE))
return buf;
+ desc = ddebug_site_get(dp);
if (desc) {
if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
@@ -670,6 +682,8 @@ static char *__dynamic_emit_prefix(const struct _ddebug
*dp, char *buf)
if (pos >= PREFIX_SIZE)
buf[PREFIX_SIZE - 1] = '\0';
+ ddebug_site_put(dp);
+
return buf;
}
@@ -952,7 +966,8 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
return 0;
}
- dc = dp->site;
+ dc = ddebug_site_get(dp);
+
if (dc) {
seq_printf(m, "%s:%u [%s]%s =%s \"",
trim_prefix(dc->filename), dp->lineno,
@@ -968,6 +983,8 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
seq_puts(m, "\"\n");
}
+ ddebug_site_put(dp);
+
return 0;
}
--
2.29.2
[RFC PATCH v2 11/19] dyndbg: refactor ddebug_alter_site out of ddebug_change
Move the JUMP_LABEL/static-key code to a separate inline function.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index daded73c8575..6203a6ad1706 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -188,6 +188,18 @@ static int ddebug_match_site(const struct ddebug_query
*query,
return true;
}
+static inline void ddebug_alter_site(struct _ddebug *dp,
+struct flag_settings *modifiers)
+{
+#ifdef CONFIG_JUMP_LABEL
+ if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+ if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
+ static_branch_disable(>key.dd_key_true);
+ } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
+ static_branch_enable(>key.dd_key_true);
+#endif
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -224,13 +236,9 @@ static int ddebug_change(const struct ddebug_query *query,
newflags = (dp->flags & modifiers->mask) |
modifiers->flags;
if (newflags == dp->flags)
continue;
-#ifdef CONFIG_JUMP_LABEL
- if (dp->flags & _DPRINTK_FLAGS_PRINT) {
- if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
-
static_branch_disable(>key.dd_key_true);
- } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
- static_branch_enable(>key.dd_key_true);
-#endif
+
+ ddebug_alter_site(dp, modifiers);
+
dp->flags = newflags;
if (dc)
--
2.29.2
[RFC PATCH v2 09/19] dyndbg: optimize ddebug_emit_prefix
Add early return if no callsite info is specified in site-flags.
This avoids fetching site info that isn't going to be printed.
RFC: is this a proper place to use likely()?
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 9 +
lib/dynamic_debug.c | 3 +++
2 files changed, 12 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index e155dfafc4d9..ea07a91a43bc 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -40,6 +40,15 @@ 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_INCL_ANYSITE\
+ (_DPRINTK_FLAGS_INCL_MODNAME\
+| _DPRINTK_FLAGS_INCL_FUNCNAME \
+| _DPRINTK_FLAGS_INCL_LINENO)
+#define _DPRINTK_FLAGS_INCL_ANY\
+ (_DPRINTK_FLAGS_INCL_ANYSITE\
+| _DPRINTK_FLAGS_INCL_TID)
+
#if defined DEBUG
#define _DPRINTK_FLAGS_DEFAULT _DPRINTK_FLAGS_PRINT
#else
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index f6d8137e4a46..8e81ce58c1bd 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -629,6 +629,9 @@ static char *dynamic_emit_prefix(const struct _ddebug *dp,
char *buf)
}
pos_after_tid = pos;
+ if (!(dp->flags & _DPRINTK_FLAGS_INCL_ANYSITE))
+ return buf;
+
if (desc) {
if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
--
2.29.2
[RFC PATCH v2 19/19] dyndbg: try DEFINE_DYNAMIC_DEBUG_TABLE
Test DEFINE_DYNAMIC_DEBUG_TABLE, in i915.ko, by adding an invocation
into i915_drv.c, the 1st object built in the Makefile. This does
manually what can perhaps be done transparently in headers (hopefully).
DEFINE_DYNAMIC_DEBUG_TABLE is based on DEFINE_DYNAMIC_DEBUG_METADATA;
just like its model, it creates a pair of structs: _ddebug &
_ddebug_callsite, and inits them with format="", lineno=0.
This:
- identifies it clearly in control output
- appears to place it 1st in the section(s)
it works here, at least for modprobed module
1st-to-build might be the real reason for the sort.
drivers/gpu/drm/i915/i915_drv.c:0 [i915](null) =_ ""
drivers/gpu/drm/i915/gvt/gvt.c:437 [i915]intel_gvt_register_hypervisor =_ "gvt:
core: Running with hypervisor %s
drivers/gpu/drm/i915/gvt/gvt.c:378 [i915]intel_gvt_init_device =_ "gvt: core:
gvt device initialization is done\0
other big diff between DEFINE_DYNAMIC_DEBUG_TABLE/_METADATA:
- not static decls.
removing static made the line appear in control output (below).
(cuz it might be reffd elsewhere, so its linked).
we want this.
it also needs to be visible where DEFINE_DYNAMIC_DEBUG_METADATA is
used, so it can be used to initialize .module_index, ie in many other
objects where pr_debugs are coded.
A peek inside i915_drv.o looks about right; i915_site is expected at least.
Relocation section '.rela__dyndbg' at offset 0x6f48 contains 2 entries:
Offset Info Type Sym. ValueSym. Name + Addend
0010 014c0001 R_X86_64_64 i915_site + 0
0018 001a0001 R_X86_64_64 .rodata.str1.1 +
548
Relocation section '.rela__dyndbg_callsites' at offset 0x6f90 contains 2
entries:
Offset Info Type Sym. ValueSym. Name + Addend
001a0001 R_X86_64_64 .rodata.str1.1 +
d8
0008 000a0001 R_X86_64_64 .rodata.str1.8 +
110
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/i915_drv.c | 3 +++
include/linux/dynamic_debug.h | 47 ++---
2 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index acc32066cec3..f2fae2a476db 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -88,6 +88,9 @@
static struct drm_driver driver;
+DEFINE_DYNAMIC_DEBUG_TABLE(i915);
+// DEFINE_DYNAMIC_DEBUG_TABLE(THIS_MODULE); // one alternative form
+
static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
{
int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 0f4e703c97ee..2f3c0f35cea0 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -102,6 +102,36 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
const struct ib_device *ibdev,
const char *fmt, ...);
+/** DEFINE_DYNAMIC_DEBUG_TABLE(module)
+ *
+ * This is an analogue to DEFINE_DYNAMIC_DEBUG_METADATA()
+
+ * It's just a "callsite" whose primary purpose is to create or
+ * reserve the 0th element pair in our sub-vectors of __dyndbg[] &
+ * __dyndbg_callsites[]. The format & lineno are set to sort 1st,
+ * though I suspect our good order is more about linkage. Either way,
+ * the TABLE is appearing 1st in control's i915 output.
+
+ * We want to use it to initialize .module_index, but I was unable to
+ * ref ##_base by any construct I thought to try;
+ * KBUILD_MODNAME in particular didnt work.
+
+ */
+
+#define DEFINE_DYNAMIC_DEBUG_TABLE(module) \
+ struct _ddebug_callsite __aligned(8) \
+ __section("__dyndbg_callsites") module##_site = { \
+ .modname = KBUILD_MODNAME, \
+ .filename = __FILE__, \
+ .function = NULL, \
+ }; \
+ struct _ddebug __aligned(8)\
+ __section("__dyndbg") module##_base = { \
+ .site = ##_site, \
+ .format = "", \
+ .lineno = 0, /* sort on mod, line */\
+ }
+
#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
static struct _ddebug_callsite __aligned(8)\
__section("__dyndbg_callsites") name##_site = { \
@@ -115,23 +145,10 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
.format = (fmt),\
.lineno = __LINE__, \
[RFC PATCH v2 13/19] dyndbg: verify __dyndbg & __dyndbg_callsite invariant
Prove that linker + DECLARE_DYNAMIC_DEBUG_METADATA reliably place the
2 related struct _ddebug* initializations into parallel/ordered slots
in the __dyndbg_* sections.
This is a step towards dropping the pointer between the 2 structs;
maybe the 2 vectors stay ordered, and we can deduce and use N. Of
course this test won't survive, since it needs the pointer we seek to
drop, but its a start.
0- iterate over __dyndbg_callsite in parallel with __dyndbg
rename var: s/iter_start/iter_mod_start/ for clarity, consistency.
I disregarded a checkpatch warning about externs in c-files, staying
consistent with long-standing code seemed better.
1- prove that iter->site == site_iter.
DECLARE_DYNAMIC_DEBUG_METADATA + linker insure this now
Maybe we can drop pointer, still get order.
WRT the debug-printing, its noisy, but only with verbose=3.
It warrants trimming later.
The offset grows smoothly, because it is N * sizeof(structs), which
differ. It looks reliable. Amend later to do math, converge on
truth. If numbers are stable after stripping pointer, we have N.
recptr mod-ptr N (void*)p
[1.929072] dyndbg: 2828: 82b32f28 82b32f10 1 24 40
[1.929326] dyndbg: 2829: 82b32f40 82b32f10 2 48 80
[1.930209] dyndbg: 2 debug prints in module i386
We have N (col 4), and N * structsize (col 5). I feel like it still
needs more staring at.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2d10fc1e16cd..c1a113460637 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -41,6 +41,8 @@
extern struct _ddebug __start___dyndbg[];
extern struct _ddebug __stop___dyndbg[];
+extern struct _ddebug_callsite __start___dyndbg_callsites[];
+extern struct _ddebug_callsite __stop___dyndbg_callsites[];
struct ddebug_table {
struct list_head link;
@@ -119,6 +121,7 @@ do {
\
#define vpr_info(fmt, ...) vnpr_info(1, fmt, ##__VA_ARGS__)
#define v2pr_info(fmt, ...)vnpr_info(2, fmt, ##__VA_ARGS__)
+#define v3pr_info(fmt, ...)vnpr_info(2, fmt, ##__VA_ARGS__)
static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
{
@@ -1147,7 +1150,8 @@ static int __init dynamic_debug_init_control(void)
static int __init dynamic_debug_init(void)
{
- struct _ddebug *iter, *iter_start;
+ struct _ddebug *iter, *iter_mod_start;
+ struct _ddebug_callsite *site, *site_mod_start;
const char *modname = NULL;
char *cmdline;
int ret = 0;
@@ -1162,23 +1166,33 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
return 0;
}
- iter = __start___dyndbg;
+
+ iter = iter_mod_start = __start___dyndbg;
+ site = site_mod_start = __start___dyndbg_callsites;
modname = iter->site->modname;
- iter_start = iter;
- for (; iter < __stop___dyndbg; iter++) {
+
+ for (; iter < __stop___dyndbg; iter++, site++) {
+
+ BUG_ON(site != iter->site);
+ v3pr_info("%u: %px %ld %ld %ld\n", entries, site,
+ site - site_mod_start,
+ ((void *)site - (void *)site_mod_start),
+ ((void *)iter - (void *)iter_mod_start));
entries++;
+
if (strcmp(modname, iter->site->modname)) {
modct++;
- ret = ddebug_add_module(iter_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, n, modname);
if (ret)
goto out_err;
n = 0;
modname = iter->site->modname;
- iter_start = iter;
+ iter_mod_start = iter;
+ site_mod_start = site;
}
n++;
}
- ret = ddebug_add_module(iter_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, n, modname);
if (ret)
goto out_err;
--
2.29.2
[RFC PATCH v2 10/19] dyndbg: avoid calling dyndbg_emit_prefix when it has no work
Wrap function in a static-inline one, which checks flags to avoid
calling the function unnecessarily.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8e81ce58c1bd..daded73c8575 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -612,7 +612,7 @@ static int remaining(int wrote)
return 0;
}
-static char *dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
+static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
{
int pos_after_tid;
int pos = 0;
@@ -652,6 +652,13 @@ static char *dynamic_emit_prefix(const struct _ddebug *dp,
char *buf)
return buf;
}
+static inline char *dynamic_emit_prefix(struct _ddebug *dp, char *buf)
+{
+ if (unlikely(dp->flags & _DPRINTK_FLAGS_INCL_ANY))
+ return __dynamic_emit_prefix(dp, buf);
+ return buf;
+}
+
void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
{
va_list args;
--
2.29.2
[RFC PATCH v2 07/19] dyndbg: accept null site in dynamic_emit_prefix
2 prints use site->member, protect them with if site.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 5422cef58130..190a796da03a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -628,15 +628,19 @@ static char *dynamic_emit_prefix(const struct _ddebug
*dp, char *buf)
task_pid_vnr(current));
}
pos_after_tid = pos;
- if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
- pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->modname);
- if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
- pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->function);
+
+ if (desc) {
+ if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+ pos += snprintf(buf + pos, remaining(pos), "%s:",
+ desc->modname);
+ if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+ pos += snprintf(buf + pos, remaining(pos), "%s:",
+ desc->function);
+ }
if (dp->flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
dp->lineno);
+
if (pos - pos_after_tid)
pos += snprintf(buf + pos, remaining(pos), " ");
if (pos >= PREFIX_SIZE)
--
2.29.2
[RFC PATCH v2 14/19] dyndbg+module: expose ddebug_callsites to modules
In order to drop the pointer connecting _ddebug records to _callsites,
we need to elevate the latter; we need to track it in (internal)
ddebug_tables, and set it in ddebug_add_module. That last part
exposes it by interface to module.c, so we add a field to load_info,
and adjust load_module to initialize it from the elf section.
Its possible that this closes a hole created when __dyndbg_callsites
section was added, and wasnt handled by module load-info. I never saw
any misbehavior loading i915.ko into a vm, but still..
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 4 ++--
kernel/module-internal.h | 1 +
kernel/module.c | 9 ++---
lib/dynamic_debug.c | 12
4 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 49fa1390d1f8..0fcbe96736f3 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -70,8 +70,8 @@ struct _ddebug {
/* 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);
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_callsite *sites,
+ unsigned int n, const char *modname);
extern int ddebug_remove_module(const char *mod_name);
extern __printf(2, 3)
void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 33783abc377b..920b085d2a1b 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -18,6 +18,7 @@ struct load_info {
char *secstrings, *strtab;
unsigned long symoffs, stroffs, init_typeoffs, core_typeoffs;
struct _ddebug *debug;
+ struct _ddebug_callsite *sites;
unsigned int num_debug;
bool sig_ok;
#ifdef CONFIG_KALLSYMS
diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..876765bc666a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2820,11 +2820,12 @@ static void add_kallsyms(struct module *mod, const
struct load_info *info)
}
#endif /* CONFIG_KALLSYMS */
-static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
unsigned int num)
+static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
+ struct _ddebug_callsite *sites, unsigned int
num)
{
if (!debug)
return;
- ddebug_add_module(debug, num, mod->name);
+ ddebug_add_module(debug, sites, num, mod->name);
}
static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
@@ -3299,6 +3300,8 @@ static int find_module_sections(struct module *mod,
struct load_info *info)
info->debug = section_objs(info, "__dyndbg",
sizeof(*info->debug), >num_debug);
+ info->sites = section_objs(info, "__dyndbg_callsites",
+ sizeof(*info->sites), >num_debug);
return 0;
}
@@ -3937,7 +3940,7 @@ static int load_module(struct load_info *info, const char
__user *uargs,
goto free_arch_cleanup;
}
- dynamic_debug_setup(mod, info->debug, info->num_debug);
+ dynamic_debug_setup(mod, info->debug, info->sites, info->num_debug);
/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
ftrace_module_init(mod);
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c1a113460637..8ad9be28f38e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -49,6 +49,7 @@ struct ddebug_table {
const char *mod_name;
unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct _ddebug_callsite *sites;
};
struct ddebug_query {
@@ -1014,8 +1015,8 @@ static const struct proc_ops proc_fops = {
* Allocate a new ddebug_table for the given module
* and add it to the global list.
*/
-int ddebug_add_module(struct _ddebug *tab, unsigned int n,
-const char *name)
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_callsite *sites,
+ unsigned int n, const char *name)
{
struct ddebug_table *dt;
@@ -1033,6 +1034,7 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
dt->mod_name = name;
dt->num_ddebugs = n;
dt->ddebugs = tab;
+ dt->sites = sites;
mutex_lock(_lock);
list_add(>link, _tables);
@@ -1182,7 +1184,9 @@ static int __init dynamic_debug_init(void)
if (strcmp(modname, iter->site->modname)) {
modct++;
- ret = ddebug_add_module(iter_mod_start, n, modname);
+
+ ret = ddebug_add_module(iter_mod_start, site_mod_start,
+
[RFC PATCH v2 08/19] dyndbg: accept null site in ddebug_proc_show
Accept a ddebug record with a null site pointer, and write abbreviated
output for that record that doesn't include site info (but does
include line-number, since that can be used in >control queries).
Also add a 2nd header line with a template for the new output.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 190a796da03a..f6d8137e4a46 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -915,18 +915,27 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
if (p == SEQ_START_TOKEN) {
seq_puts(m,
-"# filename:lineno [module]function flags format\n");
+"#: filename:lineno [module]function flags format\n"
+"#| [module]:lineno flags format\n"
+ );
return 0;
}
dc = dp->site;
-
- seq_printf(m, "%s:%u [%s]%s =%s \"",
- trim_prefix(dc->filename), dp->lineno,
- iter->table->mod_name, dc->function,
- ddebug_describe_flags(dp->flags, ));
- seq_escape(m, dp->format, "\t\r\n\"");
- seq_puts(m, "\"\n");
+ if (dc) {
+ seq_printf(m, "%s:%u [%s]%s =%s \"",
+ trim_prefix(dc->filename), dp->lineno,
+ iter->table->mod_name, dc->function,
+ ddebug_describe_flags(dp->flags, ));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ } else {
+ seq_printf(m, "[%s]:%u =%s \"",
+ iter->table->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags, ));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ }
return 0;
}
--
2.29.2
[RFC PATCH v2 04/19] dyndbg: accept null site in ddebug_match_site
make ddebug_match_site() tolerate null site.
1- move format and line-number check code to the top of the function,
since they don't use/check site info.
2- test site pointer:
If its null, we return early, skipping 3:
If the query tests against missing site info, fail the match.
otherwize site matches.
3- rest of function (checking site vs query) is unchanged.
ddebug_match_site is agnostic re' module, because it's tested already
by the caller, where it is known from debug_tables. If a query
constrains module, forex: "module drm*", non-matching modules are
skipped entirely in caller, so we can ignore it here.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 41 +
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index d9a0527ec842..bb9279c8cbfd 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -142,21 +142,7 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
static int ddebug_match_site(const struct ddebug_query *query,
const struct _ddebug *dp)
{
- struct _ddebug_callsite *dc = dp->site;
-
- /* match against the source filename */
- if (query->filename &&
- !match_wildcard(query->filename, dc->filename) &&
- !match_wildcard(query->filename,
- kbasename(dc->filename)) &&
- !match_wildcard(query->filename,
- trim_prefix(dc->filename)))
- return false;
-
- /* match against the function */
- if (query->function &&
- !match_wildcard(query->function, dc->function))
- return false;
+ struct _ddebug_callsite *dc;
/* match against the format */
if (query->format) {
@@ -178,6 +164,29 @@ static int ddebug_match_site(const struct ddebug_query
*query,
dp->lineno > query->last_lineno)
return false;
+ dc = dp->site;
+ if (!dc) {
+ /* site info has been dropped, so query cannot test these
fields */
+ if (query->filename || query->function)
+ return false;
+ else
+ return true;
+ }
+
+ /* match against the source filename */
+ if (query->filename &&
+ !match_wildcard(query->filename, dc->filename) &&
+ !match_wildcard(query->filename,
+ kbasename(dc->filename)) &&
+ !match_wildcard(query->filename,
+ trim_prefix(dc->filename)))
+ return false;
+
+ /* match against the function */
+ if (query->function &&
+ !match_wildcard(query->function, dc->function))
+ return false;
+
return true;
}
@@ -207,7 +216,7 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_callsite *dc = dp->site;
+ struct _ddebug_callsite *dc;
if (!ddebug_match_site(query, dp))
continue;
--
2.29.2
[RFC PATCH v2 06/19] dyndbg: accept null site in ddebug_change
fix a debug-print that includes site info, by adding an alternate
debug message that does not.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 050c65142d9b..5422cef58130 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -232,10 +232,17 @@ static int ddebug_change(const struct ddebug_query *query,
static_branch_enable(>key.dd_key_true);
#endif
dp->flags = newflags;
- v2pr_info("changed %s:%d [%s]%s =%s\n",
-trim_prefix(dc->filename), dp->lineno,
-dt->mod_name, dc->function,
-ddebug_describe_flags(dp->flags, ));
+
+ if (dc)
+ v2pr_info("changed %s:%d [%s]%s =%s\n",
+ trim_prefix(dc->filename), dp->lineno,
+ dt->mod_name, dc->function,
+ ddebug_describe_flags(dp->flags,
));
+ else
+ v2pr_info("changed %s:%d =%s \"%s\"\n",
+ dt->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags,
),
+ dp->format);
}
}
mutex_unlock(_lock);
--
2.29.2
[RFC PATCH v2 05/19] dyndbg: hoist ->site out of ddebug_match_site
A coming change adds _get/_put abstraction on the site pointer, to
allow managing site info more flexibly. The get/put pattern is best
done at a single lexical scope, where its more obviously correct, so
hoist the ->site ref out of ddebug_match_site, and pass it in instead.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index bb9279c8cbfd..050c65142d9b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -140,10 +140,9 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
}
static int ddebug_match_site(const struct ddebug_query *query,
-const struct _ddebug *dp)
+const struct _ddebug *dp,
+const struct _ddebug_callsite *dc)
{
- struct _ddebug_callsite *dc;
-
/* match against the format */
if (query->format) {
if (*query->format == '^') {
@@ -164,7 +163,6 @@ static int ddebug_match_site(const struct ddebug_query
*query,
dp->lineno > query->last_lineno)
return false;
- dc = dp->site;
if (!dc) {
/* site info has been dropped, so query cannot test these
fields */
if (query->filename || query->function)
@@ -216,9 +214,9 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = >ddebugs[i];
- struct _ddebug_callsite *dc;
+ struct _ddebug_callsite *dc = dp->site;
- if (!ddebug_match_site(query, dp))
+ if (!ddebug_match_site(query, dp, dc))
continue;
nfound++;
--
2.29.2
[RFC PATCH v2 02/19] dyndbg: split struct _ddebug, move display fields to new _ddebug_callsite
struct _ddebug has 5 fields used to select/display pr_debug callsites;
move 3 of them: module, function, file to new struct _ddebug_callsite,
and add pointer from 1st to 2nd (head to body).
The format field is excluded from the move because it is always needed
for an enabled site, the others are just optional decorations, at
least from the logging perspective. While lineno is also optional, it
can share space with flags, so it stays for density reasons.
While this increases memory footprint by 1 ptr per pr_debug, the
indirection gives several advantages:
- we can drop callsites and their storage opportunistically.
Subsystems may not want "module:func:line:" in their logs.
If they use format-prefixes, they can select on them,
and don't need the site info.
forex:
#> echo module drm format "^drm:kms: " +p >control
ie: dynamic_debug_exec_queries("format '^drm:kms: '", "drm");
- the moved display fields are inherently hierarchical, and the linker
section is ordered; so (module, file, function) have repeating
values (90%, 85%, 45%). This is readily compressible, even with a
simple field-wise run length encoding. Since I'm touching this so
deeply, I reordered the fields to match the hierarchy.
- the separate linker section sets up nice for block compression.
we could even provide as a kernel associated 'blob', like initrd, DT
- we can allocate uncompressed storage only for enabled callsites.
could deallocate sites on memory pressure.
- if we can rely on the linker to fill the 2 __dyndbg* sections in the
same order, we could treat them as parallel vectors, drop the
pointer, and store each element's index into _ddebug's padding to
get the _callsite[N]. Do same for flags.
Whats actually done here is rather mechanical.
dynamic_debug.h:
I cut struct _ddebug in half, renamed top-half (body), dropped
__align(8) on the body (its a no-op with 8 byte pointers), and kept
the __align(8) on the head (I suspect its there for the static_key
member). I added a forward decl for a unified comment for both head &
body, and added head.site to point at body.
DECLARE_DYNAMIC_DEBUG_METADATA does the core of the work; it declares
and initializes both static struct vars together, and refs one to the
other.
dynamic_debug.c:
dynamic_debug_init() mem-usage now also counts callsites.
dynamic_emit_prefix() & ddebug_change() use those moved fields; they
get a new initialized auto-var, and the field refs get adjusted as
needed to follow the move from one struct to the other.
struct _ddebug_callsite *dc = dp->site;
ddebug_proc_show() differs slightly; it assigns to (not initializes)
the autovar, to avoid a panic when p == SEQ_START_TOKEN.
vmlinux.lds.h:
add __ddebug_callsites section, with the same align(8) and KEEP as
used in the __ddebug section. RFC this is slightly out of sync with
METADATA code, and dropping align(8) on the struct itself.
Signed-off-by: Jim Cromie
---
include/asm-generic/vmlinux.lds.h | 4 +++
include/linux/dynamic_debug.h | 37 +-
lib/dynamic_debug.c | 44 ++-
3 files changed, 53 insertions(+), 32 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index b2b3d81b1535..1ef1efc73d20 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -340,6 +340,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 a57ee75342cf..e155dfafc4d9 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -7,20 +7,28 @@
#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.
+ * At runtime, the sections are treated as arrays.
*/
-struct _ddebug {
+struct _ddebug;
+struct _ddebug_callsite {
/*
-* These fields are used to drive the user interface
-* for selecting and displaying debug callsites.
+* The
[RFC PATCH v2 00/19] dynamic debug diet plan
Well, we're mostly overeating, but we can all look forward to a diet in January. And more exersize. dyndbg's compiled-in data-table currently uses 56 bytes per prdebug; this includes 3 pointers to hierarchical "decorator" data, which is primarily for adding "module:function:line:" prefixes to prdebug messages, and for enabling and modifying those prdebugs selectively. This patchset decouples "decorator" data, and makes it optional, and disposable. By separating that data, it opens up possiblities to compress it, swap it out, map it selectively, etc. In more detail, patchset does: 1- split struct _ddebug in 2, move "decorator" fields to _ddebug_callsites. while this adds a pointer per site to memory footprint, it allows: a- dropping decorations and storage, for some use cases. this could include DRM: - want to save calls to drm_debug_enabled() - use distinct categories, can map to format-prefixes, ex: "drm:kms:" - don't need "module:function:line" dynamic prefixes. - dont mind loss of info/context in /proc/dynamic_debug/control b- ddebug_callsites[] contents are hierarchical, compressible. c- ddebug_callsites[] in separate section is compressible as a block. d- for just enabled prdebugs, could allocate callsites and fill from zblock. 2- make ddebug_callsites optional internally. This lets us drop them outright, for any reason, perhaps memory pressure. 3- allow dropping callsites by those users. echo module drm +D > /proc/dynamic_debug/control this doesnt currently recover __dyndbg_callsites storage 4- drop _ddebug.site, convert to _ddebug[N].property lookup. RFC is mostly here. rev1: https://lore.kernel.org/lkml/20201125194855.2267337-1-jim.cro...@gmail.com/ rev2 differs by dropping zram attempt, making callsite data optional, etc. Jim Cromie (19): against v5.10 dyndbg: fix use before null check 1 dyndbg: split struct _ddebug, move display fields to new _ddebug_callsite 2 dyndbg: refactor part of ddebug_change to ddebug_match_site dyndbg: accept null site in ddebug_match_site dyndbg: hoist ->site out of ddebug_match_site dyndbg: accept null site in ddebug_change dyndbg: accept null site in dynamic_emit_prefix dyndbg: accept null site in ddebug_proc_show dyndbg: optimize ddebug_emit_prefix dyndbg: avoid calling dyndbg_emit_prefix when it has no work 3 dyndbg: refactor ddebug_alter_site out of ddebug_change dyndbg: allow deleting site info via control interface 4 dyndbg: verify __dyndbg & __dyndbg_callsite invariant dyndbg+module: expose dyndbg_callsites to modules dyndbg: add ddebug_site_get/put api with pass-thru impl dyndbg: ddebug_site_get/put api commentary dyndbg: rearrange struct ddebug_callsites dyndbg: add module_index to struct _ddebug dyndbg: try DEFINE_DYNAMIC_DEBUG_TABLE drivers/gpu/drm/i915/i915_drv.c | 3 + include/asm-generic/vmlinux.lds.h | 4 + include/linux/dynamic_debug.h | 97 --- kernel/module-internal.h | 1 + kernel/module.c | 9 +- lib/dynamic_debug.c | 271 +- 6 files changed, 283 insertions(+), 102 deletions(-) -- 2.29.2
[RFC PATCH v2 03/19] dyndbg: refactor part of ddebug_change to ddebug_match_site
Move all the site-match logic into a separate function, reindent the
code, and replace the continues with return falses.
No functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 75 ++---
1 file changed, 43 insertions(+), 32 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index fb8e0b288f89..d9a0527ec842 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -139,6 +139,48 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+static int ddebug_match_site(const struct ddebug_query *query,
+const struct _ddebug *dp)
+{
+ struct _ddebug_callsite *dc = dp->site;
+
+ /* match against the source filename */
+ if (query->filename &&
+ !match_wildcard(query->filename, dc->filename) &&
+ !match_wildcard(query->filename,
+ kbasename(dc->filename)) &&
+ !match_wildcard(query->filename,
+ trim_prefix(dc->filename)))
+ return false;
+
+ /* match against the function */
+ if (query->function &&
+ !match_wildcard(query->function, dc->function))
+ return false;
+
+ /* match against the format */
+ 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)
+ return false;
+ } else if (!strstr(dp->format, query->format))
+ return false;
+ }
+
+ /* match against the line number range */
+ if (query->first_lineno &&
+ dp->lineno < query->first_lineno)
+ return false;
+ if (query->last_lineno &&
+ dp->lineno > query->last_lineno)
+ return false;
+
+ return true;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -167,38 +209,7 @@ static int ddebug_change(const struct ddebug_query *query,
struct _ddebug *dp = >ddebugs[i];
struct _ddebug_callsite *dc = dp->site;
- /* match against the source filename */
- if (query->filename &&
- !match_wildcard(query->filename, dc->filename) &&
- !match_wildcard(query->filename,
- kbasename(dc->filename)) &&
- !match_wildcard(query->filename,
- trim_prefix(dc->filename)))
- continue;
-
- /* match against the function */
- if (query->function &&
- !match_wildcard(query->function, dc->function))
- continue;
-
- /* match against the format */
- 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 &&
- dp->lineno < query->first_lineno)
- continue;
- if (query->last_lineno &&
- dp->lineno > query->last_lineno)
+ if (!ddebug_match_site(query, dp))
continue;
nfound++;
--
2.29.2
Re: [RFC PATCH 1/2] drm: RFC add choice to use dynamic debug in drm-debug
On Fri, Dec 11, 2020 at 8:34 AM Ville Syrjälä
wrote:
>
> On Thu, Dec 03, 2020 at 08:53:17PM -0700, Jim Cromie wrote:
> > drm's debug system uses distinct categories of debug messages, mapped
> > to bits in drm.debug. Currently, code does a lot of unlikely bit-mask
> > checks on drm.debug (in drm_debug_enabled), we can use dynamic debug
> > instead, and get all that jump_label goodness.
> Is there an actual need to go through dyndbg and do all this stringy
> stuff, or would just eg. a static keys array for the debug categories
> get us the benefits of jump_label?
>
You certainly can strip the car, take the engine.
but you might need some of the drivetrain too.
maybe you want to skip the heated seats ?
dyndbg has some stuff you dont need, for sure.
for one, its heavy on data per callsite, with a static-key and
overhead for each.
But Id be wary that the jump-label code-patching is a slow path,
so trying to change hundreds of jump-sites with one static-key field
may run into problems with long lock hold times, etc.
There is a batching mechanism built-in to the jump-label stuff somewhere,
my impression is that it amortized system-wide syncs while being RT aware.
I've been working on trimming dyndbg down, at least the memory.
I'll be sending it out shortly, but heres a preview:
Subject: [RFC PATCH v2 0/7] dynamic debug diet plan
V2 is a rethought diet plan for dyndbg (I meant -v1 as rfc).
at highest level, patchset does:
1- move struct _ddebug "selector" fields to new struct _ddebug_callsite
2- make ddebug_callsites optional, good for some users
3- allow dropping callsites by those users.
1-v2. Rasmus noted that I shouldn't move format with the other fields,
and I realized that the "module:function:line" dynamic prefixes are
ultimately just log decorations, and are not needed for certain use
cases, including drm (with category -> prefix adaptation).
The drm use case:
- can benefit from jump-labels to avoid drm_debug_enabled()
- can map categories to format-prefixes: "drm:core:" "drm:kms:" etc
- can use dynamic_debug_exec_queries("format ^drm:core: +p", NULL)
- drm + amdgpu have ~3200 drm-debugs, drm + i915 have ~1600
If drm dropped optional site info, net 16 bytes saved / callsite, maybe more...
dropping optional info : module file func means loss of log "decorations"
and slimmer contents of control file. uncategorized pr-debugs can be
avoided when dropping callsites. Even with dropped info,
format, line, module queries can select individual sites precisely.
As of now, we still need the __dyndbg_callsites linker section; the
3-drop is just a forget-the-addy, not a kfree.
But compression is possible. v1 tried using zram, with mixed success.
v2 is a better foundation to re-try the zram.
[PATCH v2] dyndbg: fix use before null check
In commit a2d375eda771 ("dyndbg: refine export, rename to
dynamic_debug_exec_queries()"), a string is copied before checking it
isn't NULL. Fix this, report a usage/interface error, and return the
proper error code.
Fixes: a2d375eda771 ("dyndbg: refine export, rename to
dynamic_debug_exec_queries()")
Cc: sta...@vger.kernel.org
--
-v2 drop comment tweak, improve commit message
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index bd7b3aaa93c3..c70d6347afa2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -561,9 +561,14 @@ static int ddebug_exec_queries(char *query, const char
*modname)
int dynamic_debug_exec_queries(const char *query, const char *modname)
{
int rc;
- char *qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ char *qry; /* writable copy of query */
- if (!query)
+ if (!query) {
+ pr_err("non-null query/command string expected\n");
+ return -EINVAL;
+ }
+ qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ if (!qry)
return -ENOMEM;
rc = ddebug_exec_queries(qry, modname);
--
2.29.2
Re: [RFC PATCH 1/2] drm: RFC add choice to use dynamic debug in drm-debug
On Fri, Dec 4, 2020 at 8:42 AM Ville Syrjälä wrote: > > On Thu, Dec 03, 2020 at 08:53:17PM -0700, Jim Cromie wrote: > > drm's debug system uses distinct categories of debug messages, mapped > > to bits in drm.debug. Currently, code does a lot of unlikely bit-mask > > checks on drm.debug (in drm_debug_enabled), we can use dynamic debug > > instead, and get all that jump_label goodness. > > whatis jump_label? Sorry, I should have at least capitalized that, and spelled it differently kernel/Makefile 118:obj-$(CONFIG_JUMP_LABEL) += jump_label.o it is the hot-patching substrate underneath it all. static-key, static-call, etc? dynamic-debug uses static-key directly. > > One thing that bugs me about the current drm_dbg() stuff is that > it's a function, and thus we pay the cost of setting up the > arguments even when debugs are not enabled. I played around a bit > with making it a macro again with the unlikely bit check moved > into the macro. That did seem to make some of the asm a bit nicer > where the debug stuff got shoved out the main codepath, but > it did result in a slight net increase in code size. What I didn't > have time to do is check if this has any measurable speed effect > on eg. TEST_ONLY commits. > > And while doing that I started to ponder if we could use something > like the alternate() instruction stuff to patch the code at runtime > in order to turn all those debug checks into nops when debugging > is not enabled. But I couldn't immediately find any generic > infrastructure for it. So now I wonder if this jump_label is something > like that? > this is the droid youre looking for ;-) > > > -- > Ville Syrjälä > Intel
[RFC PATCH 2/2] i915: POC use dynamic_debug_exec_queries to control pr_debugs in gvt
The gvt component of this driver has ~120 pr_debugs, in 9 "classes".
Following model of drm.debug, add a parameter to map bits to these
classes.
In Makefile, add DYNAMIC_DEBUG_MODULE. This converts gvt's pr_debugs,
even if the rest of drm is not using CONFIG_DRM_USE_DYNAMIC_DEBUG.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/gvt/Makefile | 1 +
drivers/gpu/drm/i915/i915_params.c | 74 ++
2 files changed, 75 insertions(+)
diff --git a/drivers/gpu/drm/i915/gvt/Makefile
b/drivers/gpu/drm/i915/gvt/Makefile
index ea8324abc784..e38a1eb618bd 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -6,4 +6,5 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o
trace_points.o firmware.o \
fb_decoder.o dmabuf.o page_track.o
ccflags-y += -I $(srctree)/$(src) -I
$(srctree)/$(src)/$(GVT_DIR)/
+ccflags-y += -DDYNAMIC_DEBUG_MODULE
i915-y += $(addprefix $(GVT_DIR)/,
$(GVT_SOURCE))
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 7f139ea4a90b..ecc825558e00 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -260,3 +260,77 @@ void i915_params_free(struct i915_params *params)
I915_PARAMS_FOR_EACH(FREE);
#undef FREE
}
+
+/* POC for callback -> dynamic_debug_exec_queries */
+unsigned long __gvt_debug;
+EXPORT_SYMBOL(__gvt_debug);
+
+static char *format_prefix_classes[] = {
+ "gvt: cmd: ",
+ "gvt: core: ",
+ "gvt: dpy: ",
+ "gvt: el: ",
+ "gvt: irq: ",
+ "gvt: mm: ",
+ "gvt: mmio: ",
+ "gvt: render: ",
+ "gvt: sched: "
+};
+#define NUM_CLASSESARRAY_SIZE(format_prefix_classes)
+#define OUR_QUERY_SIZE 128 /* we need about 20 */
+
+#include
+
+static int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{
+ unsigned int val;
+ unsigned long changes, result;
+ int rc, chgct = 0, totct = 0, bitpos;
+ char query[OUR_QUERY_SIZE];
+
+ rc = kstrtouint(instr, 0, );
+ if (rc) {
+ pr_err("set_dyndbg: failed\n");
+ return -EINVAL;
+ }
+ result = val;
+ pr_info("set_dyndbg: result:0x%lx from %s\n", result, instr);
+
+ changes = result ^ __gvt_debug;
+
+ for_each_set_bit(bitpos, , NUM_CLASSES) {
+
+ sprintf(query, "format '^%s' %cp",
format_prefix_classes[bitpos],
+ test_bit(bitpos, ) ? '+' : '-');
+
+ chgct = dynamic_debug_exec_queries(query, "i915");
+
+ pr_info("%d changes on: %s\n", chgct, query);
+ totct += chgct;
+ }
+ pr_info("total changes: %d\n", totct);
+ __gvt_debug = result;
+ return 0;
+}
+static int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
+{
+ return scnprintf(buffer, PAGE_SIZE, "%u\n",
+*((unsigned int *)kp->arg));
+}
+static const struct kernel_param_ops param_ops_dyndbg = {
+ .set = param_set_dyndbg,
+ .get = param_get_dyndbg,
+};
+
+MODULE_PARM_DESC(debug_gvt, " gvt debug categories:"
+"\n\t0x1\t gvt: cmd:"
+"\n\t0x2\t gvt: core:"
+"\n\t0x4\t gvt: dpy:"
+"\n\t0x8\t gvt: el:"
+"\n\t0x10\t gvt: irq:"
+"\n\t0x20\t gvt: mm:"
+"\n\t0x40\t gvt: mmio:"
+"\n\t0x80\t gvt: render:"
+"\n\t0x100\t gvt: sched:" "\n");
+
+module_param_cb(debug_gvt, _ops_dyndbg, &__gvt_debug, 0644);
--
2.28.0
[RFC PATCH 1/2] drm: RFC add choice to use dynamic debug in drm-debug
drm's debug system uses distinct categories of debug messages, mapped
to bits in drm.debug. Currently, code does a lot of unlikely bit-mask
checks on drm.debug (in drm_debug_enabled), we can use dynamic debug
instead, and get all that jump_label goodness.
RFC: dynamic debug has no concept of category, but we can do without
one if we can prepend a class-prefix to each printk format. Then we
can use "format ^prefix" to select the whole category with one query.
This is a log-facing and user visible change, but it seems unlikely to
cause trouble for log watchers; they're not relying on the absence of
class prefix strings.
This conversion yields ~2100 new callsites on my i7 laptop:
dyndbg: 195 debug prints in module drm_kms_helper
dyndbg: 298 debug prints in module drm
dyndbg: 1630 debug prints in module i915
Since this change has wide-ranging effects (many drm drivers, with
many callsites, and kernel image growth), and most vendors don't
enable DYNAMIC_DEBUG, we supplement the existing mechanism, adding
CONFIG_DRM_USE_DYNAMIC_DEBUG to enable the new one.
The indirection/switchover has a few parts:
1 a new callback on drm.debug which calls dynamic_debug_exec_queries
to map those bits to specific query/commands
dynamic_debug_exec_queries("format ^drm:kms: +p", "drm*");
2 a "converted" or "classy" DRM_UT_* map
similar to DRM_UT_* ( symbol => bit-mask )
named it cDRM_UT_* ( symbol => format-class-prefix-string )
cDRM_UT_* is either ( CONFIG_DRM_USE_DYNAMIC_DEBUG )
legacy: cDRM_UT_* <-- DRM_UT_*
enabled:
+#define cDRM_UT_KMS"drm:kms: "
+#define cDRM_UT_PRIME "drm:prime: "
+#define cDRM_UT_ATOMIC "drm:atomic: "
these are similar to "gvt: cmd:" in i915/gvt
and effectively a replacement for DRM_NAME
please bikeshed on keys, values. latter are log-facing.
3 drm_dev_dbg & drm_debug are renamed (prefixed with '_')
old names are now macros, which are ifdefd
legacy: -> to renamed fn
enabled: -> dev_dbg & pr_debug, after prepending prefix to format.
4 names in (2) are called from DRM_DEBUG_ and drm_dbg_.
all these get "converted" to use cDRM_UT_*, to get right token type.
RFC: for dynamic debug, category is a source-facing addition;
something like pr_debug_cat(cat, ...) would do it, iff cat is a
compile-time const. Note that cat isn't needed in the printing, it
would be saved into a new field in struct _ddebug, and used only for
callsite selection, activation and control.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/Kconfig | 13 ++
drivers/gpu/drm/drm_print.c | 75 --
include/drm/drm_print.h | 92 +++--
3 files changed, 153 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 147d61b9674e..854bc1ad21fb 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -54,6 +54,19 @@ config DRM_DEBUG_MM
If in doubt, say "N".
+config DRM_USE_DYNAMIC_DEBUG
+ bool "use dynamic debug to implement drm.debug"
+ default n
+ depends on DRM
+ depends on DYNAMIC_DEBUG
+ depends on JUMP_LABEL
+ help
+ The drm debug category facility does a lot of unlikely bit-field
+ tests at runtime; while cheap individually, the cost accumulates.
+ This option uses dynamic debug facility (if configured and
+ using jump_label) to avoid those runtime checks, patching
+ the kernel when those debugs are desired.
+
config DRM_DEBUG_SELFTEST
tristate "kselftests for DRM"
depends on DRM
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 111b932cf2a9..e2acdfc7088b 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -52,8 +52,75 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit
enables a debug cat
"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n"
"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n"
"\t\tBit 8 (0x100) will enable DP messages (displayport code)");
+
+#ifndef CONFIG_DRM_USE_DYNAMIC_DEBUG
module_param_named(debug, __drm_debug, int, 0600);
+#else
+static char *format_class_prefixes[] = {
+ cDRM_UT_CORE,
+ cDRM_UT_DRIVER,
+ cDRM_UT_KMS,
+ cDRM_UT_PRIME,
+ cDRM_UT_ATOMIC,
+ cDRM_UT_VBL,
+ cDRM_UT_STATE,
+ cDRM_UT_LEASE,
+ cDRM_UT_DP,
+ cDRM_UT_DRMRES
+};
+
+#define OUR_QUERY_SIZE 64 /* > strlen "format '^%s' %cp" + longest prefix */
+
+static int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{
+ unsigned int val;
+ unsigned long changes, result;
+ int rc, chgct = 0, totct = 0, bitpos;
+ char query[OUR_QUERY_SIZE];
+
+ rc = kstrtouint(instr, 0, );
+ i
[RFC PATCH 0/2] drm: use dynamic_debug
hello gentle readers, These 2 rfc patches convert part of drm-world to use dynamic debug. 1st one addresses drm.debug category based logging. If DYNAMIC_DEBUG is configured, then CONFIG_DRM_USE_DYNAMIC_DEBUG controls whether dynamic-debug is used to avoid runtime costs of drm_debug_enabled(). We require CONFIG_JUMP_LABEL too, since we are selling its optimization. This change adds many new callsites to /proc/dynamic_debug/control; ~300 in drm, ~200 in drm_kms_helper, as well as ~1500 in i915 driver, and ~3200 in amdgpu. So there are substantial implications here. 2nd one is for i915, which I have in my laptop. `grep pr_debug` found ~90 callsites with a meaningful format-prefix-string, to demonstrate use of "format ^prefix" to control user categorized debugs. Jim Cromie (2): drm: RFC add choice to use dynamic debug in drm-debug i915: POC use dynamic_debug_exec_queries to control pr_debugs in gvt drivers/gpu/drm/Kconfig| 13 + drivers/gpu/drm/drm_print.c| 75 ++-- drivers/gpu/drm/i915/gvt/Makefile | 1 + drivers/gpu/drm/i915/i915_params.c | 74 include/drm/drm_print.h| 92 ++ 5 files changed, 228 insertions(+), 27 deletions(-) -- 2.28.0
Re: [PATCH 7/7] dyndbg: enable 'cache' of active pr_debug callsites
On Wed, Nov 25, 2020 at 1:54 PM Jason Baron wrote: > > > > On 11/25/20 2:36 PM, Jim Cromie wrote: > > In ddebug_putsite(), dont zs_unmap the callsite if it is enabled for > > printing. This means that the next time this pr_debug callsite is > > executed, the _getsite() will succeed quickly without remapping the > > zrec. > > > > Once the callsite is disabled via >control, a following _putsite() > > will see the flag cleared, and zs_unmap it. > > > > This changes the lifetime of our zs_mappings from brief (only for the > > single printk) to as long as uptime (if a prdebug is enabled til > > poweroff). This appears to be triggering the ensuing mayhem. > > > > I am able to get through init, to root console, by disabling all > > dynamic-debugs, including the ones that are enabled at compile. > > > > $ kruna --kopt \*.dyndbg=-p --kopt debug_locks_proceed=1 > > > > but enabling any pr-debug crashes. > > Plain old boot also panics, as pasted below. > > > > > Hi Jim, > > Looks like you've made a lot of progress with this series...but if I > understand correctly there are still some unresolved issues (panic). > So I think this series is more 'RFC' at this point? > yes, RFC. WIP suggests I know whats needed next. certainly Ive seen a variety of sleeping while atomic ... BUGs, > I was also expecting to see updates to the actual printing functions > to now use dp->site. But perhaps I missed those bits? those bits were changed in patch 5 basically changes are ~ s/dp/dc/g, and in ddebug_getsite / _putsite > > Thanks, > > -Jason > > > $ kruna --kopt debug_locks_proceed=1 > > [1.915768] ? rest_init+0x24d/0x24d > > [1.916417] kernel_init+0xaf/0x103 > > [1.917038] ret_from_fork+0x22/0x30 > > [1.917726] Kernel Offset: disabled > > [1.918324] ---[ end Kernel panic - not syncing: Requested init /bin/sh > > failed (error -14). ]--- > > QEMU 5.1.0 monitor - type 'help' for more information > > > > Signed-off-by: Jim Cromie > > --- > > lib/dynamic_debug.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > index 534d73e45844..0c69aa52395d 100644 > > --- a/lib/dynamic_debug.c > > +++ b/lib/dynamic_debug.c > > @@ -165,7 +165,9 @@ static void ddebug_putsite(struct _ddebug *dp) > > /* need to keep site until zmap backing exists */ > > return; > > > > - /* always unmap for now. if !pr-debug has locking issues */ > > + if (dp->flags & _DPRINTK_FLAGS_PRINT) > > + return; /* keep maps of enabled pr_debugs */ > > + > > zs_unmap_object(dd_callsite_zpool, dp->zhandle); > > dp->site = NULL; > > } > >
[PATCH 0/7] WIP dyndbg diet plan, paging Dr Zram.
hi all, To celebrate a season of ritual overeating, Id like to propose a diet plan for dynamic_debug. dynamic-debug is carrying extra weight/memory in the __dyndbg linker section, which is basically a struct _ddebug array[]. The 5 display oriented fields carry hierarchical data, and since the linker section is ordered, there is lots of repetition in .module .file .function fields. This patch-set: 1) splits struct _ddebug in 2, creating creating _ddebug_callsites with 5 display-fields, which are placed in a new linker section. while this adds indirection, it lets us manage storage of the .site data, letting us keep the bulk of it in compressed form. 2,3) explores run-length-encoding as one alternative to zram, basically throwaway, but for any ensuing discussion. 4) do kconfig bits - needs work 5) at late-init, copy site recs into zram (and save to .zhandle), thereafter .site is "refilled" (when null) from the .zhandle. Updating the _ddebug.site pointers also means we forget the link-time refs to _callsites section, allowing its reclaim later (once I know how). The .site "refill" is by _getsite(); the corresponding _putsite() works: it zs_unmaps ASAP and zeros .site. This means that every pr_debug will map/unmap once, and `cat control` will do so per line. Probably too expensive. But it works. Im seeing 3 pages_per_zspage, on entire section. 6) a hot locking mess. well, maybe not. plenty of uncertainty though. 7) does not work. It changes _putsite() to keep each zs_mapping when the pr_debug is enabled. But it panics in late-init, with BUG sleeping in atomic context. ? dynamic_emit_prefix is in backtrace. disabling all pr_debugs (--kopt \*.dyndbg=-p) gets to root prompt. once there: echo module main +m > /proc/dynamic_debug/control # is ok echo module main +p > /proc/dynamic_debug/control # BUG sleeping invalid context This strongly implicates the dynamic_emit_prefix call chain. It puzzles me that keeping the zs_mapping would cause so much trouble; its not causing more pr_debugs, and they should all be mapped already. But I really dont know what Im doing here.. WAG at possible memory savings: - add 2 pointers (zhandle, site) 56+16 - drop 48 (sizeof _callsite) * num-pr-debugs linker section - replace with 3:1 storage on same - 48 bytes per enabled pr_debug, typically <1% of set IE: dyndbg: 216 modules, 2058 entries and 8640 bytes in ddebug tables, 115248 bytes in __dyndbg section VS: dyndbg: 224 modules, 2015 entries and 8960 bytes in ddebug tables, 80600 bytes in __dyndbg section, 80600 bytes in __dyndbg_callsites section dyndbg: 2015 entries. repeated entries: 1790 module 1668 file 810 func Once __dyndbg_callsites section is reclaimed, %30 savings looks likely. Jim Cromie (7): dyndbg: move struct _ddebug's display fields to new _ddebug_callsite dyndbg: count repetition in __dyndbg_callsite fields. dyndbg: add some code to see alignments of linkage data dyndbg: select ZPOOL,ZS_MALLOC in Kconfig.debug DYNAMIC_DEBUG_CORE dyndbg: 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 | 42 -- lib/Kconfig.debug | 2 + lib/dynamic_debug.c | 242 +- 4 files changed, 239 insertions(+), 51 deletions(-) To: linux...@kvack.org Cc: Rasmus Villemoes -- 2.28.0
[PATCH 4/7] dyndbg: select ZPOOL,ZS_MALLOC in Kconfig.debug DYNAMIC_DEBUG_CORE
dyndbg will next need zs_malloc and friends, so add config reqs now, to maybe avoid churny remakes later. ZPOOL,ZSMALLOC are now required for DYNAMIC_DEBUG_CORE, as theyre needed to get value (mem use reduction) from the upcoming dyndbg/callsite split. --- ZS_MALLOC is done with "depends on" instead of "select" to break a recursive dependency. I think this doesnt quite sort things out for other config permutations, TBD. -v2 fixup -> _CORE Reported-by: kernel test robot Signed-off-by: Jim Cromie --- lib/Kconfig.debug | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index c789b39ed527..7eb7b43037d9 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -170,6 +170,8 @@ config DYNAMIC_DEBUG_CORE bool "Enable core function of dynamic debug support" depends on PRINTK depends on (DEBUG_FS || PROC_FS) + select ZPOOL + depends on ZSMALLOC help Enable core functional support of dynamic debug. It is useful when you want to tie dynamic debug to your kernel modules with -- 2.28.0
[PATCH 2/7] dyndbg: count repetition in __dyndbg_callsite fields.
The __dyndbg_callsite section is basically a flat db-table, an array
of struct _ddebug_callsite[]s. Its data is inherently hierarchical
and sorted, so a field-wise run-length encoding would compress well.
Add counters and code to test 3 fields of consecutive callsites for
repeated values. The results inform a compression estimate.
dyndbg: 2605 entries. repeated entries: 2369 module 2231 file 1147 func
Thats (91%, 86%, 44%) repeated values in those pointers/columns, on my
i7 laptop build. With a zero-overhead markup, like LSB-stealing, we
could mark the differing tails of consecutive records, and get 11/24
compression on init/main pr_debugs.
For a slightly apples-to-oranges comparison (text vs pointers),
`gzip /proc/dynamic_debug/control` achieves 6/1 compression.
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 2e4a39c349a5..5980d44ff2f8 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1081,11 +1081,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 = NULL;
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)) {
@@ -1099,7 +1100,16 @@ static int __init dynamic_debug_init(void)
iter = __start___dyndbg;
modname = iter->site->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->site->modname)) {
modct++;
@@ -1122,6 +1132,9 @@ static int __init dynamic_debug_init(void)
(int)(entries * sizeof(struct _ddebug)),
(int)(entries * sizeof(struct _ddebug_callsite)));
+ 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.28.0
[PATCH 7/7] dyndbg: enable 'cache' of active pr_debug callsites
1996), BIOS 1.13.0-3.fc33 04/01/2014 [1.912845] Call Trace: [1.913289] dump_stack+0x7d/0x9f [1.913957] ? rest_init+0x210/0x24d [1.914535] panic+0x10a/0x2de [1.915094] ? kernel_execve+0x145/0x1b0 [1.915768] ? rest_init+0x24d/0x24d [1.916417] kernel_init+0xaf/0x103 [1.917038] ret_from_fork+0x22/0x30 [1.917726] Kernel Offset: disabled [1.918324] ---[ end Kernel panic - not syncing: Requested init /bin/sh failed (error -14). ]--- QEMU 5.1.0 monitor - type 'help' for more information Signed-off-by: Jim Cromie --- lib/dynamic_debug.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index 534d73e45844..0c69aa52395d 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -165,7 +165,9 @@ static void ddebug_putsite(struct _ddebug *dp) /* need to keep site until zmap backing exists */ return; - /* always unmap for now. if !pr-debug has locking issues */ + if (dp->flags & _DPRINTK_FLAGS_PRINT) + return; /* keep maps of enabled pr_debugs */ + zs_unmap_object(dd_callsite_zpool, dp->zhandle); dp->site = NULL; } -- 2.28.0
[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 to isolate it
and the reasoning for review, which now stated, will be refuted as
needed. ;-)
Locking review:
ddebug_zpool_init(), like other ddebug_*_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 of them; ddebug_change() and 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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 473406b069a7..534d73e45844 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1195,8 +1195,10 @@ static void __init ddebug_zpool_init(void)
}
/* add-module normally does this, but not in time for builtins */
+ mutex_lock(_lock);
for (iter = __start___dyndbg; iter < __stop___dyndbg; iter++)
ddebug_zpool_add(iter);
+ mutex_unlock(_lock);
v2pr_info("total pages: %lu compaction: %lu\n",
zs_get_total_pages(dd_callsite_zpool),
--
2.28.0
[PATCH 1/7] dyndbg: move struct _ddebug's display fields to new _ddebug_callsite
struct _ddebug has 5 fields used to display and select pr_debug callsites, move these to a new struct _ddebug_callsite, and add ptr to link 1st to 2nd. While this increases memory footprint by 1 ptr per pr_debug, the indirection gives several advantages: - we can allocate storage only for enabled callsites. Since pr_debugs are 99% disabled, we should see savings. - the display fields are inherently hierarchical, and the linker section is ordered; so (module, file, function) have redundant values (90%, 85%, 45%). This is readily compressible, even with a simple field-wise run length encoding. Because theyre placed in a separate linker section, theyre in a contiguous block of memory, which should simplify that compression. Looking forward, there are several approaches to get the advantages. A - copy each callsite to zram, save to new .zhandle member, and update site pointers. Must later retire __dyndbg_callsite section afterwards to actually recover memory. I did this (next patches), and get 3:1 zs_page:page compression. It works when sites are zs_mapped in just for the print. But if I leave them mapped in cuz the pr_debug is enabled, locking conflicts & panic ensue. Patches follow. B - compress __dyndbg_callsite linker section, using some format which is good at random-index decompression. I did objcopy --dump-sections .. vmlinux.o, got mostly empty data, like Im getting values before the final link. Im missing some understanding. C - field-wise RLE. This is feeling increasingly suitable. Whats actually done here: dynamic_debug.h: I cut struct _ddebug in half, renamed top-half (body), kept __align(8) on both head & body, added a forward decl for a unified comment for both head & body. And added head.site to point at body. DECLARE_DYNAMIC_DEBUG_METADATA does the core of the work; it declares and initializes both static struct vars together, and refs one to the other. And since Im rejiggering the structs: - I moved static_key key to front of struct _ddebug; its the biggest member, and most alignment sensitive, so moving it to front may improve ambient pahole conditions. - reorder display fields to match the hierarchy. This should help improve compressability, particularly for field-wise RLE. With this, consecutive records are tail-different. Also - reserved a flag bit for zram mapping (no use yet) - I shrunk lineno member from 18 to 16 bits, and made it const. No source file is near 64k-lines, I doubt any could get added. dynamic_debug.c: dynamic_debug_init() mem-usage now counts callsites. The 3 funcs which use _ddebug* pointers (ddebug_change, dynamic_emit_prefix, ddebug_proc_show) each get an auto-var, inited with ->site, and s/dp/dc/ as needed. These once-per-func dp->site derefs are also 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. TBD: _align(8) may be unnecessary on struct _ddebug_callsite, I think its there for the static_key member. I do wonder if its arch dependent, 8 seems big for i686 at least. Signed-off-by: Jim Cromie --- include/asm-generic/vmlinux.lds.h | 4 +++ include/linux/dynamic_debug.h | 41 +++- lib/dynamic_debug.c | 52 +-- 3 files changed, 58 insertions(+), 39 deletions(-) diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index b2b3d81b1535..1ef1efc73d20 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -340,6 +340,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 a57ee75342cf..0bf7036bcdb2 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 up
[PATCH 5/7] dyndbg: replace __dyndbg_callsite section with a zs-pool copy.
A previous commit split struct _ddebugs into heads & bodies, linked
across 2 ELF sections. Lets now copy the bodies into a zs_pool, and
relink the head to the new body. This should allow recycling the
__dyndbg_callsite section soon.
The goal is to let a compression algo handle the repetition, and map
individual records in when needed. I have seen 3:1 page/zspage
compression (in /sys/kernel/debug/zsmalloc/callsites)
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:
New ddebug_zpool_init(), at late_initcall time, creates the zpool.
Per dmesg, this is shortly after zswap is ready.
ddebug_add_module() now also calls new ddebug_zpool_add(1) foreach
callsite in the module. (1) copies the _ddebug_callsite record into
the zpool, once the zpool is ready. For builtin modules, added in
early-boot, that zpool was not ready yet. So ddebug_zpool_init() also
calls ddebug_zpool_add() for all those builtin modules already added
by ddebug_add_module().
New ddebug_zpool_add() does, foreach _ddebug:
- zs_mallocs a zrec, saves handle to .zhandle,
- zs_maps and copies callsite to it
- zs_unmaps it, triggering write to zram
- .site=0, detaching __dyndbg_callsites[item]
which later triggers zs_map (.zhandle)
So we leave late-init with a full zram copy of __dyndbg_callsites
section, which is thus ready to reclaim.
ddebug_zpool_remove() undoes ddebug_zpool_add().
We call it from ddebug_remove_module().
The 3 existing users of struct _ddebug get the site ref entirely via 2
helpers, which manage the zs-mapping;
- ddebug_getsite() returns .site 1st, or maps zrec to it
and returns it.
- ddebug_putsite() always unmaps, minimizing online ram.
(see 1 below)
Those 3 users each take ddebug_lock around the get/puts, either
directly, or via ddebug_proc_(start|end).
ddebug_change() also gets s/continue/goto skipsite/g to cleanly unmap
the record at the bottom of the loop.
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
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. 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 would save on-line ram.
But it broke in HEAD~1 when I tried it, with asm goto errors.
`grep arch_static_branch` suggests my argument to it was uniquely
complicated and dereferencing.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 1 +
lib/dynamic_debug.c | 148 +++---
2 files changed, 137 insertions(+), 12 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 0bf7036bcdb2..a8336b9cc011 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -34,6 +34,7 @@ struct _ddebug {
} key;
#endif
struct _ddebug_callsite *site;
+ long unsigned int zhandle;
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c84efb4e036b..473406b069a7 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
#include
@@ -72,6 +73,8 @@ static LIST_HEAD(ddebug_tables);
static int verbose;
module_param(verbose, int, 0644);
+static struct zs_pool *dd_callsite_zpool;
+
/* Return the path relative to source root */
static inline const char *trim_prefix(const char *path)
{
@@ -139,6 +142,34 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno);
}
+/*
+ * ddebug_getsite - returns site, or maps it then returns it.
+ */
+static struct _ddebug_callsite* ddebug_getsite(struct _ddebug *dp)
+{
+ if (dp->site)
+ return dp->site;
+
+ dp->site = (struct _ddebug_callsite*)
+ zs_map_object(dd_callsite_zpool, dp->zhandle, ZS_MM_RO);
+
+ return dp->site;
+}
+
+/*
+ * ddebug_putsite - unmaps site, implements policy choice
+ */
+static void ddebug_putsite(struct _ddebug *dp)
+{
+ if (!dp->zhandle)
+ /* need to keep site until zmap backing exists */
+ return;
+
+ /* always unmap for now. if !pr-debug has locking issues */
+ zs_unmap_object(dd_callsite_zpool, dp-
[PATCH 3/7] dyndbg: add some code to see alignments of linkage data
To be able to use LSB-stealing (lsb as flag, mask b4 addressing), we
need to know that the alignments of existing data is never odd. So
add code to check the alignment of pointers in _dyndbg_callsites
section.
It turns out that all these fields point to a variety of odd
alignments in the deeper linkage data. We will need ALIGN(2) to allow
use of LSB to signal the EOT of differing-tail records.
[0.587654] dyndbg: 2014 entries. \
repeated entries: 1789 module 1667 file 809 func
[0.588653] dyndbg: mod aligns:
[0.589130] dyndbg: align 0 0: 107
[0.589653] dyndbg: align 0 1: 210
[0.590271] dyndbg: align 0 2: 234
[0.590652] dyndbg: align 0 3: 179
[0.591224] dyndbg: align 0 4: 358
[0.591652] dyndbg: align 0 5: 374
[0.592283] dyndbg: align 0 6: 231
[0.592653] dyndbg: align 0 7: 321
[0.593203] dyndbg: mod align totals: 2014
[0.593652] dyndbg: file aligns:
[0.594168] dyndbg: align 1 0: 569
[0.594652] dyndbg: align 1 1: 172
[0.595179] dyndbg: align 1 2: 263
[0.595652] dyndbg: align 1 3: 252
[0.596368] dyndbg: align 1 4: 200
[0.596652] dyndbg: align 1 5: 126
[0.597327] dyndbg: align 1 6: 177
[0.597669] dyndbg: align 1 7: 255
[0.598316] dyndbg: file align totals: 2014
[0.598652] dyndbg: func aligns:
[0.599214] dyndbg: align 2 0: 2006
[0.599652] dyndbg: align 2 1: 7
[0.600168] dyndbg: align 2 2: 0
[0.600648] dyndbg: align 2 3: 0
[0.600652] dyndbg: align 2 4: 0
[0.601105] dyndbg: align 2 5: 0
[0.601652] dyndbg: align 2 6: 0
[0.602203] dyndbg: align 2 7: 1
[0.602654] dyndbg: func align totals: 2014
[0.603431] dyndbg: format aligns:
[0.603658] dyndbg: align 3 0: 1053
[0.604429] dyndbg: align 3 1: 134
[0.604653] dyndbg: align 3 2: 161
[0.605181] dyndbg: align 3 3: 142
[0.605652] dyndbg: align 3 4: 137
[0.606250] dyndbg: align 3 5: 117
[0.606652] dyndbg: align 3 6: 124
[0.607183] dyndbg: align 3 7: 146
[0.608669] dyndbg: earlyprintk="serial,ttyS0,115200"
This means low order bits are unavailable for encoding extra info, as
is done in struct static_key.
Consider the last lines of /proc/dynamic_debug/control, here with
matching fields replaced by '^'. The pattern here is common head(s)
& differing tail(s).
init/main.c main run_init_process "%s\012" 1339
^ ^ ^ " with environment:\012" 1337
^ ^ ^ "%s\012" 1336
^ ^ ^ " with arguments:\012" 1334
^ ^ initcall_blacklisted "initcall %s blacklisted\012" 1123
^ ^ initcall_blacklist "blacklisting initcall %s\012" 1084
struct ddebug_callsite fields [mod, file, func, format] are 4-tuples;
each element points at different kinds/sources of linkage data
(>name, __FILE__, __FUNC__, format-strings).
callsite-tails are 4,3,2,1-tuples, with matching heads removed.
If we can force ALIGN(2) on all those linkage data sources, then we
can use +1 to mark the Beginning-Of-Tuple (or EOT), and so distinguish
amongst the N-tuples, and reconstruct each record by copying and
changing the previous record. Each new module block starts with a
4-tuple; the init/main example block above is a 4,1,1,1,2,2-tuple
sequence, using 11/24 of the space.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 5980d44ff2f8..c84efb4e036b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1079,6 +1079,13 @@ static int __init dynamic_debug_init_control(void)
return 0;
}
+
+static __initdata int alignments[4][8];
+static void bump_ct(int i, const char *p)
+{
+ alignments[i][ (long)p & 7 ]++;
+}
+
static int __init dynamic_debug_init(void)
{
struct _ddebug *iter, *iter_start, *prev = NULL;
@@ -1087,7 +1094,9 @@ static int __init dynamic_debug_init(void)
int ret = 0;
int n = 0, entries = 0, modct = 0;
int modreps = 0, funcreps = 0, filereps = 0;
-
+ int i,k;
+ char *kind[] = { "mod","file","func","format" };
+
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
pr_warn("_ddebug table is empty in a
CONFIG_DYNAMIC_DEBUG build\n");
@@ -,6 +1120,12 @@ static int __init dynamic_debug_init(void)
prev++; /* one behind iter */
}
entries++;
+
+ bump_ct(0, iter->site->modname);
+ bump_ct(1, iter->site->filename);
+ bump_ct(2, iter->site->function);
+ bump_ct(3, iter->site->format);
+
if (strcmp(modname, iter->site->modname)) {
modct++;
Re: [PATCH] locking-selftest: add option to proceed through unexpected failures
On Tue, Nov 17, 2020 at 3:02 AM Peter Zijlstra wrote: > > On Fri, Nov 13, 2020 at 04:00:48PM -0700, Jim Cromie wrote: > > Locking selftest is currently (v.5.10-rc3) seeing 14 unexpected failures. > > Add option to not disable debug_locks, so as to let it reveal any > > locking flaws in new unrelated work. > > I'm assuming this is the arm64 fallout? Mark anything I can do to help > you there? > > The reasoning doesn't make sense though; if it can't pass the selftest, > then why would you trust any further reports? because it knows more than I do. having chosen to proceed anyway, Im free to discount the advice.
[PATCH] dyndbg: fix use before null check
commit a2d375eda771 ("dyndbg: refine export, rename to
dynamic_debug_exec_queries()")
Above commit copies a string before checking for null pointer, fix
this, and add a pr_err. Also trim comment, and add return val info.
Fixes: a2d375eda771
Cc: sta...@vger.kernel.org
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index bd7b3aaa93c3..711a9def8c83 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -553,17 +553,23 @@ static int ddebug_exec_queries(char *query, const char
*modname)
* @query: query-string described in admin-guide/dynamic-debug-howto
* @modname: string containing module name, usually _name
*
- * This uses 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*".
+ * This uses the >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*".
+ * Returns <0 on error, >=0 for callsites changed
*/
int dynamic_debug_exec_queries(const char *query, const char *modname)
{
int rc;
- char *qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ char *qry; /* writable copy of query */
- if (!query)
+ if (!query) {
+ pr_err("non-null query/command string expected\n");
+ return -EINVAL;
+ }
+ qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ if (!qry)
return -ENOMEM;
rc = ddebug_exec_queries(qry, modname);
--
2.28.0
[PATCH] locking-selftest: add option to proceed through unexpected failures
Locking selftest is currently (v.5.10-rc3) seeing 14 unexpected failures.
Add option to not disable debug_locks, so as to let it reveal any
locking flaws in new unrelated work.
Signed-off-by: Jim Cromie
---
lib/locking-selftest.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
index a899b3f0e2e5..87889bcf3232 100644
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -41,6 +41,20 @@ static int __init setup_debug_locks_verbose(char *str)
__setup("debug_locks_verbose=", setup_debug_locks_verbose);
+/*
+ * proceed thru debug fails, leave debugging enabled
+ */
+static unsigned int debug_locks_proceed;
+
+static int __init setup_debug_locks_proceed(char *str)
+{
+ get_option(, _locks_proceed);
+
+ return 1;
+}
+
+__setup("debug_locks_proceed=", setup_debug_locks_proceed);
+
#define FAILURE0
#define SUCCESS1
@@ -2480,9 +2494,10 @@ void locking_selftest(void)
if (unexpected_testcase_failures) {
printk("-\n");
- debug_locks = 0;
- printk("BUG: %3d unexpected failures (out of %3d) - debugging
disabled! |\n",
- unexpected_testcase_failures, testcase_total);
+ debug_locks = debug_locks_proceed;
+ printk("BUG: %3d unexpected failures (out of %3d) - debugging
%s! |\n",
+ unexpected_testcase_failures, testcase_total,
+ (!debug_locks_proceed) ? "disabled" : "proceeding
anyway");
printk("-\n");
} else if (expected_testcase_failures && testcase_successes) {
printk("\n");
--
2.28.0
Re: [PATCH 1/2] dyndbg: dont panic over bad input
On Tue, Sep 22, 2020 at 2:08 AM Petr Mladek wrote:
>
> On Mon 2020-09-21 13:04:32, Jim Cromie wrote:
> > This BUG_ON, from 2009, caught the impossible case of a word-char both
> > starting and ending a string (loosely speaking). A bad (reverted)
> > patch finally hit this case, but even "impossibly bad input" is no
> > reason to panic the kernel. Instead pr_err and return -EINVAL.
> >
> > Reported-by: Petr Mladek
> > Signed-off-by: Jim Cromie
> > ---
> > lib/dynamic_debug.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 2d4dfd44b0fa5..90ddf07ce34fe 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[],
> > int maxwords)
> > } else {
> > for (end = buf; *end && !isspace(*end); end++)
> > ;
> > - BUG_ON(end == buf);
> > + if (end == buf) {
> > + pr_err("expected non-empty bareword");
> > + return -EINVAL;
>
> My understanding is that the BUG_ON() was there to catch eventual bugs
> in the algorithm.
>
> IMHO, it was never reachable in the original code:
>
>1. The following lines will skip all spaces and bail out
> when we reached the trailing '\0':
>
> buf = skip_spaces(buf);
> if (!*buf)
> break; /* oh, it was trailing whitespace */
>
>
>2. The following code will find the end of a quoted text:
>
> if (*buf == '"' || *buf == '\'') {
> int quote = *buf++;
> for (end = buf; *end && *end != quote; end++)
>
>
> 3. The else part will find end of non-quoted word:
>
> } else {
> for (end = buf; *end && !isspace(*end); end++)
>
> 4. The BUG_ON() will trigger when the above cycle reached the
>trailing '\0' or space.
>
>This will never happen because this situation was caught
>in the 1st step.
>
>
> Your patch triggered the BUG_ON() because it wanted to handle
> '=' as a special character and was incomplete.
>
> If you want to handle '=' special way. You need to do it the same way
> as with the space. You need to skip it in 1st step. And it must mark
> the end of the word in 4th step.
>
> But it will be more complicated. You must be able to handle
> mix of spaces and '='. I mean the following situations:
>
> word=word
> word =word
> word= word
> word = word
> word = = word /* failure ? */
>
>
> Back to the BUG_ON(). It might be changed to something like:
>
> pr_err("Internal error when parsing dynamic debug query\n");
> return -EINVAL;
>
>
> Best Regards,
> Petr
yes, the original patch was ill conceived
Im blaming Transient Acute Myopia,
where I couldnt see to the end of the line, where "=flags"
is a proper flag setting.
That "interferes" with using '=' to separate keyword=value.
As you outlined, its possible to implement something that handles both,
but I decided that handling keyword=value is just a convenience feature,
and isnt worth the added corner-cases and explanatory burden.
Re: [PATCH 1/2] dyndbg: dont panic over bad input
On Mon, Sep 21, 2020 at 1:29 PM Joe Perches wrote:
>
> On Mon, 2020-09-21 at 13:04 -0600, Jim Cromie wrote:
> > This BUG_ON, from 2009, caught the impossible case of a word-char both
> > starting and ending a string (loosely speaking). A bad (reverted)
> > patch finally hit this case, but even "impossibly bad input" is no
> > reason to panic the kernel. Instead pr_err and return -EINVAL.
> []
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> []
> > @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[],
> > int maxwords)
> > } else {
> > for (end = buf; *end && !isspace(*end); end++)
> > ;
> > - BUG_ON(end == buf);
> > + if (end == buf) {
> > + pr_err("expected non-empty bareword");
>
> missing newline
>
> This message is also unintelligible.
> What is a non-empty bareword?
>
>
hmm, I borrowed the term from perl
en.wiktionary.org › wiki › bareword
(programming, chiefly Perl) A sequence of text characters in source
code that do not form a keyword nor part of a quoted string, and may
potentially be interpreted ...
basically, a not-quoted word, a keyword or not-quoted-value
Im open that there might be better terminology.
have any suggestions ?
[PATCH 0/2] dyndbg: 2 fixes/cleanups for 5.9
2 things which might qualify as fixes a bad (reverted) patch hit a BUG_ON, change that to return -EINVAL instead keep useful part of the reverted patch, use keyword, arg varnames Jim Cromie (2): dyndbg: dont panic over bad input dyndbg: use keyword, arg varnames for query term pairs lib/dynamic_debug.c | 32 +++- 1 file changed, 19 insertions(+), 13 deletions(-) -- 2.26.2
[PATCH 2/2] dyndbg: use keyword, arg varnames for query term pairs
optimize for clarity by replacing word[i,i+1] refs with temps.
no functional changes.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 90ddf07ce34fe..23e7dd967b0e6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -387,10 +387,13 @@ static int ddebug_parse_query(char *words[], int nwords,
query->module = modname;
for (i = 0; i < nwords; i += 2) {
- if (!strcmp(words[i], "func")) {
- rc = check_set(>function, words[i+1], "func");
- } else if (!strcmp(words[i], "file")) {
- if (check_set(>filename, words[i+1], "file"))
+ char *keyword = words[i];
+ char *arg = words[i+1];
+
+ if (!strcmp(keyword, "func")) {
+ rc = check_set(>function, arg, "func");
+ } else if (!strcmp(keyword, "file")) {
+ if (check_set(>filename, arg, "file"))
return -EINVAL;
/* tail :$info is function or line-range */
@@ -406,18 +409,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(>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(>module, arg, "module");
+ } else if (!strcmp(keyword, "format")) {
+ string_unescape_inplace(arg, UNESCAPE_SPACE |
UNESCAPE_OCTAL |
UNESCAPE_SPECIAL);
- rc = check_set(>format, words[i+1], "format");
- } else if (!strcmp(words[i], "line")) {
- if (parse_linerange(query, words[i+1]))
+ rc = check_set(>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 1/2] dyndbg: dont panic over bad input
This BUG_ON, from 2009, caught the impossible case of a word-char both
starting and ending a string (loosely speaking). A bad (reverted)
patch finally hit this case, but even "impossibly bad input" is no
reason to panic the kernel. Instead pr_err and return -EINVAL.
Reported-by: Petr Mladek
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2d4dfd44b0fa5..90ddf07ce34fe 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
} else {
for (end = buf; *end && !isspace(*end); end++)
;
- BUG_ON(end == buf);
+ if (end == buf) {
+ pr_err("expected non-empty bareword");
+ return -EINVAL;
+ }
}
/* `buf' is start of word, `end' is one past its end */
--
2.26.2
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
> > > > Please you should not be using netdev_info(). netdev_dbg() please. > > > > I changed most netif_msg_*()+netdev_*() to netif_*(), including > netif_dbg() in several places. However, after reading other drivers I > decided to leave this at INFO level. I think this is the way to go, > because this is what user asks for and with dynamic debug enabled users > would have to ask for these in two different places. dynamic_debug_exec_queries is now exported, allowing module authors full >control of their debug
Re: kernel BUG at /usr/src/kernel/lib/dynamic_debug.c:267!
On Wed, Sep 9, 2020 at 6:24 AM Greg Kroah-Hartman wrote: > > On Wed, Sep 09, 2020 at 10:00:25AM +0200, Petr Mladek wrote: > > On Wed 2020-09-09 14:47:45, Masami Hiramatsu wrote: > > > Hi Naresh, > > > > > > Thanks for reporting, it seems that you have run the > > > kselftests/livepatch/test-livepatch.sh. > > > Then, I think it is better to report to Livepatch maintainers too. (I > > > Cc'd them) > > > > > > Thank you, > > > > > > On Wed, 9 Sep 2020 10:24:09 +0530 > > > Naresh Kamboju wrote: > > > > > > > While testing livepatch test cases on x86_64 with Linux next 20200908 > > > > tag kernel > > > > this kernel BUG noticed several times. > > > > > > > > metadata: > > > > git branch: master > > > > git repo: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git > > > > git commit: dff9f829e5b0181d4ed9d35aa62d695292399b54 > > > > git describe: next-20200908 > > > > kernel-config: > > > > http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/intel-corei7-64/lkft/linux-next/853/config > > > > > > > > kernel BUG log, > > > > -- > > > > [ 634.063970] % rmmod test_klp_livepatch > > > > [ 634.114787] test_klp_atomic_replace: this has been live patched > > > > [ 634.121953] % echo 0 > > > > > /sys/kernel/livepatch/test_klp_atomic_replace/enabled > > > > [ 634.129391] livepatch: 'test_klp_atomic_replace': starting > > > > unpatching transition > > > > [ 634.143990] livepatch: 'test_klp_atomic_replace': unpatching complete > > > > [ 634.156223] % rmmod test_klp_atomic_replace > > > > [ 634.235451] [ cut here ] > > > > [ 634.240314] kernel BUG at /usr/src/kernel/lib/dynamic_debug.c:267! > > > > [ 634.246584] invalid opcode: [#1] SMP PTI > > > > [ 634.250955] CPU: 0 PID: 12791 Comm: test-livepatch. Tainted: G > > > > W K 5.9.0-rc4-next-20200908 #1 > > > > [ 634.260615] Hardware name: Supermicro SYS-5019S-ML/X11SSH-F, BIOS > > > > 2.2 05/23/2018 > > > > [ 634.268007] RIP: 0010:ddebug_exec_query+0x77b/0xb90 > > > > [ 634.272886] Code: 4c 89 ad 70 ff ff ff e9 db fb ff ff b8 03 00 00 > > > > 00 e9 20 fb ff ff b8 02 00 00 00 e9 16 fb ff ff b8 01 00 00 00 e9 0c > > > > fb ff ff <0f> 0b 31 c0 e9 03 fb ff ff 49 89 f4 48 89 f7 e9 78 f9 ff ff > > > > 8b 15 > > > > [ 634.291630] RSP: 0018:9f0c80a5bd18 EFLAGS: 00010246 > > > > [ 634.296856] RAX: 003d RBX: RCX: > > > > > > > > [ 634.303987] RDX: 003d RSI: RDI: > > > > 90db906583ec > > > > [ 634.31] RBP: 9f0c80a5bde8 R08: 000a R09: > > > > 003b > > > > [ 634.318236] R10: 90db9261 R11: 0246 R12: > > > > 90db906583ec > > > > [ 634.325368] R13: be87cbc0 R14: R15: > > > > 0004 > > > > [ 634.332500] FS: 7fd37249a740() GS:90dbefa0() > > > > knlGS: > > > > [ 634.340578] CS: 0010 DS: ES: CR0: 80050033 > > > > [ 634.346315] CR2: 00e6d00c CR3: 00026a4b8004 CR4: > > > > 003706f0 > > > > [ 634.353446] DR0: DR1: DR2: > > > > > > > > [ 634.360570] DR3: DR6: fffe0ff0 DR7: > > > > 0400 > > > > [ 634.367693] Call Trace: > > > > [ 634.370139] ? lock_acquire+0xa6/0x390 > > > > [ 634.373892] ? __might_fault+0x34/0x80 > > > > [ 634.377648] ddebug_exec_queries+0x6e/0x140 > > > > [ 634.381831] ddebug_proc_write+0x4b/0xa0 > > > > [ 634.385756] full_proxy_write+0x5f/0x90 > > > > [ 634.389595] vfs_write+0xed/0x240 > > > > [ 634.392915] ksys_write+0xad/0xf0 > > > > [ 634.396233] ? syscall_trace_enter+0x17a/0x240 > > > > [ 634.400670] __x64_sys_write+0x1a/0x20 > > > > [ 634.404416] do_syscall_64+0x37/0x50 > > > > [ 634.407993] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > > [ 634.413038] RIP: 0033:0x7fd371b84144 > > > > [ 634.416617] Code: 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 > > > > 00 00 00 00 66 90 48 8d 05 c1 e7 2c 00 8b 00 85 c0 75 13 b8 01 00 00 > > > > 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 > > > > f5 53 > > > > [ 634.435362] RSP: 002b:7ffd19447658 EFLAGS: 0246 ORIG_RAX: > > > > 0001 > > > > [ 634.442928] RAX: ffda RBX: 00bc RCX: > > > > 7fd371b84144 > > > > [ 634.450059] RDX: 00bc RSI: 00e6cf70 RDI: > > > > 0001 > > > > [ 634.457181] RBP: 00e6cf70 R08: 00e9 R09: > > > > > > > > [ 634.464305] R10: 7ffd19447c48 R11: 0246 R12: > > > > 7fd371e4e760 > > > > [ 634.471429] R13: 00bc R14: 7fd371e49760 R15: > > > > 00bc > > > > [ 634.478559] Modules linked in: trace_printk sch_fq 8021q > > > > iptable_filter xt_mark ip_tables cls_bpf sch_ingress veth algif_hash > > > > x86_pkg_temp_thermal fuse [last
Re: [dyndbg] 70f06a871f: kernel_BUG_at_lib/dynamic_debug.c
Got it.
will investigate asap
On Wed, Sep 2, 2020 at 3:42 AM kernel test robot wrote:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 70f06a871f5d40ca8f977eb412358ab03b6804da ("[PATCH v3 3/3] dyndbg: fix
> problem parsing format="foo bar"")
> url:
> https://github.com/0day-ci/linux/commits/Jim-Cromie/dyndbg-cleanups-for-5-9/20200901-022403
> base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git
> f75aef392f869018f78cfedf3c320a6b3fcfda6b
>
> in testcase: kernel-selftests
> with following parameters:
>
> group: kselftests-livepatch
>
> test-description: The kernel contains a set of "self tests" under the
> tools/testing/selftests/ directory. These are intended to be small unit tests
> to exercise individual code paths in the kernel.
> test-url: https://www.kernel.org/doc/Documentation/kselftest.txt
>
>
> on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 8G
>
> caused below changes (please refer to attached dmesg/kmsg for entire
> log/backtrace):
>
>
> +--+++
> | | 12aeaa9439 | 70f06a871f |
> +--+++
> | boot_successes | 6 | 2 |
> | boot_failures| 0 | 4 |
> | kernel_BUG_at_lib/dynamic_debug.c| 0 | 4 |
> | invalid_opcode:#[##] | 0 | 4 |
> | RIP:ddebug_exec_query| 0 | 4 |
> | Kernel_panic-not_syncing:Fatal_exception | 0 | 4 |
> +--+++
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot
>
>
> [ 78.796907] kernel BUG at lib/dynamic_debug.c:267!
> [ 78.799930] invalid opcode: [#1] PREEMPT SMP PTI
> [ 78.801632] CPU: 1 PID: 1068 Comm: test-livepatch. Tainted: G
> K 5.9.0-rc3-3-g70f06a871f5d4 #1
> [ 78.803877] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.12.0-1 04/01/2014
> [ 78.805929] RIP: 0010:ddebug_exec_query+0x684/0x760
> [ 78.807642] Code: 0f 85 c0 08 00 00 4c 89 7c 24 28 e9 2d fc ff ff 4c 89 fe
> 48 8d 7c 24 10 e8 a9 f8 ff ff 85 c0 0f 84 18 fc ff ff e9 c1 07 00 00 <0f> 0b
> 8b 15 ac 37 e2 02 85 d2 0f 85 c1 00 00 00 45 31 e4 48 c7 c6
> [ 78.812169] RSP: 0018:b325411c7d78 EFLAGS: 00010246
> [ 78.815411] RAX: 003d RBX: 9f4dab69286c RCX:
>
> [ 78.821166] RDX: 003d RSI: 9f4dab692868 RDI:
> 9f4dab69286c
> [ 78.823160] RBP: 0004 R08: 9f4dab69286e R09:
> 0001
> [ 78.825172] R10: R11: 0246 R12:
> 0004
> [ 78.827212] R13: 0004 R14: R15:
>
> [ 78.829228] FS: 7f024c29c740() GS:9f4e77d0()
> knlGS:
> [ 78.831343] CS: 0010 DS: ES: CR0: 80050033
> [ 78.833243] CR2: 7f024c45c8a0 CR3: 00016836 CR4:
> 000406e0
> [ 78.835312] DR0: DR1: DR2:
>
> [ 78.837371] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 78.839398] Call Trace:
> [ 78.840935] ? __might_fault+0x36/0x80
> [ 78.842574] ddebug_exec_queries+0x6a/0x100
> [ 78.844322] ddebug_proc_write+0x4e/0x80
> [ 78.845985] full_proxy_write+0x56/0x80
> [ 78.847621] vfs_write+0xec/0x240
> [ 78.849189] ksys_write+0x68/0xe0
> [ 78.850738] do_syscall_64+0x33/0x40
> [ 78.852347] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 78.854106] RIP: 0033:0x7f024c389504
> [ 78.855634] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00
> 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d
> 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
> [ 78.860364] RSP: 002b:7fff5962d2b8 EFLAGS: 0246 ORIG_RAX:
> 0001
> [ 78.864609] RAX: ffda RBX: 00bc RCX:
> 7f024c389504
> [ 78.866696] RDX: 00bc RSI: 5582ce63dcd0 RDI:
> 0001
> [ 78.868751] RBP: 5582ce63dcd0 R08: fff0 R09:
> 7f024c419e80
> [ 78.870832] R10: 5582ce63dd8c R11: 0246 R12:
> 7f024c45b760
> [ 78.872933] R13: 00bc R14: 7f024c456760 R15:
> 00bc
> [ 78.875077] Modules linked in: intel_rapl_msr intel_rap
[PATCH v3 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(>link, _tables);
mutex_unlock(_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 v3 0/3] dyndbg: cleanups for 5.9
3 items for v5.9-rc fixed width format %3u in debug output - cosmetic fix export: ddebug_exec_queries -> dynamic_debug_exec_queries fix format="foo bar" parsing 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 | 67 ++- 2 files changed, 59 insertions(+), 28 deletions(-) -- 2.26.2
[PATCH v3 2/3] dyndbg: refine export, rename to dynamic_debug_exec_queries()
commit 4c0d77828d4f ("dyndbg: export ddebug_exec_queries")
had a few problems:
- broken non DYNAMIC_DEBUG_CORE configs, sparse warning
- the exported function modifies query string, breaks on RO strings.
- func name follows internal convention, shouldn't be exposed as is.
1st is fixed in header with ifdefd function prototype or stub defn.
Also remove an obsolete HAVE-symbol ifdef-comment, and add others.
Fix others by wrapping existing internal function with a new one,
named in accordance with module-prefix naming convention, before
export hits v5.9.0. In new function, copy query string to a local
buffer, so users can pass hard-coded/RO queries, and internal function
can be used unchanged.
Fixes: 4c0d77828d4f ("dyndbg: export ddebug_exec_queries")
---
v2- code improvements, per Joe Perches
v3- commit message fixups
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 20
lib/dynamic_debug.c | 27 +--
2 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa9ff9e1c0b3..8aa0c7c2608c 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(_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)
+{
+ pr_warn("kernel not built with 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..08e4b057514c 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,30 @@ int ddebug_exec_queries(char *query, const char *modname)
return exitcode;
return nfound;
}
-EXPORT_SYMBOL_GPL(ddebug_exec_queries);
+
+/**
+ * dynamic_debug_exec_queries - select and change dynamic-debug prints
+ * @query: query-string described in admin-guide/dynamic-debug-howto
+ * @modname: string containing module name, usually _name
+ *
+ * This uses 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)
+{
+ int rc;
+ char *qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+
+ if (!query)
+ return -ENOMEM;
+
+ 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 v3 3/3] dyndbg: fix problem parsing format="foo bar"
commit 14775b049642 ("dyndbg: accept query terms like file=bar and module=foo")
added the combined keyword=value parsing poorly; revert most of it,
keeping the keyword & arg change.
Instead, fix the tokenizer for the new input, by terminating the
keyword (an unquoted word) on '=' as well as space, thus letting the
tokenizer work on the quoted argument, like it would have previously.
Also add a few debug-prints to show more parsing context, into
tokenizer and parse-query, and use "keyword, value" in others.
Fixes: 14775b049642 ("dyndbg: accept query terms like file=bar and module=foo")
---
-v3 commit message, checkpatch fixes
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 38 +-
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 08e4b057514c..04f4c80b0d16 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,21 @@ static int ddebug_parse_query(char *words[], int nwords,
unsigned int i;
int rc = 0;
char *fline;
- char *keyword, *arg;
- if (modname)
+ if (nwords % 2 != 0) {
+ pr_err("expecting pairs of match-spec \n");
+ return -EINVAL;
+ }
+ if (modname) {
/* support $modname.dyndbg= */
+ vpr_info("module:%s queries:'%s'\n", modname);
query->module = modname;
+ }
+ for (i = 0; i < nwords; i += 2) {
+ char *keyword = words[i];
+ char *arg = words[i+1];
- 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);
-
+ vpr_info("keyword:'%s' value:'%s'\n", keyword, arg);
if (!strcmp(keyword, "func")) {
rc = check_set(>function, arg, "func");
} else if (!strcmp(keyword, "file")) {
--
2.26.2
[PATCH 0/4] dyndbg: POC use dynamic_debug_exec_queries in DRM
This patchset tests/demonstrates using dynamic_debug_exec_queries() to alter 2 drivers' pr_debugs without a user directly using >control. For drm.core, I copied drm.debug module parameter model, adding drm.debug2 as a POC user interface to control 2 pr_debug additions to drm_printk:drm_dbg,dev_dbg, allowing both category and >control to work in parallel. This patch makes no attempt to integrate drm's category mechanism with the "format=^class" callsite selection; thats the "theory", but it needs testing. For i915/gvt, I repeated the pattern. I focussed on gvt only, because it had the most compelling use of format strings as pr_debug classes; ~120 pr_debugs in 9 classes. These are mapped onto bits of the param. bash-5.0# echo 0x0 > /sys/module/i915/parameters/debug_dyn [ 3137.044185] set_dyndbg: result:0x0 from 0x0 [ 3137.044185] [ 3137.047370] dyndbg: query 0: "format='^gvt: cmd: ' -p" [ 3137.050302] dyndbg: entry, buf:'format='^gvt: cmd: ' -p' [ 3137.053331] dyndbg: start-of-word:0 'format='^gvt: cmd: ' -p' These patches were the test/use case for-59 fixes: https://lore.kernel.org/lkml/20200825173339.2082585-1-jim.cro...@gmail.com/ Jim Cromie (4): drm-printk: POC caller of dynamic-debug-exec-queries drm-printk: call pr_debug() from drm_dev_dbg, __drm_dbg i915: add -DDYNAMIC_DEBUG_MODULE to i915/gvt/Makefile i915: POC use dynamic_debug_exec_queries to control pr_debugs in gvt drivers/gpu/drm/drm_print.c| 54 ++--- drivers/gpu/drm/i915/gvt/Makefile | 5 +- drivers/gpu/drm/i915/i915_params.c | 76 ++ 3 files changed, 127 insertions(+), 8 deletions(-) -- 2.26.2
[PATCH 4/4] i915: POC use dynamic_debug_exec_queries to control pr_debugs in gvt
The gvt component of this driver has ~120 pr_debugs, in 9 "classes".
Add a "knob", like drm.debug, to map bits to these classes.
bash-5.0# echo 0x01 > /sys/module/i915/parameters/debug_dyn
set_dyndbg: result:0x1 from 0x01
dyndbg: query 0: "format='^gvt: cmd: ' +p"
dyndbg: entry, buf:'format='^gvt: cmd: ' +p'
dyndbg: start-of-word:0 'format='^gvt: cmd: ' +p'
dyndbg: start-of-word:1 ''^gvt: cmd: ' +p'
dyndbg: start-of-word:2 '+p'
dyndbg: split into words: "format" "^gvt: cmd: " "+p"
dyndbg: op='+'
dyndbg: flags=0x1
dyndbg: *flagsp=0x1 *maskp=0x
dyndbg: key:'format' arg:'^gvt: cmd: '
dyndbg: parsed: func="" file="" module="i915" format="^gvt: cmd: " lineno=0-0
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:3081
[i915]init_cmd_table =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:1376
[i915]gen8_check_mi_display_flip =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:1373
[i915]gen8_check_mi_display_flip =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:745
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:744
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:742
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:733
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:729
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:722
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:716
[i915]parser_exec_state_dump =p
dyndbg: changed drivers/gpu/drm/i915/gvt/cmd_parser.c:691 [i915]print_opcode =p
dyndbg: applied: func="" file="" module="i915" format="^gvt: cmd: " lineno=0-0
dyndbg: processed 1 queries, with 11 matches, 0 errs
change ct:11 on format='gvt: cmd: '
change ct:11
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/i915_params.c | 76 ++
1 file changed, 76 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 8d8db9ff0a48..4e1f01ab4865 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -255,3 +255,79 @@ void i915_params_free(struct i915_params *params)
I915_PARAMS_FOR_EACH(FREE);
#undef FREE
}
+
+/* POC for callback -> dynamic_debug_exec_queries */
+unsigned long __new_knob;
+EXPORT_SYMBOL(__new_knob);
+
+static char *pr_debug_classes[] = {
+ "gvt: cmd: ",
+ "gvt: core: ",
+ "gvt: dpy: ",
+ "gvt: el: ",
+ "gvt: irq: ",
+ "gvt: mm: ",
+ "gvt: mmio: ",
+ "gvt: render: ",
+ "gvt: sched: "
+};
+#define NUM_CLASSESARRAY_SIZE(pr_debug_classes)
+#define OUR_QUERY_SIZE 128 /* we need about 20 */
+
+#include
+
+static int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{
+ static unsigned long int old_val;
+unsigned int val;
+ unsigned long int changes, result;
+ int rc, chgct = 0, totct = 0, bitpos;
+ char query[OUR_QUERY_SIZE];
+
+ rc = kstrtouint(instr, 0, );
+ if (rc) {
+ pr_err("set_dyndbg: failed\n");
+ return -EINVAL;
+ }
+ result = val;
+ pr_info("set_dyndbg: result:0x%lx from %s\n", result, instr);
+
+ changes = result ^ old_val;
+
+ for_each_set_bit(bitpos, , NUM_CLASSES) {
+
+ sprintf(query, "format='^%s' %cp", pr_debug_classes[bitpos],
+ test_bit(bitpos, ) ? '+' : '-');
+
+ chgct = dynamic_debug_exec_queries(query, "i915");
+ totct += chgct;
+ pr_info("change ct:%d on format='%s'\n", chgct,
+ pr_debug_classes[bitpos]);
+ }
+ old_val = result;
+ pr_info("change ct:%d\n", totct);
+ return 0;
+}
+static int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
+{
+ return scnprintf(buffer, PAGE_SIZE, "%u\n",
+*((unsigned int *)kp->arg));
+}
+static const struct kernel_param_ops param_ops_dyndbg = {
+ .set = param_set_dyndbg,
+ .get = param_get_dyndbg,
+};
+
+MODULE_PARM_DESC(debug_dyn, " enable dynamic-debug by format-string
classifications.\n"
+"\t\twhich are:"
+"\n\t\t gvt: cmd:"
+"\n\t\t gvt: core:"
+"\n\t\t gvt: dpy:"
+"\n\t\t gvt: el:"
+"\n\t\t gvt: irq:"
+"\n\t\t gvt: mm:"
+"\n\t\t gvt: mmio:"
+"\n\t\t gvt: render:"
+"\n\t\t gvt: sched:" "\n");
+
+module_param_cb(debug_dyn, _ops_dyndbg, &__new_knob, 0644);
--
2.26.2
[PATCH 1/4] drm-printk: POC caller of dynamic-debug-exec-queries
Export of dynamic-debug-exec-queries exists for users like drm.
This commit is a 1st user-test; it adds a 2nd knob, __drm_debug2,
similar in function to __drm_debug. module_param_cb wires it to a
callback which maps the input value to one of: "module=drm* +/-p".
The include is needed to see the exported function prototype.
Notes:
The define DEBUG at top of drm-printk enables all pr_debug()s,
independent of CONFIG_DYNAMIC_DEBUG_CORE.
drm-printk is an independent print control system using __drm_debug
bits. The plan is to find the low-level macros in which to insert a
pr_debug call, their eventual callsites will have distinct METADATA,
so will be itemized in control, and individually selectable.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/drm_print.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 111b932cf2a9..52abaf2ae053 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -27,6 +27,7 @@
#include
+#include
#include
#include
#include
@@ -54,6 +55,40 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit
enables a debug cat
"\t\tBit 8 (0x100) will enable DP messages (displayport code)");
module_param_named(debug, __drm_debug, int, 0600);
+/* POC for callback -> ddebug_exec_queries */
+unsigned int __drm_debug2;
+EXPORT_SYMBOL(__drm_debug2);
+
+static int param_set_dyndbg(const char *val, const struct kernel_param *kp)
+{
+ int chgct, result;
+
+ result = kstrtouint(val, 0, (unsigned int *)kp->arg);
+ pr_warn("set_dyndbg: result:%d from %s\n", result, val);
+
+ if (result)
+ chgct = dynamic_debug_exec_queries("module=drm* +p", NULL);
+ else
+ chgct = dynamic_debug_exec_queries("module=drm* -p", NULL);
+
+ pr_warn("change ct:%d\n", chgct);
+ return 0;
+}
+static int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
+{
+ return scnprintf(buffer, PAGE_SIZE, "%u\n",
+*((unsigned int *)kp->arg));
+}
+static const struct kernel_param_ops param_ops_dyndbg = {
+ .set = param_set_dyndbg,
+ .get = param_get_dyndbg,
+};
+
+MODULE_PARM_DESC(debug_dyn, "Enable debug output, where each bit enables a
debug category.\n"
+"\t\tthese wont work yet\n");
+module_param_cb(debug_dyn, _ops_dyndbg, &__drm_debug2, 0644);
+
+
void __drm_puts_coredump(struct drm_printer *p, const char *str)
{
struct drm_print_iterator *iterator = p->arg;
--
2.26.2
[PATCH 2/4] drm-printk: call pr_debug() from drm_dev_dbg, __drm_dbg
Put the pr_debug() after the vaf setup work, so as to use it. And
move the if-category-disabled-return after both, so the pr_debug()
runs unconditionally.
This lets both debug-printers execute independently, according to
their respective controls, allowing later comparison to each other.
#> echo module=drm +pmftl > /proc/dynamic_debug/control
yields logging like:
[1772] drm:drm_dev_dbg:305: i915 :00:02.0:
[drm:intel_atomic_get_global_obj_state [i915]]
Cat:16 Added new global object 6fa51dd6 state bbddcf9d to
5f6e1bc3
[1772] drm:drm_dev_dbg:305: i915 :00:02.0:
[drm:intel_atomic_get_global_obj_state [i915]]
Cat:16 Added new global object 2a5e020a state 2b3685ed to
5f6e1bc3
[198] drm:drm_dev_dbg:305: i915 :00:02.0: [drm:drm_update_vblank_count
[drm]]
Cat:32 updating vblank count on crtc 0: current=260920, diff=0, hw=192024
hw_last=192024
[1772] drm:__drm_dbg:331: [drm:drm_atomic_nonblocking_commit [drm]] Cat:16
committing 5f6e1bc3 nonblocking
[1772] drm:__drm_dbg:331: [drm:drm_mode_object_put.part.0 [drm]] Cat:1 OBJ ID:
113 (4)
[1772] drm:__drm_dbg:331: [drm:drm_ioctl [drm]] Cat:1 comm="gnome-shell"
pid=1772, dev=0xe200, auth=1, I915_GEM_CREATE
[1772] drm:__drm_dbg:331: [drm:drm_ioctl [drm]] Cat:1 comm="gnome-shell"
pid=1772, dev=0xe200, auth=1, I915_GEM_SET_DOMAIN
[1772] drm:__drm_dbg:331: [drm:drm_ioctl [drm]] Cat:1 comm="gnome-shell"
pid=1772, dev=0xe200, auth=1, I915_GEM_SET_TILING
[1772] drm:__drm_dbg:331: [drm:drm_ioctl [drm]] Cat:1 comm="gnome-shell"
pid=1772, dev=0xe200, auth=1, I915_GEM_MMAP_OFFSET
Clearly, the mflt flags arent very helpful here, and callsite control
is all or nothing (since theres only 2 callsites handling all the
categories). We are 1 below the function layer of interest, but
theres room for optimism.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/drm_print.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 52abaf2ae053..fa2bcf4ec475 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -27,6 +27,7 @@
#include
+#define DYNAMIC_DEBUG_MODULE
#include
#include
#include
@@ -61,17 +62,17 @@ EXPORT_SYMBOL(__drm_debug2);
static int param_set_dyndbg(const char *val, const struct kernel_param *kp)
{
- int chgct, result;
+ int chgct, rc, result;
- result = kstrtouint(val, 0, (unsigned int *)kp->arg);
- pr_warn("set_dyndbg: result:%d from %s\n", result, val);
+ rc = kstrtouint(val, 0, );
+ pr_debug("set_dyndbg: rc:%d got:%d from %s\n", rc, result, val);
if (result)
chgct = dynamic_debug_exec_queries("module=drm* +p", NULL);
else
chgct = dynamic_debug_exec_queries("module=drm* -p", NULL);
- pr_warn("change ct:%d\n", chgct);
+ pr_debug("change ct:%d\n", chgct);
return 0;
}
static int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
@@ -297,13 +298,16 @@ void drm_dev_dbg(const struct device *dev, enum
drm_debug_category category,
struct va_format vaf;
va_list args;
- if (!drm_debug_enabled(category))
- return;
-
va_start(args, format);
vaf.fmt = format;
vaf.va =
+ dev_dbg(dev, "[" DRM_NAME ":%ps] Cat:%d %pV",
+ __builtin_return_address(0), category, );
+
+ if (!drm_debug_enabled(category))
+ return;
+
if (dev)
dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
__builtin_return_address(0), );
@@ -320,13 +324,16 @@ void __drm_dbg(enum drm_debug_category category, const
char *format, ...)
struct va_format vaf;
va_list args;
- if (!drm_debug_enabled(category))
- return;
-
va_start(args, format);
vaf.fmt = format;
vaf.va =
+ pr_debug("[" DRM_NAME ":%ps] Cat:%d %pV",
+__builtin_return_address(0), category, );
+
+ if (!drm_debug_enabled(category))
+ return;
+
printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
__builtin_return_address(0), );
--
2.26.2
[PATCH 3/4] i915: add -DDYNAMIC_DEBUG_MODULE to i915/gvt/Makefile
This addition to cflags enables dyndbg in the gvt component of the
i915 module, on a CONFIG_DYNAMIC_DEBUG_CORE build.
So here are the message classifications that the gvt driver uses.
cut -d= -f2 | cut -d\ -f2,3 | \
perl -ne 'chomp $_ && $h{$_}++; END{print "$_\" \tseen $h{$_}\n" for sort
keys %h}'
"gvt: cmd:" seen 11
"gvt: core:"seen 48
"gvt: dpy:" seen 4
"gvt: el:" seen 21
"gvt: irq:" seen 1
"gvt: mm:" seen 6
"gvt: mmio:"seen 9
"gvt: render:" seen 1
"gvt: sched:" seen 15
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/i915/gvt/Makefile | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/Makefile
b/drivers/gpu/drm/i915/gvt/Makefile
index ea8324abc784..2c581e910688 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -5,5 +5,6 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o
trace_points.o firmware.o \
execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o
debugfs.o \
fb_decoder.o dmabuf.o page_track.o
-ccflags-y += -I $(srctree)/$(src) -I
$(srctree)/$(src)/$(GVT_DIR)/
-i915-y += $(addprefix $(GVT_DIR)/,
$(GVT_SOURCE))
+ccflags-y += -I $(srctree)/$(src) -I $(srctree)/$(src)/$(GVT_DIR)/
+ccflags-y += -DDYNAMIC_DEBUG_MODULE
+i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
--
2.26.2
[PATCH v2 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 a23b5d153153..04b851117eeb 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(>function, arg, "func");
} else if (!strcmp(keyword, "file")) {
--
2.26.2
[PATCH-v2 0/3] dynamic-debug fixups for 5.9
- fix new export name, with a wrapper for more utility. now with fixes per Joe Perches - 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 v2 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(>link, _tables);
mutex_unlock(_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 v2 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 stub-fns.
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.
--
v2- fixes per Joe Perches
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 20
lib/dynamic_debug.c | 26 --
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa9ff9e1c0b3..8aa0c7c2608c 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(_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)
+{
+ pr_warn("kernel not built with 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..a23b5d153153 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,29 @@ int ddebug_exec_queries(char *query, const char *modname)
return exitcode;
return nfound;
}
-EXPORT_SYMBOL_GPL(ddebug_exec_queries);
+
+/**
+ * dynamic_debug_exec_queries - select and change dynamic-debug prints
+ * @query: query-string described in admin-guide/dynamic-debug-howto
+ * @modname: string containing module name, usually _name
+ *
+ * This uses 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)
+{
+ int rc;
+ char *qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
+ if (!query)
+ return -ENOMEM;
+
+ rc = ddebug_exec_queries(qry, modname);
+ kfree(qry);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(dynamic_debug_exec_queries);
#define PREFIX_SIZE 64
--
2.26.2
