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<x> lookup.
RFC is mostly here.
rev1: https://lore.kernel.org/lkml/[email protected]/
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
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 <[email protected]>
---
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, &fbuf),
dp->format);
+
+ skipsite:
+ ddebug_site_put(dp);
}
}
mutex_unlock(&ddebug_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
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 <[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 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 = &dt->ddebugs[i];
- struct _ddebug_callsite *dc = dp->site;
+ struct _ddebug_callsite *dc;
if (!ddebug_match_site(query, dp))
continue;
--
2.29.2
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 <[email protected]>
---
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 = &dt->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
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 <[email protected]>
---
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.
+ * 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;
+};
+
+struct _ddebug {
+ struct _ddebug_callsite *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_callsite __aligned(8) \
+ __section("__dyndbg_callsites") 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..fb8e0b288f89 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -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_callsite *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_callsite *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_callsite *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_callsites 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_callsite)));
/* apply ddebug_query boot param, dont unload tables on err */
if (ddebug_setup_string[0] != '\0') {
--
2.29.2
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 <[email protected]>
---
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 = &dt->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
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 <[email protected]>
---
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(&dp->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, &fbuf));
+
+ 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, &fbuf));
+ else
+ v2pr_info("changed %s:%d =%s \"%s\"\n",
+ dt->mod_name, dp->lineno,
+ ddebug_describe_flags(dp->flags, &fbuf),
+ dp->format);
}
}
mutex_unlock(&ddebug_lock);
--
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 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
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 <[email protected]>
---
lib/dynamic_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 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
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.
rec ptr mod-ptr N (void*)p
[ 1.929072] dyndbg: 2828: ffffffff82b32f28 ffffffff82b32f10 1 24 40
[ 1.929326] dyndbg: 2829: ffffffff82b32f40 ffffffff82b32f10 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 <[email protected]>
---
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
Wrap function in a static-inline one, which checks flags to avoid
calling the function unnecessarily.
Signed-off-by: Jim Cromie <[email protected]>
---
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
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="0000", 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) =_ "0000"
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. Value Sym. Name + Addend
000000000010 014c00000001 R_X86_64_64 0000000000000000 i915_site + 0
000000000018 001a00000001 R_X86_64_64 0000000000000000 .rodata.str1.1 + 548
Relocation section '.rela__dyndbg_callsites' at offset 0x6f90 contains 2 entries:
Offset Info Type Sym. Value Sym. Name + Addend
000000000000 001a00000001 R_X86_64_64 0000000000000000 .rodata.str1.1 + d8
000000000008 000a00000001 R_X86_64_64 0000000000000000 .rodata.str1.8 + 110
Signed-off-by: Jim Cromie <[email protected]>
---
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 &module##_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 = &module##_site, \
+ .format = "0000", \
+ .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__, \
.flags = _DPRINTK_FLAGS_DEFAULT, \
- _DPRINTK_KEY_INIT \
+ _DPRINTK_KEY_INIT, \
+ .module_index = 0 /* want offset from _base */ \
}
-/** DEFINE_DYNAMIC_DEBUG_TABLE(module)
-
- * It's just a "callsite" we expect not to use, except as a 0th
- * element we can use to initialize each ddebug.module_index.
- *
- * Later, this will be adapted to initialize a pair of alternate
- * structures; analogous to structs _ddebug & _ddebug_callsite, and
- * implemented as structs unionized with them. This pair of
- * alt-structs could supplant most or all of the function and purpose
- * of the linked list of ddebug_tables built by ddebug_add_module.
- */
-#define DEFINE_DYNAMIC_DEBUG_TABLE(module) \
- DEFINE_DYNAMIC_DEBUG_METADATA(module, "00-1st-in-subsection")
-
#ifdef CONFIG_JUMP_LABEL
#ifdef DEBUG
--
2.29.2
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 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, &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 inline 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 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(&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
@@ -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(&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
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 <[email protected]>
---
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), &info->num_debug);
+ info->sites = section_objs(info, "__dyndbg_callsites",
+ sizeof(*info->sites), &info->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(&ddebug_lock);
list_add(&dt->link, &ddebug_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,
+ n, modname);
if (ret)
goto out_err;
n = 0;
@@ -1192,7 +1196,7 @@ static int __init dynamic_debug_init(void)
}
n++;
}
- ret = ddebug_add_module(iter_mod_start, n, modname);
+ ret = ddebug_add_module(iter_mod_start, site_mod_start, n, modname);
if (ret)
goto out_err;
--
2.29.2
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 <[email protected]>
---
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(&dp->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
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; /* 0 16 */
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 ordering. I hope not.
Signed-off-by: Jim Cromie <[email protected]>
---
include/linux/dynamic_debug.h | 16 ++++++++++++++++
lib/dynamic_debug.c | 4 ++++
2 files changed, 20 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index e7b5e7664e51..0f4e703c97ee 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -63,6 +63,8 @@ struct _ddebug {
#define _DPRINTK_FLAGS_DEFAULT 0
#endif
unsigned int flags:8;
+
+ unsigned module_index; /* offset from 1st in this module */
} __aligned(8);
@@ -116,6 +118,20 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
_DPRINTK_KEY_INIT \
}
+/** DEFINE_DYNAMIC_DEBUG_TABLE(module)
+
+ * It's just a "callsite" we expect not to use, except as a 0th
+ * element we can use to initialize each ddebug.module_index.
+ *
+ * Later, this will be adapted to initialize a pair of alternate
+ * structures; analogous to structs _ddebug & _ddebug_callsite, and
+ * implemented as structs unionized with them. This pair of
+ * alt-structs could supplant most or all of the function and purpose
+ * of the linked list of ddebug_tables built by ddebug_add_module.
+ */
+#define DEFINE_DYNAMIC_DEBUG_TABLE(module) \
+ DEFINE_DYNAMIC_DEBUG_METADATA(module, "00-1st-in-subsection")
+
#ifdef CONFIG_JUMP_LABEL
#ifdef DEBUG
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ec28c113a361..90bea4909720 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1036,6 +1036,7 @@ int ddebug_add_module(struct _ddebug *tab, struct _ddebug_callsite *sites,
unsigned int n, const char *name)
{
struct ddebug_table *dt;
+ int i;
dt = kzalloc(sizeof(*dt), GFP_KERNEL);
if (dt == NULL) {
@@ -1053,6 +1054,9 @@ int ddebug_add_module(struct _ddebug *tab, struct _ddebug_callsite *sites,
dt->ddebugs = tab;
dt->sites = sites;
+ for (i = 0; i < n; i++)
+ tab[i].module_index = i;
+
mutex_lock(&ddebug_lock);
list_add(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);
--
2.29.2
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?
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: e83b4a5011dc4c54048fa264da5923d3253df8fd ("[RFC PATCH v2 02/19] dyndbg: split struct _ddebug, move display fields to new _ddebug_callsite")
url: https://github.com/0day-ci/linux/commits/Jim-Cromie/dynamic-debug-diet-plan/20201226-042542
base: https://git.kernel.org/cgit/linux/kernel/git/arnd/asm-generic.git master
in testcase: leaking-addresses
version: leaking-addresses-x86_64-4f19048-1_20201111
with following parameters:
ucode: 0xde
on test machine: 4 threads Intel(R) Core(TM) i7-7567U CPU @ 3.50GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
2020-12-30 15:54:07 ./leaking_addresses.pl --output-raw result/scan.out
2020-12-30 15:54:27 ./leaking_addresses.pl --input-raw result/scan.out --squash-by-filename
Total number of results from scan (incl dmesg): 160105
dmesg output:
[ 0.058423] mapped IOAPIC to ffffffffff5fb000 (fec00000)
Results squashed by filename (excl dmesg). Displaying [<number of results> <filename>], <example result>
[5 .altinstr_aux] 0xffffffffc0131a49
[50 .symtab] 0xffffffffc00c2000
[2 .noinstr.text] 0xffffffffc0762a40
[41 .data] 0xffffffffc009f000
[158484 kallsyms] ffffffff81000000 T startup_64
[22 .smp_locks] 0xffffffffc009d094
[1 .rodata.cst16.mask2] 0xffffffffc00de0e0
[1 key] 1000000000007 ff980000000007ff febeffdfffefffff fffffffffffffffe
[49 __mcount_loc] 0xffffffffc009d03c
[46 .orc_unwind_ip] 0xffffffffc009d3a0
[24 __ksymtab_strings] 0xffffffffc0118048
[7 .fixup] 0xffffffffc00d62ea
[1 .rodata.cst16.mask1] 0xffffffffc00de0d0
[6 __tracepoints] 0xffffffffc02efcc0
[6 _ftrace_events] 0xffffffffc02efb80
[1 .data..cacheline_aligned] 0xffffffffc064ac80
[31 .bss] 0xffffffffc009f500
[38 .rodata.str1.8] 0xffffffffc009d170
[35 .text.unlikely] 0xffffffffc009cbaf
[2 .rodata.cst16.bswap_mask] 0xffffffffc0098070
[37 .exit.text] 0xffffffffc009cc70
[14 .parainstructions] 0xffffffffc02b4d88
[50 .strtab] 0xffffffffc00c2bb8
[50 .note.Linux] 0xffffffffc009d024
[18 __dyndbg] 0xffffffffc009f0c8
[50 .text] 0xffffffffc009c000
[6 __tracepoints_strings] 0xffffffffc02eb7d0
[50 .note.gnu.build-id] 0xffffffffc009d000
[339 blacklist] 0xffffffff81c00860-0xffffffff81c00880 asm_exc_divide_error
[11 __ex_table] 0xffffffffc00d1128
[1 _ftrace_eval_map] 0xffffffffc0985148
[14 __ksymtab_gpl] 0xffffffffc02b403c
[9 .init.rodata] 0xffffffffc00c1000
[1 _error_injection_whitelist] 0xffffffffc098ab70
[6 .ref.data] 0xffffffffc02efba0
[10 .data..read_mostly] 0xffffffffc009f108
[42 .rodata.str1.1] 0xffffffffc009d09c
[1 ___srcu_struct_ptrs] 0xffffffffc027a000
[140 printk_formats] 0xffffffff8234e927 : "CPU_ON"
[1 devices] B: KEY=1000000000007 ff980000000007ff febeffdfffefffff fffffffffffffffe
[1 uevent] KEY=1000000000007 ff980000000007ff febeffdfffefffff fffffffffffffffe
[18 __ksymtab] 0xffffffffc011803c
[6 __bpf_raw_tp_map] 0xffffffffc02efb20
[50 .gnu.linkonce.this_module] 0xffffffffc009f140
[50 modules] netconsole 20480 0 - Live 0xffffffffc00c0000
[1 .rodata.cst32.byteshift_table] 0xffffffffc00de100
[14 .altinstr_replacement] 0xffffffffc03d39ca
[25 __jump_table] 0xffffffffc009e000
[7 .static_call_sites] 0xffffffffc0987088
[46 .orc_unwind] 0xffffffffc009d544
[42 .rodata] 0xffffffffc009d2c0
[6 __tracepoints_ptrs] 0xffffffffc02eb7bc
[4 .init.data] 0xffffffffc0069000
[14 .altinstructions] 0xffffffffc03e5846
[10 .data.once] 0xffffffffc03e65b4
[6 .static_call.text] 0xffffffffc02e2b24
[18 __dyndbg_callsites] 0xffffffffc009f0f0
[40 .init.text] 0xffffffffc00c0000
[19 __param] 0xffffffffc009d378
[25 __bug_table] 0xffffffffc01ee070
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml
Thanks,
Oliver Sang
On Tue, Dec 29, 2020 at 11:56 AM Joe Perches <[email protected]> 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/[email protected]/
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
0000000 \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 recognized
objcopy: vmlinux: can't dump section '__dyndbg_callsites' - it does
not exist: file format not recognized
[jimc@frodo local-i915m]$ ll dd*
-rw-rw-r--. 1 jimc jimc 114160 Dec 31 11:36 dd
-rw-rw-r--. 1 jimc jimc 68496 Dec 31 11:36 dd_callsites
[jimc@frodo local-i915m]$ od -d dd | head -n 40
0000000 1 0 0 0 1 0 0 0
0000020 0 0 0 0 0 0 0 0
0000040 1349 4 0 0 1 0 0 0
0000060 1 0 0 0 0 0 0 0
0000100 0 0 0 0 1347 4 0 0
0000120 1 0 0 0 1 0 0 0
0000140 0 0 0 0 0 0 0 0
0000160 1346 4 0 0 1 0 0 0
0000200 1 0 0 0 0 0 0 0
0000220 0 0 0 0 1344 4 0 0
0000240 1 0 0 0 1 0 0 0
0000260 0 0 0 0 0 0 0 0
0000300 1133 4 0 0 1 0 0 0
0000320 1 0 0 0 0 0 0 0
0000340 0 0 0 0 1094 4 0 0
0000360 0 0 0 0 0 0 0 0
*
0000420 485 0 0 0 0 0 0 0
0000440 0 0 0 0 0 0 0 0
0000460 0 0 0 0 926 0 0 0
0000500 0 0 0 0 0 0 0 0
*
0000540 897 0 0 0 0 0 0 0
0000560 0 0 0 0 0 0 0 0
0000600 0 0 0 0 890 0 0 0
0000620 0 0 0 0 0 0 0 0
*
0000660 782 0 0 0 0 0 0 0
0000700 0 0 0 0 0 0 0 0
0000720 0 0 0 0 779 0 0 0
0000740 0 0 0 0 0 0 0 0
*
0001000 9691 0 0 0 0 0 0 0
0001020 0 0 0 0 0 0 0 0
0001040 0 0 0 0 2109 0 0 0
0001060 0 0 0 0 0 0 0 0
*
0001120 273 0 0 0 0 0 0 0
0001140 0 0 0 0 0 0 0 0
0001160 0 0 0 0 457 0 0 0
Theres obviously some pattern to it, but I dont see a path towards
compressing and packaging it to load like a DT-file
And sorry for late reply, Ive continued cooking, thinking it would
help clarify my responses here. I'll send v3 at some point
thanks
JIm