[PATCH v6 41/57] dyndbg: split repeating columns to new struct _ddebug_site
From: Jim Cromie
Date: Sun Sep 04 2022 - 17:45:48 EST
Struct _ddebug has 3 RO-data fields: _module, _file, _function, which
have significant repetition in the builtins: 4222 unique records /
8920 callsites on a recent laptop build. Thats just 47% unique, on
24/56 of the unitary record.
The quick path to excising this redundancy is:
1- split the table in 2, link 1st to 2nd (done here)
2- de-duplicate the 2nd table. (soon)
So split struct _ddebug, move the 3 fields to a new struct
_ddebug_site, and add a pointer from _ddebug to _debug_sites. The
lineno field stays in _ddebug, so all _sites in a fn are identical.
The new ptr from _ddebug -> _ddebug_site increases memory footprint,
until step 2 is completed, at which point:
old = 56 * 8920
new = (56-24+8) * 8920 + 24 * 4222
IE:
DB<2> p 56 * 8920
499520
DB<3> p 40*8920 + 24 * 4222
458128
Thats 41392 saved, or ~8.3%
Further, the site pointer is just temporary scaffolding:
- descriptors are in a vector.
- new desc._idx field (from spare bits) can get us to 0.
set during _init, not by linker
- add a header record in front of vector (.gnu.linkonce.dyndbg*)
point it up to struct dyndbg_info
- dyndbg_info has .sites
- same desc._idx gets us sites[._idx]
- new desc._map field, gives sites[._map]
this allows de-duplication and packing.
Once that is done, the savings increases:
DB<7> p (56 * 8920) - (((56-24) *8920) + 24*4222)
112752 saved, or 22%
STEP 1:
dynamic_debug.h:
This cuts struct _ddebug in half, renames the top-half to
_ddebug_site, keeps __align(8) for both halves. Adds a forward decl
for a unified comment for both halves, and added _ddebug.site field to
point at a site record.
Rework DEFINE_DYNAMIC_DEBUG_METADATA_CLS macro to declare & initialize
the 2 static/private struct vars together, and link them together. It
places each struct into its own section, so the linker packs 2
parallel arrays, and links them like a ladder.
struct _ddebug_info is extended to track _ddebug_site[] just like it
does for _ddebug[] and _ddebug_classes[].
The accessor macros desc_{module,filename,function} follow the
field-moves with added '->site->' references, and return "_nope_" or
"_na_" if the desc or site are null. This makes those ptrs nullable,
and their referents recoverable (nothing tries to use this yet).
NB: the "_na_" is undone temporarily later, for dev shortcut.
Also add const to lineno field. It is set by compiler.
In struct ddebug_table, add struct _ddebug_site *sites, to treat new
vector just like the module's _ddebug[]s (its __dyndbg section, for
loadable mods). While we don't need it now, we will need it to
de-scaffold (drop the _ddebug.site).
dynamic_debug.c:
extern declarations of the section start/end symbols named and
initialized in vmlinux.lds.h
dynamic_debug_init():
Initialize global builtin_state from initialized cursor var. Trying
to do so statically gave:
"error: initializer element is not computable at load time"
Check (num-descs == num-sites), and quit early otherwise. This is an
important precondition, w/o it, we cannot really continue confidently.
I inadvertently created the situation by having __used on 1/2 of the
_ddebug{,_site} pair created by DECLARE_DYNAMIC_DEBUG_METADATA; this
created ~70/ extra site records. This "worked", but was unnerving
until I tracked it down.
Add site iterator & site_mod_start marker, recapping iter/_mod_start.
Inside the main loop, validate (site == iter->site). This is the
full/proper precondition for the expected section contents and
inter-linkage; the (num-descs == num-sites) check is just a quick
necessary-but-not-sufficient version of this.
NOTE: this check could be a BUG_ON, esp as any mismatch should have
been caught by the quick-check. ATM it is just a pr_err; Id prefer to
see errors rather than crashes.
Demotes iter->site by replacing iter->site->_module by site->_module.
This is a small step towards dropping it entirely.
vmlinux.lds.h:
add __dyndbg_sites section and start/end symbols, with the same
align(8) and KEEP as used in the __dyndbg section.
kernel/module/main.c:load_info():
Initialize _ddebug_info.sites with new section address.
Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
---
include/asm-generic/vmlinux.lds.h | 3 +++
include/linux/dynamic_debug.h | 37 +++++++++++++++++++++---------
kernel/module/main.c | 2 ++
lib/dynamic_debug.c | 38 +++++++++++++++++++++++--------
4 files changed, 60 insertions(+), 20 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 9b8bd5504ad9..1e7ee65e8591 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -351,6 +351,9 @@
__start___dyndbg = .; \
KEEP(*(__dyndbg)) \
__stop___dyndbg = .; \
+ __start___dyndbg_sites = .; \
+ KEEP(*(__dyndbg_sites)) \
+ __stop___dyndbg_sites = .; \
LIKELY_PROFILE() \
BRANCH_PROFILE() \
TRACE_PRINTKS() \
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index e04f5b0a31e2..dcc0e8cc2ef0 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -9,20 +9,28 @@
#include <linux/build_bug.h>
/*
- * 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 _ddebug.flags
+ * - select callsites for modification via >control
+ * - display callsites & settings in `cat control`
*/
const char *_modname;
const char *_function;
const char *_filename;
+} __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;
#define CLS_BITS 6
unsigned int class_id:CLS_BITS;
#define _DPRINTK_CLASS_DFLT ((1 << CLS_BITS) - 1)
@@ -58,7 +66,7 @@ struct _ddebug {
struct static_key_false dd_key_false;
} key;
#endif
-} __attribute__((aligned(8)));
+} __aligned(8);
enum class_map_type {
DD_CLASS_TYPE_DISJOINT_BITS,
@@ -118,8 +126,10 @@ struct ddebug_class_map {
/* encapsulate linker provided built-in (or module) dyndbg data */
struct _ddebug_info {
struct _ddebug *descs;
+ struct _ddebug_site *sites;
struct ddebug_class_map *classes;
unsigned int num_descs;
+ unsigned int num_sites;
unsigned int num_classes;
};
@@ -137,6 +147,7 @@ struct ddebug_class_param {
int ddebug_add_module(struct _ddebug_info *dyndbg, 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, ...);
@@ -164,11 +175,15 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
const char *fmt, ...);
#define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, 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/kernel/module/main.c b/kernel/module/main.c
index 6aa6153aa6e0..5a80f18f8e4a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2113,6 +2113,8 @@ static int find_module_sections(struct module *mod, struct load_info *info)
info->dyndbg.descs = section_objs(info, "__dyndbg",
sizeof(*info->dyndbg.descs), &info->dyndbg.num_descs);
+ info->dyndbg.sites = section_objs(info, "__dyndbg_sites",
+ sizeof(*info->dyndbg.sites), &info->dyndbg.num_sites);
info->dyndbg.classes = section_objs(info, "__dyndbg_classes",
sizeof(*info->dyndbg.classes), &info->dyndbg.num_classes);
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 5a22708679a7..f1f354efed5a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -44,6 +44,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[];
extern struct ddebug_class_map __start___dyndbg_classes[];
extern struct ddebug_class_map __stop___dyndbg_classes[];
@@ -52,6 +54,7 @@ struct ddebug_table {
const char *mod_name;
unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct _ddebug_site *sites;
};
struct ddebug_query {
@@ -1487,20 +1490,27 @@ static int __init dynamic_debug_init_control(void)
return 0;
}
+fs_initcall(dynamic_debug_init_control);
+
+static struct _ddebug_info builtin_state;
static int __init dynamic_debug_init(void)
{
struct _ddebug *iter, *iter_mod_start;
+ struct _ddebug_site *site, *site_mod_start;
int ret, i, mod_sites, mod_ct;
const char *modname;
char *cmdline;
struct _ddebug_info di = {
.descs = __start___dyndbg,
+ .sites = __start___dyndbg_sites,
.classes = __start___dyndbg_classes,
- .num_descs = __stop___dyndbg - __start___dyndbg,
- .num_classes = __stop___dyndbg_classes - __start___dyndbg_classes,
+ .num_descs = __stop___dyndbg - __start___dyndbg,
+ .num_sites = __stop___dyndbg_sites - __start___dyndbg_sites,
+ .num_classes = __stop___dyndbg_classes - __start___dyndbg_classes,
};
+ builtin_state = di;
if (&__start___dyndbg == &__stop___dyndbg) {
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1511,28 +1521,40 @@ static int __init dynamic_debug_init(void)
ddebug_init_success = 1;
return 0;
}
-
+ if (di.num_descs != di.num_sites) {
+ /* cant happen, unless site section has __used, desc does not */
+ pr_err("unequal vectors: descs/sites %d/%d\n", di.num_descs, di.num_sites);
+ return 1;
+ }
iter = iter_mod_start = __start___dyndbg;
- modname = iter->_modname;
+ site = site_mod_start = __start___dyndbg_sites;
+ modname = iter->site->_modname;
i = mod_sites = mod_ct = 0;
- for (; iter < __stop___dyndbg; iter++, i++, mod_sites++) {
+ for (; iter < __stop___dyndbg; iter++, site++, i++, mod_sites++) {
+
+ if (site != iter->site)
+ /* XXX: also cant happen, but lets see how it plays */
+ pr_err("linkage problem: site != iter->site\n");
- if (strcmp(modname, iter->_modname)) {
+ if (strcmp(modname, site->_modname)) {
mod_ct++;
di.num_descs = mod_sites;
di.descs = iter_mod_start;
+ di.sites = site_mod_start;
ret = __ddebug_add_module(&di, i - mod_sites, modname);
if (ret)
goto out_err;
mod_sites = 0;
- modname = iter->_modname;
+ modname = site->_modname;
iter_mod_start = iter;
+ site_mod_start = site;
}
}
di.num_descs = mod_sites;
di.descs = iter_mod_start;
+ di.sites = site_mod_start;
ret = __ddebug_add_module(&di, i - mod_sites, modname);
if (ret)
goto out_err;
@@ -1566,5 +1588,3 @@ static int __init dynamic_debug_init(void)
/* Allow early initialization for boot messages via boot param */
early_initcall(dynamic_debug_init);
-/* Debugfs setup must be done later */
-fs_initcall(dynamic_debug_init_control);
--
2.37.2