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
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 <[email protected]>
---
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 = &dt->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, &fbuf),
dp->format);
+
+ skipsite:
+ ddebug_site_put(dp);
}
}
mutex_unlock(&ddebug_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(struct _ddebug *dp, char *buf)
{
int pos_after_tid;
int pos = 0;
- const struct _ddebug_site *desc = dp->site;
+ const struct _ddebug_site *desc;
*buf = '\0';
@@ -653,6 +666,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 +684,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 +968,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 +985,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
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 &ddebug_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 <[email protected]>
---
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)
@@ -3333,6 +3334,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
info->debug = section_objs(info, "__dyndbg",
sizeof(*info->debug), &info->num_debug);
+ info->sites = section_objs(info, "__dyndbg_sites",
+ sizeof(*info->sites), &info->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
* this struct ddebug_table.
*/
- dt->mod_name = name;
- dt->num_ddebugs = n;
+ dt->mod_name = modname;
+ dt->num_ddebugs = numdbgs;
dt->ddebugs = tab;
+ dt->sites = sites;
mutex_lock(&ddebug_lock);
list_add(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
- v2pr_info("%3u debug prints in module %s\n", n, dt->mod_name);
+ v2pr_info("%3u debug prints in module %s\n", numdbgs, modname);
return 0;
}
@@ -1178,7 +1180,9 @@ static int __init dynamic_debug_init(void)
if (strcmp(modname, site->modname)) {
modct++;
- ret = ddebug_add_module(iter_mod_start, site_ct, modname);
+
+ ret = ddebug_add_module(iter_mod_start, site_mod_start,
+ site_ct, modname);
if (ret)
goto out_err;
site_ct = 0;
@@ -1188,7 +1192,7 @@ static int __init dynamic_debug_init(void)
}
site_ct++;
}
- ret = ddebug_add_module(iter_mod_start, site_ct, modname);
+ ret = ddebug_add_module(iter_mod_start, site_mod_start, site_ct, modname);
if (ret)
goto out_err;
--
2.29.2
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 <[email protected]>
---
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 @@
KEEP(*(.dtb.init.rodata)) \
__dtb_end = .;
+/* implement dynamic printk debug */
+#if defined(CONFIG_DYNAMIC_DEBUG) || \
+ (defined(CONFIG_DYNAMIC_DEBUG_CORE) \
+ && defined(DYNAMIC_DEBUG_MODULE))
+#define DYNAMIC_DEBUG_DATA() \
+ . = ALIGN(8); \
+ __start___dyndbg_sites = .; \
+ KEEP(*(__dyndbg_sites .gnu.linkonce.dyndbg_site)) \
+ __stop___dyndbg_sites = .; \
+ __start___dyndbg = .; \
+ KEEP(*(__dyndbg .gnu.linkonce.dyndbg)) \
+ __stop___dyndbg = .;
+#else
+#define DYNAMIC_DEBUG_DATA()
+#endif
+
/*
* .data section
*/
@@ -351,15 +367,8 @@
__end_once = .; \
STRUCT_ALIGN(); \
*(__tracepoints) \
- /* implement dynamic printk debug */ \
- . = ALIGN(8); \
- __start___dyndbg_sites = .; \
- KEEP(*(__dyndbg_sites)) \
- __stop___dyndbg_sites = .; \
- __start___dyndbg = .; \
- KEEP(*(__dyndbg)) \
- __stop___dyndbg = .; \
- LIKELY_PROFILE() \
+ DYNAMIC_DEBUG_DATA() \
+ LIKELY_PROFILE() \
BRANCH_PROFILE() \
TRACE_PRINTKS() \
BPF_RAW_TP() \
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 18689db0e2c0..229cfd81ffb3 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -101,10 +101,92 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
const struct ib_device *ibdev,
const char *fmt, ...);
-#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+/**
+ * DEFINE_DYNAMIC_DEBUG_TABLE(), DECLARE_DYNAMIC_DEBUG_TABLE()
+ *
+ * These are special versions of DEFINE_DYNAMIC_DEBUG_METADATA(). The
+ * job is to create/reserve a module-header struct-pair as the last
+ * element of the module's sub-vectors of __dyndbg & __dyndbg_sites,
+ * ie at a fixed offset from them. I expect to settle on 1 of these
+ * 2; DEFINE_ has seen most testing recently, and is favored.
+ *
+ * With this record reliably in-situ at a fixed offset for each
+ * callsite, we can use ._index to remember this offset, find the
+ * header, find the parallel vector, then index the corresponding site
+ * data.
+ *
+ * This macro is invoked at the bottom of this header. It is
+ * typically invoked multiple times for a module, generally at least
+ * once per object file. The combination of .gnu.linkonce._ and
+ * __weak appear to resolve earlier troubles with compile errors or
+ * multiple copies of headers.
+ */
+
+#define DECLARE_DYNAMIC_DEBUG_TABLE_(_sym_, _mod_) \
+ static struct _ddebug_site \
+ __section(".gnu.linkonce.dyndbg_site") \
+ __aligned(8) \
+ __maybe_unused \
+ _sym_##_dyndbg_site; \
+ static struct _ddebug \
+ __section(".gnu.linkonce.dyndbg") \
+ __aligned(8) \
+ __maybe_unused \
+ _sym_##_dyndbg_base
+
+#define DYNAMIC_DEBUG_TABLE_refer(_sym_) \
+ static void __used _sym_##_take_internal_refs(void) \
+{ \
+ struct _ddebug_site * dc = &_sym_##_dyndbg_site; \
+ struct _ddebug * dp = &_sym_##_dyndbg_base; \
+ printk(KERN_INFO "%s %d\n", dc->function, dp->lineno); \
+}
+
+#define DECLARE_DYNAMIC_DEBUG_TABLE() \
+ DECLARE_DYNAMIC_DEBUG_TABLE_(KBUILD_MODSYM, KBUILD_MODNAME); \
+ DYNAMIC_DEBUG_TABLE_refer(KBUILD_MODSYM)
+
+//#define DEFN_SC static // clashes with extern forward decl
+//#define DEFN_SC extern // warning: ‘KBUILD_MODNAME_dyndbg_site’ initialized and declared ‘extern’
+//#define DEFN_SC // no section allowd on locals
+#define DEFN_SC __weak
+
+#define DEFINE_DYNAMIC_DEBUG_TABLE_(_sym_,_mod_) \
+ DEFN_SC struct _ddebug_site \
+ __section(".gnu.linkonce.dyndbg_site") \
+ __used __aligned(8) \
+ _sym_ ##_dyndbg_site = { \
+ .modname = _mod_, \
+ .filename = __FILE__, \
+ .function = (void*) _mod_ \
+ }; \
+ DEFN_SC struct _ddebug \
+ __section(".gnu.linkonce.dyndbg") \
+ __used __aligned(8) \
+ _sym_ ##_dyndbg_base = { \
+ .site = & _sym_ ##_dyndbg_site, \
+ .format = _mod_, \
+ .lineno = 0 \
+ }
+
+/* above init conditions as distinguishing predicate.
+ * (site == iter->site) should work but doesnt, possibly cuz MODSYM
+ * expansion problem
+ */
+#define is_dyndbg_header_pair(iter, site) \
+ ((iter->format == site->modname) \
+ && (site->modname == site->function))
+
+// build-time expensive, shows repetitive includes
+// #pragma message "<" __stringify(KBUILD_MODSYM) "> adding DYNDBG_TABLE"
+
+#define DEFINE_DYNAMIC_DEBUG_TABLE() \
+ DEFINE_DYNAMIC_DEBUG_TABLE_(KBUILD_MODSYM, KBUILD_MODNAME);
+
+#define DEFINE_DYNAMIC_DEBUG_METADATA_(_mod_, name, fmt) \
static struct _ddebug_site __aligned(8) \
__section("__dyndbg_sites") name##_site = { \
- .modname = KBUILD_MODNAME, \
+ .modname = _mod_, \
.filename = __FILE__, \
.function = __func__, \
}; \
@@ -114,9 +196,13 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
.format = (fmt), \
.lineno = __LINE__, \
.flags = _DPRINTK_FLAGS_DEFAULT, \
+ ._index = 0, \
_DPRINTK_KEY_INIT \
}
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+ DEFINE_DYNAMIC_DEBUG_METADATA_(KBUILD_MODNAME, name, fmt)
+
#ifdef CONFIG_JUMP_LABEL
#ifdef DEBUG
@@ -247,4 +333,14 @@ static inline int dynamic_debug_exec_queries(const char *query, const char *modn
#endif /* !CONFIG_DYNAMIC_DEBUG_CORE */
+#if ((defined(CONFIG_DYNAMIC_DEBUG) || \
+ (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))) \
+ && defined(KBUILD_MODNAME) \
+ && !defined(NO_DYNAMIC_DEBUG_TABLE))
+
+/* transparently invoked, except for -DNO_DYNAMIC_DEBUG_TABLE */
+DEFINE_DYNAMIC_DEBUG_TABLE()
+
+#endif
+
#endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 3b53035d63d6..c1a7345277eb 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1203,8 +1203,7 @@ static int __init dynamic_debug_init(void)
const char *modname = NULL;
char *cmdline;
int ret = 0;
- int site_ct = 0, entries = 0, modct = 0;
- int mod_index = 0;
+ int i, site_ct = 0, modct = 0, mod_index = 0;
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1220,10 +1219,20 @@ static int __init dynamic_debug_init(void)
site = site_mod_start = __start___dyndbg_sites;
modname = site->modname;
- for (; iter < __stop___dyndbg; iter++, site++) {
+ for (i = 0; iter < __stop___dyndbg; iter++, site++, i++) {
+ /* we should have 1 header for kernel, at beginning of blocks
+ * but lets be safe for a while
+ */
+ if (is_dyndbg_header_pair(iter, site)) {
+ v3pr_info("header.%d: %s\n", i, site->modname);
+ iter->site = site; /* fixup bad init */
+ continue;
+ }
BUG_ON(site != iter->site);
- entries++;
+
+ v3pr_info("site: %s.%s.%d\n", site->modname,
+ site->function, iter->lineno);
if (strcmp(modname, site->modname)) {
modct++;
@@ -1232,13 +1241,14 @@ static int __init dynamic_debug_init(void)
site_ct, mod_index, modname);
if (ret)
goto out_err;
- site_ct = 0;
+
modname = site->modname;
iter_mod_start = iter;
site_mod_start = site;
+ mod_index += site_ct;
+ site_ct = 0;
}
site_ct++;
- iter->_index = index++;
}
ret = __ddebug_add_module(iter_mod_start, site_mod_start, site_ct, mod_index, modname);
if (ret)
@@ -1246,9 +1256,9 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
vpr_info("%d modules, %d entries and %d bytes in ddebug tables, %d bytes in __dyndbg section, %d bytes in __dyndbg_sites section\n",
- modct, entries, (int)(modct * sizeof(struct ddebug_table)),
- (int)(entries * sizeof(struct _ddebug)),
- (int)(entries * sizeof(struct _ddebug_site)));
+ modct, i, (int)(modct * sizeof(struct ddebug_table)),
+ (int)(i * sizeof(struct _ddebug)),
+ (int)(i * sizeof(struct _ddebug_site)));
/* apply ddebug_query boot param, dont unload tables on err */
if (ddebug_setup_string[0] != '\0') {
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 8cd67b1b6d15..051993ef9b92 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -111,10 +111,12 @@ target-stem = $(basename $(patsubst $(obj)/%,%,$@))
# These flags are needed for modversions and compiling, so we define them here
# $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
# end up in (or would, if it gets compiled in)
+
name-fix-token = $(subst $(comma),_,$(subst -,_,$1))
name-fix = $(call stringify,$(call name-fix-token,$1))
basename_flags = -DKBUILD_BASENAME=$(call name-fix,$(basetarget))
modname_flags = -DKBUILD_MODNAME=$(call name-fix,$(modname)) \
+ -DKBUILD_MODSYM=$(call name-fix-token,$(modname)) \
-D__KBUILD_MODNAME=kmod_$(call name-fix-token,$(modname))
modfile_flags = -DKBUILD_MODFILE=$(call stringify,$(modfile))
--
2.29.2
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 <[email protected]>
---
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
2 prints use site->member, protect them with if site.
no functional changes.
Signed-off-by: Jim Cromie <[email protected]>
---
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
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 <[email protected]>
---
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, &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, &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, &flags));
+ seq_escape(m, dp->format, "\t\r\n\"");
+ seq_puts(m, "\"\n");
+ }
return 0;
}
--
2.29.2
Move the JUMP_LABEL/static-key code to a separate function.
no functional changes.
Signed-off-by: Jim Cromie <[email protected]>
---
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(&dp->key.dd_key_true);
+ } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
+ static_branch_enable(&dp->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(&dp->key.dd_key_true);
- } else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
- static_branch_enable(&dp->key.dd_key_true);
-#endif
+
+ ddebug_alter_site(dp, modifiers);
+
dp->flags = newflags;
if (dc)
--
2.29.2
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 <[email protected]>
---
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;
const char *filename;
+ const char *function;
+} __aligned(8);
+
+struct _ddebug {
+ struct _ddebug_site *site;
+ /* format is always needed, lineno shares word with flags */
const char *format;
- unsigned int lineno:18;
+ const unsigned lineno:18;
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -44,8 +52,7 @@ struct _ddebug {
struct static_key_false dd_key_false;
} key;
#endif
-} __attribute__((aligned(8)));
-
+} __aligned(8);
#if defined(CONFIG_DYNAMIC_DEBUG_CORE)
@@ -83,11 +90,15 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
const char *fmt, ...);
#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
- static struct _ddebug __aligned(8) \
- __section("__dyndbg") name = { \
+ static struct _ddebug_site __aligned(8) \
+ __section("__dyndbg_sites") name##_site = { \
.modname = KBUILD_MODNAME, \
- .function = __func__, \
.filename = __FILE__, \
+ .function = __func__, \
+ }; \
+ static struct _ddebug __aligned(8) \
+ __section("__dyndbg") name = { \
+ .site = &name##_site, \
.format = (fmt), \
.lineno = __LINE__, \
.flags = _DPRINTK_FLAGS_DEFAULT, \
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c70d6347afa2..738c4ce28046 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -142,7 +142,7 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
- * callsites, normally the same as number of changes. If verbose,
+ * sites, normally the same as number of changes. If verbose,
* logs the changes. Takes ddebug_lock.
*/
static int ddebug_change(const struct ddebug_query *query,
@@ -165,19 +165,20 @@ static int ddebug_change(const struct ddebug_query *query,
for (i = 0; i < dt->num_ddebugs; i++) {
struct _ddebug *dp = &dt->ddebugs[i];
+ struct _ddebug_site *dc = dp->site;
/* match against the source filename */
if (query->filename &&
- !match_wildcard(query->filename, dp->filename) &&
+ !match_wildcard(query->filename, dc->filename) &&
!match_wildcard(query->filename,
- kbasename(dp->filename)) &&
+ kbasename(dc->filename)) &&
!match_wildcard(query->filename,
- trim_prefix(dp->filename)))
+ trim_prefix(dc->filename)))
continue;
/* match against the function */
if (query->function &&
- !match_wildcard(query->function, dp->function))
+ !match_wildcard(query->function, dc->function))
continue;
/* match against the format */
@@ -214,8 +215,8 @@ static int ddebug_change(const struct ddebug_query *query,
#endif
dp->flags = newflags;
v2pr_info("changed %s:%d [%s]%s =%s\n",
- trim_prefix(dp->filename), dp->lineno,
- dt->mod_name, dp->function,
+ trim_prefix(dc->filename), dp->lineno,
+ dt->mod_name, dc->function,
ddebug_describe_flags(dp->flags, &fbuf));
}
}
@@ -586,14 +587,15 @@ static int remaining(int wrote)
return 0;
}
-static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
+static char *dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
{
int pos_after_tid;
int pos = 0;
+ const struct _ddebug_site *desc = dp->site;
*buf = '\0';
- if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
+ if (dp->flags & _DPRINTK_FLAGS_INCL_TID) {
if (in_interrupt())
pos += snprintf(buf + pos, remaining(pos), "<intr> ");
else
@@ -601,15 +603,15 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
task_pid_vnr(current));
}
pos_after_tid = pos;
- if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+ if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
desc->modname);
- if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+ if (dp->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
desc->function);
- if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
+ if (dp->flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
- desc->lineno);
+ dp->lineno);
if (pos - pos_after_tid)
pos += snprintf(buf + pos, remaining(pos), " ");
if (pos >= PREFIX_SIZE)
@@ -879,6 +881,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
{
struct ddebug_iter *iter = m->private;
struct _ddebug *dp = p;
+ struct _ddebug_site *dc;
struct flagsbuf flags;
if (p == SEQ_START_TOKEN) {
@@ -887,9 +890,11 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
return 0;
}
+ dc = dp->site;
+
seq_printf(m, "%s:%u [%s]%s =%s \"",
- trim_prefix(dp->filename), dp->lineno,
- iter->table->mod_name, dp->function,
+ trim_prefix(dc->filename), dp->lineno,
+ iter->table->mod_name, dc->function,
ddebug_describe_flags(dp->flags, &flags));
seq_escape(m, dp->format, "\t\r\n\"");
seq_puts(m, "\"\n");
@@ -1093,17 +1098,17 @@ static int __init dynamic_debug_init(void)
return 0;
}
iter = __start___dyndbg;
- modname = iter->modname;
+ modname = iter->site->modname;
iter_start = iter;
for (; iter < __stop___dyndbg; iter++) {
entries++;
- if (strcmp(modname, iter->modname)) {
+ if (strcmp(modname, iter->site->modname)) {
modct++;
ret = ddebug_add_module(iter_start, n, modname);
if (ret)
goto out_err;
n = 0;
- modname = iter->modname;
+ modname = iter->site->modname;
iter_start = iter;
}
n++;
@@ -1113,9 +1118,10 @@ static int __init dynamic_debug_init(void)
goto out_err;
ddebug_init_success = 1;
- vpr_info("%d modules, %d entries and %d bytes in ddebug tables, %d bytes in __dyndbg section\n",
+ vpr_info("%d modules, %d entries and %d bytes in ddebug tables, %d bytes in __dyndbg section, %d bytes in __dyndbg_sites section\n",
modct, entries, (int)(modct * sizeof(struct ddebug_table)),
- (int)(entries * sizeof(struct _ddebug)));
+ (int)(entries * sizeof(struct _ddebug)),
+ (int)(entries * sizeof(struct _ddebug_site)));
/* apply ddebug_query boot param, dont unload tables on err */
if (ddebug_setup_string[0] != '\0') {
--
2.29.2
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 <[email protected]>
---
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 = &dt->ddebugs[i];
- struct _ddebug_site *dc = dp->site;
+ struct _ddebug_site *dc;
if (!ddebug_match_site(query, dp))
continue;
--
2.29.2
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 <[email protected]>
---
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
Jim Cromie <[email protected]> writes:
> 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:
So how much memory does it actually save?
-Andi
On Thu, Mar 18, 2021 at 10:12 PM Andi Kleen <[email protected]> wrote:
>
> Jim Cromie <[email protected]> writes:
>
> > 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:
>
> So how much memory does it actually save?
> -Andi
sorry for late reply, html mode got switched on and I didnt see kickback
on my laptop/build, master has 165kb, about 70k is the compressible data,
RLE column-wize could get close to 60% on my data. so ~40kb ?
3 things to do to get the savings:
figure the compression,
figure the hash holding enabled/used/expanded pr_debug decorations
(maybe optional, depending on indexed/seek decompress time)
drop the site pointer, with some anonymous union struct combo to blend
header with callsites cleanly