[RFC PATCH v5 00/28] dynamic debug diet plan

[RFC PATCH v5 00/28] dynamic debug diet plan

[Jim Cromie]

DYNAMIC_DEBUG has extra weight/memory in its data table/section; its
(module,filename,function) columns have ordered, repeating data
(90%,55%,45%), and is highly compressible (~110/300) with simple RLE.

On my i7 laptop, the compressible chunk is ~70kb, This patchset
prepares for compression, but doesn't try it; RFC.  Using that RLE
compression estimate, we could save ~45kb; Id hope for better with ZSTD.

Series Overview:

1st, split the struct _ddebug & __dyndbg elf section into 2, moving
compressible fields into their own section/array/block.  A temporary
site pointer connects the 2 halves of the composite callsite, it will
be obsoleted later, after we verify our replacement against it, we can
drop the .site pointer.

table change:
 from:	[ key,mod,func,filenm,fmt,ln,flg ]
 into:	[ key,fmt,ln,flg,site-> ]	[ mod,func,filenm ]

2nd, make the 'site' data optional.  The "module:function:line"
log-message prefix is fundamentally an optional decoration, if we can
do without it, we simplify codepath, and we could drop the storage in
those cases, possibly reducing memory footprint.

 into:	[ key,fmt,ln,flg,site->? ]  0
 
Then things get more interesting:

3rd, make kernel/module handle "__dyndbg_sites" ELF section (which was
added in 1) as is already done for "__dyndbg".

Hide/replace ->site with site_get/_put api
this will let us retrieve site info from compressed storage.

Add _index.
gives us the fixed offset from __dyndbg[N] to __dyndbg[0].

 into:	[ key,fmt,ln,flg,_index,site->? ]  0

Add table/header record-pair.
These 2 records are created into ".gnu.linkonce.dyndbg*" sections.
The linker script places these 2 sections in front of their respective
"__dyndbg*" sections.
So this header-pair becomes __dyndbg[0], __dyndbg_sites[0].
we can use __dyndbg[0]->site[N] to get the site info.

module.lds.h - insert ".gnu.linkonce.dyndbg*" sections into "__dyndbg*".
Without this, headers arent part of the sections that kernel/module loads.

validate ddebug_site_get() invariants.
prove that our indirection to ->site[N] is correct.
BUG_ON uses survived 0-day tests.
they go away with !SITE_CHK

 into:
    0: 	[ ...,_index,site-> ]		--->  0:[ ... ]
    	  	^
  ..N:  [ key,fmt,ln,flg,_index ]	      N:[ ... ]

specialize header from _ddebug, keep header.site, drop _ddebug.site
RFC: needs refinement, better type choices?


Status:
  recent revs passed 0day compile grind, including BUG_ONs
  I got one KASAN report, quick reproduction failed, none more recently
  memory footprint is same as master.

Whats Next ? Compression: RFC !!!

POC: could compress during __init, decompress on-demand.  Doing this
would give something to decompress, to build out that code.

ISTM Compression during build is best, but targets are unclear:
initrd, initramfs, cpio, FW-blob all seem suitable at some level, but
so does (ro) zram & zswap.  How should I think about this ?

Also, I understand that support exists for compressed elf sections, I
dunno if/how kernel uses it currently, which would considerably ease
this.  Builtin __dyndbg_site data is in .data, probably needs to be
moved to a separate elf section to get generic tool support for
compressed sections.

It would be nice if a compressed "__dyndbg_sites" elf section could be
loaded by unchanged kernel/module.c.  A magic number in the attached
memory block could distinguish it from the current elf block.  There
is room in the 2 header records for new private data; we could store
decompression contexts there while iterating thru a block.

Decompression - a few use-cases:

= for builtins, each loadable module:
1- stream, for `cat control` etc.
   these are 1 visit and forget
2- `module foo +mfp > control` - enable contiguous blocks w decorations
3- pr_debug - semi-random access - by (ptr - _index)
   [0-5k] for builtin pr-debugs, [0-20] for most loadable modules
   _index is 16 bit currently

Simplest, minimum-memory strategy is late-fetch; to wait until a
pr_debug is actually called, then check flags before fetching the
site-rec, and save it to a hash on modname+_index (and delete on -p).
Without the save, it works for (1) too.

The only downside with late-fetch is that during pr_debug, a 1st fetch
of site-info maybe cant come from io/zswap without locking/latency
concerns.  A forced-prefetch ala (2) with a new flag '!' could work
around this, but might look like a wart.

So thats it. I hope its clear and concise.

Jim Cromie (28):

trivial cleanup:
  dyndbg: avoid calling dyndbg_emit_prefix when it has no work
  dyndbg: drop uninformative vpr_info

split to 2 vectors of callsite:
  dyndbg: split struct _ddebug's display fields to new _ddebug_site
  dyndbg: __init iterate over __dyndbg & __dyndbg_site in parallel

make site info optional:
  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: refactor ddebug_alter_site out of ddebug_change
  dyndbg: allow deleting site info via control interface

interesting: 
  dyndbg+module: expose ddebug_sites to modules
  dyndbg: add ddebug_site(_get|_put) abstraction
  dyndbg: ddebug_add_module avoid adding empty modules
  dyndbg: add _index to struct _ddebug
  dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE
  dyndbg: RFC - DEFINE_DYNAMIC_DEBUG_TABLE
  dyndbg: RFC handle __dyndbg* sections in module.lds.h
  dyndbg: ddebug_add_module() handle headers.
  dyndbg: validate ddebug_site_get invariants
  dyndbg: fix NULL deref after deleting sites
  dyndbg: dont show header records in control
  dyndbg: make site pointer and checks on it optional (not quite)
  dyndbg: swap WARN_ON for BUG_ON see what 0-day says
  dyndbg: fixup protect header when deleting site
  dyndbg: unionize _ddebug*_headers with struct _ddebug*
  dyndbg: drop _ddebug.site pointer

 arch/arm/boot/compressed/Makefile     |   2 +
 arch/sparc/vdso/Makefile              |   2 +
 arch/x86/boot/compressed/Makefile     |   1 +
 arch/x86/entry/vdso/Makefile          |   3 +
 arch/x86/purgatory/Makefile           |   1 +
 drivers/firmware/efi/libstub/Makefile |   3 +-
 include/asm-generic/module.lds.h      |  12 +-
 include/asm-generic/vmlinux.lds.h     |  24 +-
 include/linux/dynamic_debug.h         | 135 ++++++++--
 kernel/module-internal.h              |   1 +
 kernel/module.c                       |   9 +-
 lib/dynamic_debug.c                   | 363 +++++++++++++++++++-------
 12 files changed, 437 insertions(+), 119 deletions(-)

-- 
2.31.1

[RFC PATCH v5 01/28] dyndbg: avoid calling dyndbg_emit_prefix when it has no work

[Jim Cromie]

Wrap function in a static-inline one, which checks flags to avoid
calling the function unnecessarily.

And hoist its output-buffer initialization to the grand-caller, which
is already allocating the buffer on the stack, and can trivially
initialize it too.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h |  5 +++++
 lib/dynamic_debug.c           | 19 ++++++++++++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index a57ee75342cf..dce631e678dd 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -32,6 +32,11 @@ 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_ANY		\
+	(_DPRINTK_FLAGS_INCL_MODNAME | _DPRINTK_FLAGS_INCL_FUNCNAME |\
+	 _DPRINTK_FLAGS_INCL_LINENO  | _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 c70d6347afa2..ede4a491ee87 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -586,13 +586,11 @@ 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 *desc, char *buf)
 {
 	int pos_after_tid;
 	int pos = 0;
 
-	*buf = '\0';
-
 	if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
 			pos += snprintf(buf + pos, remaining(pos), "<intr> ");
@@ -618,11 +616,18 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
 	return buf;
 }
 
+static inline char *dynamic_emit_prefix(struct _ddebug *desc, char *buf)
+{
+	if (unlikely(desc->flags & _DPRINTK_FLAGS_INCL_ANY))
+		return __dynamic_emit_prefix(desc, buf);
+	return buf;
+}
+
 void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 {
 	va_list args;
 	struct va_format vaf;
-	char buf[PREFIX_SIZE];
+	char buf[PREFIX_SIZE] = "";
 
 	BUG_ON(!descriptor);
 	BUG_ON(!fmt);
@@ -655,7 +660,7 @@ void __dynamic_dev_dbg(struct _ddebug *descriptor,
 	if (!dev) {
 		printk(KERN_DEBUG "(NULL device *): %pV", &vaf);
 	} else {
-		char buf[PREFIX_SIZE];
+		char buf[PREFIX_SIZE] = "";
 
 		dev_printk_emit(LOGLEVEL_DEBUG, dev, "%s%s %s: %pV",
 				dynamic_emit_prefix(descriptor, buf),
@@ -684,7 +689,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 	vaf.va = &args;
 
 	if (dev && dev->dev.parent) {
-		char buf[PREFIX_SIZE];
+		char buf[PREFIX_SIZE] = "";
 
 		dev_printk_emit(LOGLEVEL_DEBUG, dev->dev.parent,
 				"%s%s %s %s%s: %pV",
@@ -720,7 +725,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 	vaf.va = &args;
 
 	if (ibdev && ibdev->dev.parent) {
-		char buf[PREFIX_SIZE];
+		char buf[PREFIX_SIZE] = "";
 
 		dev_printk_emit(LOGLEVEL_DEBUG, ibdev->dev.parent,
 				"%s%s %s %s: %pV",
-- 
2.31.1

[RFC PATCH v5 02/28] dyndbg: drop uninformative vpr_info

[Jim Cromie]

Remove a vpr_info which I added in 2012, when I knew even less than now.
In 2020, a simpler pr_fmt stripped it of context, and any remaining value.

no functional change.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ede4a491ee87..3a7d1f9bcf4d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -920,7 +920,6 @@ static const struct seq_operations ddebug_proc_seqops = {
 
 static int ddebug_proc_open(struct inode *inode, struct file *file)
 {
-	vpr_info("called\n");
 	return seq_open_private(file, &ddebug_proc_seqops,
 				sizeof(struct ddebug_iter));
 }
-- 
2.31.1

[RFC PATCH v5 03/28] dyndbg: split struct _ddebug's display fields to new _ddebug_site

[Jim Cromie]

Struct _ddebug has 2 kinds of fields: essential & optional/compressible.
Move 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
display control, and to select callsites to >control.  lineno is
arguably optional too, but it uses spare bytes in struct _ddebug, so
leaving it is free.

The "__dyndbg" ELF section contains the array of struct _ddebugs for
all the pr-debugs in the kernel/module.  Reuse this pattern for the
new "__dyndbg_sites" section.

The new ptr increases memory footprint, but it is temporary
scaffolding; once we can map from _ddebugs[N] --> _ddebug_sites[N]
indirectly, we can drop site pointer, regaining worst case memory
parity with master.

The indirection gives several advantages:

- site ptr lets us decouple the 2 arrays
  we can properly isolate dependencies by allowing null site

- 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.
  section compression at build time is practical - how ?
  include/linux/decompress/generic.h has no corresponding compression

- we could drop sites and their storage opportunistically.
  this could reduce per-site mem by 24/56.

  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");

Once we can map: ddebugs[N] -> ddebug_sites[N], we can:

- compress __dyndbg_sites during __init, and mark section __initdata
- store the compressed block instead.
- decompress on-demand, stream for `cat control`
- save chunks of decompressed buffer for enabled callsites
- free chunks on site disable, or on memory pressure.

Whats actually done here is ths rather mechanical, and preparatory.

dynamic_debug.h:

I cut struct _ddebug in half, renamed the 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 _ddebug.site to
point at body.

DEFINE_DYNAMIC_DEBUG_METADATA now declares and initializes a 2nd
static struct var holding the _ddebug_site, and refs _ddebug to it.

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 field moves 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 <jim.cromie@gmail.com>
---
 include/asm-generic/vmlinux.lds.h |  3 +++
 include/linux/dynamic_debug.h     | 37 +++++++++++++++++---------
 lib/dynamic_debug.c               | 44 ++++++++++++++++++-------------
 3 files changed, 52 insertions(+), 32 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 dce631e678dd..d56c02ed0c45 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 _ddebug.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
@@ -49,8 +57,7 @@ struct _ddebug {
 		struct static_key_false dd_key_false;
 	} key;
 #endif
-} __attribute__((aligned(8)));
-
+} __aligned(8);
 
 
 #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
@@ -88,11 +95,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 3a7d1f9bcf4d..c1c2c90ed944 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_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,12 +587,13 @@ 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;
 
-	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
@@ -599,15 +601,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)
@@ -884,6 +886,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) {
@@ -892,9 +895,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");
@@ -1097,17 +1102,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++;
@@ -1117,9 +1122,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.31.1

[RFC PATCH v5 04/28] dyndbg: __init iterate over __dyndbg & __dyndbg_site in parallel

[Jim Cromie]

In HEAD~1, a new elf section was added, and dynamic_debug_init() got
minimal adjustments; basically s/iter/iter->site/ where needed.

Now we rework the for-loop for clarity:
. iterate over both sections in parallel (iter & site)
. demote iter->site indirection (1st step towards dropping site)
. add __(start|stop)___dyndbg_sites decls (init'd in vmlinux.lds.h)
. add/rename several iterator/mod_start variables for clarity.
. add BUG_ON(iter->site != site)
. var rename n to site_ct

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 c1c2c90ed944..5a33143215a4 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)
 {
@@ -1086,11 +1089,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)) {
@@ -1101,23 +1105,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.31.1

[RFC PATCH v5 05/28] dyndbg: refactor part of ddebug_change to ddebug_match_site

[Jim Cromie]

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 <jim.cromie@gmail.com>
---
 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 5a33143215a4..e6e2cb0180e5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -142,6 +142,48 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
 		 query->first_lineno, query->last_lineno);
 }
 
+static int ddebug_match_site(const struct ddebug_query *query,
+			     const struct _ddebug *dp)
+{
+	struct _ddebug_site *dc = dp->site;
+
+	/* match against the source filename */
+	if (query->filename &&
+	    !match_wildcard(query->filename, dc->filename) &&
+	    !match_wildcard(query->filename,
+			    kbasename(dc->filename)) &&
+	    !match_wildcard(query->filename,
+			    trim_prefix(dc->filename)))
+		return false;
+
+	/* match against the function */
+	if (query->function &&
+	    !match_wildcard(query->function, dc->function))
+		return false;
+
+	/* match against the format */
+	if (query->format) {
+		if (*query->format == '^') {
+			char *p;
+			/* anchored search. match must be at beginning */
+			p = strstr(dp->format, query->format+1);
+			if (p != dp->format)
+				return false;
+		} else if (!strstr(dp->format, query->format))
+			return false;
+	}
+
+	/* match against the line number range */
+	if (query->first_lineno &&
+	    dp->lineno < query->first_lineno)
+		return false;
+	if (query->last_lineno &&
+	    dp->lineno > query->last_lineno)
+		return false;
+
+	return true;
+}
+
 /*
  * Search the tables for _ddebug's which match the given `query' and
  * apply the `flags' and `mask' to them.  Returns number of matching
@@ -170,38 +212,7 @@ static int ddebug_change(const struct ddebug_query *query,
 			struct _ddebug *dp = &dt->ddebugs[i];
 			struct _ddebug_site *dc = dp->site;
 
-			/* match against the source filename */
-			if (query->filename &&
-			    !match_wildcard(query->filename, dc->filename) &&
-			    !match_wildcard(query->filename,
-					   kbasename(dc->filename)) &&
-			    !match_wildcard(query->filename,
-					   trim_prefix(dc->filename)))
-				continue;
-
-			/* match against the function */
-			if (query->function &&
-			    !match_wildcard(query->function, dc->function))
-				continue;
-
-			/* match against the format */
-			if (query->format) {
-				if (*query->format == '^') {
-					char *p;
-					/* anchored search. match must be at beginning */
-					p = strstr(dp->format, query->format+1);
-					if (p != dp->format)
-						continue;
-				} else if (!strstr(dp->format, query->format))
-					continue;
-			}
-
-			/* match against the line number range */
-			if (query->first_lineno &&
-			    dp->lineno < query->first_lineno)
-				continue;
-			if (query->last_lineno &&
-			    dp->lineno > query->last_lineno)
+			if (!ddebug_match_site(query, dp))
 				continue;
 
 			nfound++;
-- 
2.31.1

[RFC PATCH v5 06/28] dyndbg: accept null site in ddebug_match_site

[Jim Cromie]

Since all query terms must be satisfied, we can reorder for clarity,
moving !!site dependent checks after those on _ddebug itself.

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.

no functional changes

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 e6e2cb0180e5..47d427b82ae1 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.31.1

[RFC PATCH v5 07/28] dyndbg: hoist ->site out of ddebug_match_site

[Jim Cromie]

A coming change adds _get/_put abstraction on the site pointer, to
allow managing site info more flexibly.  The get/put pattern is best
done at a single lexical scope, where its more obviously correct, so
hoist the ->site ref out of ddebug_match_site, and pass it in instead.

no functional changes

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 47d427b82ae1..0896c681db40 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -143,10 +143,9 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
 }
 
 static int ddebug_match_site(const struct ddebug_query *query,
-			     const struct _ddebug *dp)
+			     const struct _ddebug *dp,
+			     const struct _ddebug_site *dc)
 {
-	struct _ddebug_site *dc;
-
 	/* match against the format */
 	if (query->format) {
 		if (*query->format == '^') {
@@ -167,7 +166,6 @@ static int ddebug_match_site(const struct ddebug_query *query,
 	    dp->lineno > query->last_lineno)
 		return false;
 
-	dc = dp->site;
 	if (!dc) {
 		/* site info has been dropped, so query cannot test these fields */
 		if (query->filename || query->function)
@@ -219,9 +217,9 @@ static int ddebug_change(const struct ddebug_query *query,
 
 		for (i = 0; i < dt->num_ddebugs; i++) {
 			struct _ddebug *dp = &dt->ddebugs[i];
-			struct _ddebug_site *dc;
+			struct _ddebug_site *dc = dp->site;
 
-			if (!ddebug_match_site(query, dp))
+			if (!ddebug_match_site(query, dp, dc))
 				continue;
 
 			nfound++;
-- 
2.31.1

[RFC PATCH v5 08/28] dyndbg: accept null site in ddebug_change

[Jim Cromie]

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 <jim.cromie@gmail.com>
---
 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 0896c681db40..fa89d69dad4e 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -235,10 +235,17 @@ static int ddebug_change(const struct ddebug_query *query,
 				static_branch_enable(&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.31.1

[RFC PATCH v5 09/28] dyndbg: accept null site in dynamic_emit_prefix

[Jim Cromie]

2 prints use site->member, protect them with if site.

no functional changes.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 fa89d69dad4e..fb8547feb061 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -629,15 +629,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.31.1

[RFC PATCH v5 10/28] dyndbg: accept null site in ddebug_proc_show

[Jim Cromie]

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 <jim.cromie@gmail.com>
---
 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 fb8547feb061..5faf49054b1d 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -923,18 +923,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.31.1

[RFC PATCH v5 11/28] dyndbg: refactor ddebug_alter_site out of ddebug_change

[Jim Cromie]

Move the JUMP_LABEL/static-key code to a separate function.

no functional changes.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 5faf49054b1d..a4ddafdbd9a1 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.31.1

[RFC PATCH v5 12/28] dyndbg: allow deleting site info via control interface

[Jim Cromie]

Allow users & subsystems to selectively delete callsite info for
pr-debug callsites.  Hopefully, this can lead to actual recovery of
memory.

DRM is a potential user which would drop the sites:

- has distinct categories for logging, and can map them over to a
  format prefix, like: "drm:core:", "drm:kms:", etc.

- are happy with group control of all the callsites in a class/cateory.
  individual control is still possible using queries including line numbers

- don't need dynamic "module:function:line:" prefixes in log messages

- don't care about loss of context in /proc/dynamic_debug/control

before:

init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed data\012"
init/main.c:1337 [main]run_init_process =pm "    %s\012"
init/main.c:1335 [main]run_init_process =pm "  with environment:\012"
init/main.c:1334 [main]run_init_process =pm "    %s\012"
init/main.c:1332 [main]run_init_process =pm "  with arguments:\012"
init/main.c:1121 [main]initcall_blacklisted =pm "initcall %s blacklisted\012"
init/main.c:1082 [main]initcall_blacklist =pm "blacklisting initcall %s\012"

then:
  bash-5.0# echo file init/main.c +D > /proc/dynamic_debug/control

after:

init/initramfs.c:485 [initramfs]unpack_to_rootfs =_ "Detected %s compressed data\012"
[main]:1337 =pmD "    %s\012"
[main]:1335 =pmD "  with environment:\012"
[main]:1334 =pmD "    %s\012"
[main]:1332 =pmD "  with arguments:\012"
[main]:1121 =pmD "initcall %s blacklisted\012"
[main]:1082 =pmD "blacklisting initcall %s\012"

Notes:

If Drm adopted dyndbg, i915 + drm* would add ~1600 prdebugs, amdgpu +
drm* would add ~3200 callsites, so the additional memory costs are
substantial.  In trade, drm and drivers would avoid lots of calls to
drm_debug_enabled().  This patch should reduce the costs.

Using this interface, drm could drop site info for all categories /
prefixes controlled by bits in drm.debug, while preserving site info
and individual selectivity for any uncategorized prdebugs, and for all
other modules.

Lastly, because lineno field was not moved into _ddebug_callsite, it
can be used to modify a single[*] callsite even if drm has dropped all
the callsite data:

  echo module $mod format ^$prefix line $line +p >control

Dropping site info is a one-way, information losing operation, so
minor misuse is possible.  Worst case is maybe (depending upon
previous settings) some loss of logging context/decorations.

  echo +D > /proc/dynamic_debug/control

[*] amdgpu has some macros invoking clusters of pr_debugs; each use of
them creates a cluster of pr-debugs with the same line number.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 d56c02ed0c45..bc4e778b755c 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_ANY		\
 	(_DPRINTK_FLAGS_INCL_MODNAME | _DPRINTK_FLAGS_INCL_FUNCNAME |\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index a4ddafdbd9a1..76315d20672a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -92,6 +92,7 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
 	{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
 	{ _DPRINTK_FLAGS_INCL_TID, 't' },
 	{ _DPRINTK_FLAGS_NONE, '_' },
+	{ _DPRINTK_FLAGS_DELETE_SITE, 'D' },
 };
 
 struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
@@ -201,6 +202,14 @@ static void ddebug_alter_site(struct _ddebug *dp,
 	} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
 		static_branch_enable(&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.31.1

[RFC PATCH v5 13/28] dyndbg+module: expose ddebug_sites to modules

[Jim Cromie]

Up until now, the "__dyndbg_sites" section was kinda dangling.

Unlike the "__dyndbg" section, it was not explicitly loaded by
module.c:load_info(); but it came along quietly, perhaps because of
the per .site initialization (by DEFINE_DYNAMIC_DEBUG_METADATA)
across the section boundary, or maybe because x86 is more permissive.
ISTM we need to treat "__dyndbg_sites" section just like "__dyndbg",
on basic principles.  (And maybe do this with commit that splits the
sections, if robot tests indicate problems between).

The changes to load_info propagated out to ddebug_add_module's
prototype, which needs to manage the 2nd section like the 1st.

RFC: Maybe the 2 sections/arrays should be a single item/tuple, but I
didn't see any real benefit to adding something; recapitulating the
previous pattern was straightforward.

-v5 fix !CONFIG_DYNAMIC_DEBUG fn-stub
Reported-by: kernel test robot <lkp@intel.com>

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h |  8 ++++----
 kernel/module-internal.h      |  1 +
 kernel/module.c               |  9 ++++++---
 lib/dynamic_debug.c           | 20 ++++++++++++--------
 4 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index bc4e778b755c..868e0769b72d 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -66,8 +66,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, ...);
@@ -199,8 +199,8 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 #include <linux/errno.h>
 #include <linux/printk.h>
 
-static inline int ddebug_add_module(struct _ddebug *tab, unsigned int n,
-				    const char *modname)
+static inline int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+				    unsigned int numdbgs, const char *modname)
 {
 	return 0;
 }
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 76315d20672a..1441b31915e7 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 {
@@ -1008,14 +1009,14 @@ static const struct proc_ops proc_fops = {
  * Allocate a new ddebug_table for the given module
  * and add it to the global list.
  */
-int ddebug_add_module(struct _ddebug *tab, unsigned int n,
-			     const char *name)
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+		      unsigned int numdbgs, const char *modname)
 {
 	struct ddebug_table *dt;
 
 	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
 	if (dt == NULL) {
-		pr_err("error adding module: %s\n", name);
+		pr_err("error adding module: %s\n", modname);
 		return -ENOMEM;
 	}
 	/*
@@ -1024,15 +1025,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;
 }
 
@@ -1172,7 +1174,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;
@@ -1182,7 +1186,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.31.1

[RFC PATCH v5 14/28] dyndbg: add ddebug_site(_get|_put) abstraction

[Jim Cromie]

Replace direct ->site refs with _get(),_put() internal API.  Right
now, _get() just returns ->site and _put() does nothing.  Later we can
replace this implementation with one using ->_index to fetch site data
then forget or keep it selectively.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 1441b31915e7..90bea97c0da1 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;
 
 	if (dp->flags & _DPRINTK_FLAGS_INCL_TID) {
 		if (in_interrupt())
@@ -648,6 +661,7 @@ static char *__dynamic_emit_prefix(const struct _ddebug *dp, char *buf)
 	}
 	pos_after_tid = pos;
 
+	desc = ddebug_site_get(dp);
 	if (desc) {
 		if (dp->flags & _DPRINTK_FLAGS_INCL_MODNAME)
 			pos += snprintf(buf + pos, remaining(pos), "%s:",
@@ -665,6 +679,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;
 }
 
@@ -947,7 +963,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,
@@ -963,6 +980,8 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 		seq_puts(m, "\"\n");
 	}
 
+	ddebug_site_put(dp);
+
 	return 0;
 }
 
-- 
2.31.1

[RFC PATCH v5 15/28] dyndbg: ddebug_add_module avoid adding empty modules

[Jim Cromie]

Don't create ddebug_table's for modules with 0 callsites.  This saves
memory, and avoids creating ddebug_tables with questionable contents,
which are then iterated over for 'cat control'.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 90bea97c0da1..77c5135879c2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1033,6 +1033,12 @@ int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 {
 	struct ddebug_table *dt;
 
+	v3pr_info("add-module: %s.%d sites\n", modname, numdbgs);
+	if (!numdbgs) {
+		v3pr_info(" skip %s\n", modname);
+		return 0;
+	}
+
 	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
 	if (dt == NULL) {
 		pr_err("error adding module: %s\n", modname);
-- 
2.31.1

[RFC PATCH v5 16/28] dyndbg: add _index to struct _ddebug

[Jim Cromie]

We currently use dp->site to map: &__dyndbg[N] -> &__dyndbg_sites[N].
We want to drop site; new _ddebug._index provides the N.  This just
initializes that index.

ddebug_add_module()'s new job is to initialize _index.  In order to
handle builtin modules (sections contain catenated blocks of modules'
callsites) it gets a new base arg to monotonically increment _index
over multiple modules.  Since ddebug_add_module() is used indirectly
by module.c, the new arg is hidden in __ddebug_add_module(), and
defaults to 0 in the wrapper.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h |  2 ++
 lib/dynamic_debug.c           | 25 ++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 868e0769b72d..a15e417cbba8 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -29,6 +29,7 @@ struct _ddebug {
 	/* format is always needed, lineno shares word with flags */
 	const char *format;
 	const unsigned lineno:18;
+	unsigned _index:14;
 	/*
 	 * The flags field controls the behaviour at the callsite.
 	 * The bits here are changed dynamically when the user
@@ -52,6 +53,7 @@ struct _ddebug {
 #define _DPRINTK_FLAGS_DEFAULT 0
 #endif
 	unsigned int flags:8;
+
 #ifdef CONFIG_JUMP_LABEL
 	union {
 		struct static_key_true dd_key_true;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 77c5135879c2..c5927b6c1c0c 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1028,10 +1028,12 @@ static const struct proc_ops proc_fops = {
  * Allocate a new ddebug_table for the given module
  * and add it to the global list.
  */
-int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
-		      unsigned int numdbgs, const char *modname)
+static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+			       unsigned int numdbgs, unsigned int base,
+			       const char *modname)
 {
 	struct ddebug_table *dt;
+	int i;
 
 	v3pr_info("add-module: %s.%d sites\n", modname, numdbgs);
 	if (!numdbgs) {
@@ -1055,6 +1057,12 @@ int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 	dt->ddebugs = tab;
 	dt->sites = sites;
 
+	for (i = 0; i < numdbgs; i++, base++) {
+		tab[i]._index = base;
+		v3pr_info(" %d %d %s.%s.%d\n", i, base, modname,
+			  tab[i].site->function, tab[i].lineno);
+	}
+
 	mutex_lock(&ddebug_lock);
 	list_add(&dt->link, &ddebug_tables);
 	mutex_unlock(&ddebug_lock);
@@ -1063,6 +1071,12 @@ int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 	return 0;
 }
 
+int ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
+		      unsigned int numdbgs, const char *modname)
+{
+	return __ddebug_add_module(tab, sites, numdbgs, 0, modname);
+}
+
 /* helper for ddebug_dyndbg_(boot|module)_param_cb */
 static int ddebug_dyndbg_param_cb(char *param, char *val,
 				const char *modname, int on_err)
@@ -1177,6 +1191,7 @@ static int __init dynamic_debug_init(void)
 	char *cmdline;
 	int ret = 0;
 	int site_ct = 0, entries = 0, modct = 0;
+	int mod_index = 0;
 
 	if (&__start___dyndbg == &__stop___dyndbg) {
 		if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
@@ -1200,8 +1215,8 @@ static int __init dynamic_debug_init(void)
 		if (strcmp(modname, site->modname)) {
 			modct++;
 
-			ret = ddebug_add_module(iter_mod_start, site_mod_start,
-						site_ct, modname);
+			ret = __ddebug_add_module(iter_mod_start, site_mod_start,
+						  site_ct, mod_index, modname);
 			if (ret)
 				goto out_err;
 			site_ct = 0;
@@ -1211,7 +1226,7 @@ static int __init dynamic_debug_init(void)
 		}
 		site_ct++;
 	}
-	ret = ddebug_add_module(iter_mod_start, site_mod_start, site_ct, modname);
+	ret = __ddebug_add_module(iter_mod_start, site_mod_start, site_ct, mod_index, modname);
 	if (ret)
 		goto out_err;
 
-- 
2.31.1

[RFC PATCH v5 17/28] dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE

[Jim Cromie]

The next patch adds DEFINE_DYNAMIC_DEBUG_TABLE(), which broke several
subtrees, including efi, vdso, and some of arch/*/boot/compressed,
with various relocation errors, iirc.

Avoid those problems by adding a define to suppress the "transparent"
DEFINE_DYNAMIC_DEBUG_TABLE() invocation.  I found the x86 problems
myself, lkp@intel.com found arm & sparc problems, and may yet find
others.

Reported-by: <lkp@intel.com> # on [jimc:lkp-test/dyndbg-diet] recently
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 arch/arm/boot/compressed/Makefile     | 2 ++
 arch/sparc/vdso/Makefile              | 2 ++
 arch/x86/boot/compressed/Makefile     | 1 +
 arch/x86/entry/vdso/Makefile          | 3 +++
 arch/x86/purgatory/Makefile           | 1 +
 drivers/firmware/efi/libstub/Makefile | 3 ++-
 6 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index fd94e27ba4fa..72f056a00ad4 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -82,6 +82,8 @@ compress-$(CONFIG_KERNEL_LZMA) = lzma
 compress-$(CONFIG_KERNEL_XZ)   = xzkern
 compress-$(CONFIG_KERNEL_LZ4)  = lz4
 
+KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
+
 libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o
 
 ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
diff --git a/arch/sparc/vdso/Makefile b/arch/sparc/vdso/Makefile
index c5e1545bc5cf..960ed0fb6804 100644
--- a/arch/sparc/vdso/Makefile
+++ b/arch/sparc/vdso/Makefile
@@ -30,6 +30,8 @@ obj-y += $(vdso_img_objs)
 targets += $(vdso_img_cfiles)
 targets += $(vdso_img_sodbg) $(vdso_img-y:%=vdso%.so)
 
+KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
+
 CPPFLAGS_vdso.lds += -P -C
 
 VDSO_LDFLAGS_vdso.lds = -m elf64_sparc -soname linux-vdso.so.1 --no-undefined \
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index e0bc3988c3fa..ada4eb960d95 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -31,6 +31,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \
 KBUILD_CFLAGS := -m$(BITS) -O2
 KBUILD_CFLAGS += -fno-strict-aliasing -fPIE
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
+KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
 cflags-$(CONFIG_X86_32) := -march=i386
 cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone
 KBUILD_CFLAGS += $(cflags-y)
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index 05c4abc2fdfd..619878f2c427 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -29,6 +29,9 @@ vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
 vobjs32-y += vdso32/vclock_gettime.o
 vobjs-$(CONFIG_X86_SGX)	+= vsgx.o
 
+# avoid a x86_64_RELATIVE error
+KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
+
 # files to link into kernel
 obj-y				+= vma.o extable.o
 KASAN_SANITIZE_vma.o		:= y
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 95ea17a9d20c..95ba7b18410f 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -35,6 +35,7 @@ PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
 PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0
 PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING
 PURGATORY_CFLAGS += -fno-stack-protector
+PURGATORY_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
 
 # Default KBUILD_CFLAGS can have -pg option set when FTRACE is enabled. That
 # in turn leaves some undefined symbols like __fentry__ in purgatory and not
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index c23466e05e60..def8febefbd3 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -13,7 +13,8 @@ cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ \
 				   -Wno-pointer-sign \
 				   $(call cc-disable-warning, address-of-packed-member) \
 				   $(call cc-disable-warning, gnu) \
-				   -fno-asynchronous-unwind-tables
+				   -fno-asynchronous-unwind-tables \
+				   -DNO_DYNAMIC_DEBUG_TABLE
 
 # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
 # disable the stackleak plugin
-- 
2.31.1

Re: [RFC PATCH v5 17/28] dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE

[Ard Biesheuvel]

On Tue, 11 May 2021 at 20:51, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> The next patch adds DEFINE_DYNAMIC_DEBUG_TABLE(), which broke several
> subtrees, including efi, vdso, and some of arch/*/boot/compressed,
> with various relocation errors, iirc.
>
> Avoid those problems by adding a define to suppress the "transparent"
> DEFINE_DYNAMIC_DEBUG_TABLE() invocation.  I found the x86 problems
> myself, lkp@intel.com found arm & sparc problems, and may yet find
> others.
>

Given that I was only cc'ed on this patch in isolation, would you mind
adding more clarification here? What is DEFINE_DYNAMIC_DEBUG_TABLE()
supposed to do, and why is it breaking standalone binaries?


> Reported-by: <lkp@intel.com> # on [jimc:lkp-test/dyndbg-diet] recently
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  arch/arm/boot/compressed/Makefile     | 2 ++
>  arch/sparc/vdso/Makefile              | 2 ++
>  arch/x86/boot/compressed/Makefile     | 1 +
>  arch/x86/entry/vdso/Makefile          | 3 +++
>  arch/x86/purgatory/Makefile           | 1 +
>  drivers/firmware/efi/libstub/Makefile | 3 ++-
>  6 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index fd94e27ba4fa..72f056a00ad4 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -82,6 +82,8 @@ compress-$(CONFIG_KERNEL_LZMA) = lzma
>  compress-$(CONFIG_KERNEL_XZ)   = xzkern
>  compress-$(CONFIG_KERNEL_LZ4)  = lz4
>
> +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
> +
>  libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o
>
>  ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
> diff --git a/arch/sparc/vdso/Makefile b/arch/sparc/vdso/Makefile
> index c5e1545bc5cf..960ed0fb6804 100644
> --- a/arch/sparc/vdso/Makefile
> +++ b/arch/sparc/vdso/Makefile
> @@ -30,6 +30,8 @@ obj-y += $(vdso_img_objs)
>  targets += $(vdso_img_cfiles)
>  targets += $(vdso_img_sodbg) $(vdso_img-y:%=vdso%.so)
>
> +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
> +
>  CPPFLAGS_vdso.lds += -P -C
>
>  VDSO_LDFLAGS_vdso.lds = -m elf64_sparc -soname linux-vdso.so.1 --no-undefined \
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index e0bc3988c3fa..ada4eb960d95 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -31,6 +31,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \
>  KBUILD_CFLAGS := -m$(BITS) -O2
>  KBUILD_CFLAGS += -fno-strict-aliasing -fPIE
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
> +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
>  cflags-$(CONFIG_X86_32) := -march=i386
>  cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone
>  KBUILD_CFLAGS += $(cflags-y)
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index 05c4abc2fdfd..619878f2c427 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -29,6 +29,9 @@ vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
>  vobjs32-y += vdso32/vclock_gettime.o
>  vobjs-$(CONFIG_X86_SGX)        += vsgx.o
>
> +# avoid a x86_64_RELATIVE error
> +KBUILD_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
> +
>  # files to link into kernel
>  obj-y                          += vma.o extable.o
>  KASAN_SANITIZE_vma.o           := y
> diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
> index 95ea17a9d20c..95ba7b18410f 100644
> --- a/arch/x86/purgatory/Makefile
> +++ b/arch/x86/purgatory/Makefile
> @@ -35,6 +35,7 @@ PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
>  PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0
>  PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING
>  PURGATORY_CFLAGS += -fno-stack-protector
> +PURGATORY_CFLAGS += -DNO_DYNAMIC_DEBUG_TABLE
>
>  # Default KBUILD_CFLAGS can have -pg option set when FTRACE is enabled. That
>  # in turn leaves some undefined symbols like __fentry__ in purgatory and not
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index c23466e05e60..def8febefbd3 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -13,7 +13,8 @@ cflags-$(CONFIG_X86)          += -m$(BITS) -D__KERNEL__ \
>                                    -Wno-pointer-sign \
>                                    $(call cc-disable-warning, address-of-packed-member) \
>                                    $(call cc-disable-warning, gnu) \
> -                                  -fno-asynchronous-unwind-tables
> +                                  -fno-asynchronous-unwind-tables \
> +                                  -DNO_DYNAMIC_DEBUG_TABLE
>
>  # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
>  # disable the stackleak plugin
> --
> 2.31.1
>

Re: [RFC PATCH v5 17/28] dyndbg: prevent build bugs via -DNO_DYNAMIC_DEBUG_TABLE

[jim.cromie]

On Wed, May 12, 2021 at 8:55 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 11 May 2021 at 20:51, Jim Cromie <jim.cromie@gmail.com> wrote:
> >
> > The next patch adds DEFINE_DYNAMIC_DEBUG_TABLE(), which broke several
> > subtrees, including efi, vdso, and some of arch/*/boot/compressed,
> > with various relocation errors, iirc.
> >
> > Avoid those problems by adding a define to suppress the "transparent"
> > DEFINE_DYNAMIC_DEBUG_TABLE() invocation.  I found the x86 problems
> > myself, lkp@intel.com found arm & sparc problems, and may yet find
> > others.
> >
>
> Given that I was only cc'ed on this patch in isolation, would you mind
> adding more clarification here? What is DEFINE_DYNAMIC_DEBUG_TABLE()
> supposed to do, and why is it breaking standalone binaries?
>
>

hi Ard,

the thread starts here:
https://lore.kernel.org/linux-mm/20210511185057.3815777-1-jim.cromie@gmail.com/

the _TABLE macro derives from DEFINE_DYNAMIC_DEBUG_METADATA,
which puts private static struct _ddebug's in section("__dyndbg")
the _TABLE macro populates a different section(".gnu.linkonce.dyndbg"),
which is then placed by linker script at the start of the section.

ISTM that the new section might be whats breaking things.
And maybe that the vmlinux linker script isnt involved.
so the storage the _TABLE wants to define is unbound
(and unused, since there are no pr_debugs)
I did see relocation errors somewhere...

This is my 1st time doing something creative with the linker


As to larger purpose, I'll try to restate the patchset mission:

theres ~45kb savings possible by compressing the highly redundant data (~70kb)
 which decorates pr_debug messages.

1 - split the compressible/decoration columns to a different
section|block, for block compression
      this adds temporary .site pointer from _ddebug -> _ddebug_site

2 -  change code so !site is safe.


_TABLEs only real job is to provide a header record, at the beginning
of the section/array,
for a single .site pointer to the _dyndbg_sites section added in 1.
Because the header has a fixed offset from any pr_debug in the vector,
all pr_debugs can use the headers copy of .site, and dont need their own.
specialize & unionize

So it allows to drop the temporary pointer, restoring memory size
parity with master.
And we then have the _dyndbg_sites  section, full of redundant data,
ready to compress.

suppression with -DNO_DYNAMIC_DEBUG was a workaround, didnt think
about it afterwards

does this clarify ?


thanks
Jim

[RFC PATCH v5 18/28] dyndbg: RFC - DEFINE_DYNAMIC_DEBUG_TABLE

[Jim Cromie]

DEFINE_DYNAMIC_DEBUG_TABLE is based on DEFINE_DYNAMIC_DEBUG_METADATA.
Like its model, it creates/allocates a pair of structs: _ddebug &
_ddebug_site.  It inits them distinctively, so is_dyndbg_header_pair()
macro can verify them.

Its purpose is to reserve a single pair of header records in the front
of the "__dyndbg" & "__dyndbg_sites" sections.  This has several parts;

- the pair of structs are placed into 2 .gnu.linkonce.dyndbg* sections
  corresponding to the 2 dyndbg* sections populated by _METADATA

- vmlinux.lds.h places these 2 new sections into the dyndbg* sections,
  at the start___dyndbg*s.

- the pair has "__used __weak" to qualm compiler concerns.
  explicit externs were problematic with initialization, static decls too.

With this, the header record is now really __dyndbg[0].

Notes:

DYNAMIC_DEBUG is a 'transparent' facility, in that pr_debug() users
get extra features without additional api.  Because of this,
DEFINE_DYNAMIC_DEBUG_TABLE is 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.  Placing these records into .gnu.linkonce. sections
with "__used __weak " seems to resolve the redundancies.  This may
well be the cause of the need for HEAD~1.

In vmlinux-lds.h, I added 2 more KEEPs to place the .gnu.linkonce.*
headers at the front of their respective __dyndbg* sections.  I moved
the __dyndbg lines into a new macro, which maybe should be done as a
separate commit, or in the earlier commit that touched it. RFC.

The headers are really just struct _ddebug*s, they are initialized
distinctively so that they're recogizable by code, using macro
is_dyndbg_header_pair(dbg, dbgsite).

DECLARE_DYNAMIC_DEBUG_TABLE() initializes the header record pairs with
values which are "impossible" for pr_debug()s:

- lineno == 0
- site == iter->site
- modname == function		not possible in proper linkage
- modname == format		not possible in normal linkage
- filename = (void*) iter	forced loopback

Next:

Get __dyndbg[0] from any *dp within __dyndbg[N].  Then with
__dyndbg[0].site, we can get __dyndbg_sites[N].  This is a little
slower than a direct pointer, but this is an unlikely debug path, so
this 'up-N-over-down-N' access is ok.

Eventually we can adapt the header (as a new struct/union) to keep its
pointer to the __dyndbg_sites[] section/block, while allowing us to
drop it from struct _ddebug, regaining memory parity with master.  But
for now, we keep .site, and will soon use it to validate the
'up-N-over-down-N' computation.

For clarity, N is _ddebug._index.  For both builtins & loadable
modules, it is init'd to remember the fixed offset from the beginning
of the section/block/memory-allocation (actual elf sections for *ko's,
and a __start/__stop delineated part of .data for builtins).

N/_index will be used solely to get to __debugs[0] and over to
__debug_sites[N].  It is distinct from a module's numdbgs, which is
used mainly when walking control, and is kept in struct _ddeug_table.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/asm-generic/vmlinux.lds.h | 27 ++++++++++++-------
 include/linux/dynamic_debug.h     | 45 +++++++++++++++++++++++++++++++
 lib/dynamic_debug.c               | 18 ++++++-------
 3 files changed, 72 insertions(+), 18 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 4f2af9de2f03..189286405b40 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)
+#define DYNAMIC_DEBUG_DATA()						\
+	. = ALIGN(8);							\
+	__start___dyndbg_sites = .;					\
+	KEEP(*(.gnu.linkonce.dyndbg_site))				\
+	KEEP(*(__dyndbg_sites))						\
+	__stop___dyndbg_sites = .;					\
+	__start___dyndbg = .;						\
+	KEEP(*(.gnu.linkonce.dyndbg))					\
+	KEEP(*(__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 a15e417cbba8..06ae1d8d8c10 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -113,6 +113,43 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 		_DPRINTK_KEY_INIT				\
 	}
 
+/*
+ * DEFINE_DYNAMIC_DEBUG_TABLE(): This is a special version of
+ * DEFINE_DYNAMIC_DEBUG_METADATA().  It creates a single pair of
+ * header records, which linker scripts place into __dyndbg[0] &
+ * __dyndbg_sites[0].
+ * To allow a robust runtime test for is_dyndbg_header_pair(I,S),
+ * force-initialize S->filename to point back to I, an otherwize
+ * pathological condition.
+ */
+#define DEFINE_DYNAMIC_DEBUG_TABLE()				\
+	/* forward decl, allowing loopback pointer */		\
+	__weak struct _ddebug __used __aligned(8)		\
+		__section(".gnu.linkonce.dyndbg")		\
+		_LINKONCE_dyndbg_header;			\
+	__weak struct _ddebug_site __used __aligned(8)		\
+		__section(".gnu.linkonce.dyndbg_site")		\
+	_LINKONCE_dyndbg_site_header = {			\
+		.modname = KBUILD_MODNAME,			\
+		.function = KBUILD_MODNAME,			\
+		/* forced pointer loopback, for distinction */	\
+		.filename = (void *) &_LINKONCE_dyndbg_header	\
+	};							\
+	__weak struct _ddebug __used __aligned(8)		\
+		__section(".gnu.linkonce.dyndbg")		\
+	_LINKONCE_dyndbg_header = {				\
+		.site = &_LINKONCE_dyndbg_site_header,		\
+		.format = KBUILD_MODNAME			\
+	}
+
+/* The header initializations as a distinguishing predicate */
+#define is_dyndbg_header_pair(iter, sitep)			\
+	(sitep == iter->site					\
+	 && (!iter->lineno)					\
+	 && (iter->format == sitep->modname)			\
+	 && (sitep->modname == sitep->function)			\
+	 && ((void *)sitep->filename == (void *)iter))
+
 #ifdef CONFIG_JUMP_LABEL
 
 #ifdef DEBUG
@@ -243,4 +280,12 @@ 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 when -DNO_DYNAMIC_DEBUG_TABLE */
+DEFINE_DYNAMIC_DEBUG_TABLE();
+#endif
+
 #endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c5927b6c1c0c..9d9cb36f40a6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1190,8 +1190,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)) {
@@ -1207,10 +1206,9 @@ 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++) {
 
-		BUG_ON(site != iter->site);
-		entries++;
+		WARN_ON(site != iter->site);
 
 		if (strcmp(modname, site->modname)) {
 			modct++;
@@ -1219,10 +1217,12 @@ 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++;
 	}
@@ -1232,9 +1232,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') {
-- 
2.31.1

[RFC PATCH v5 19/28] dyndbg: RFC handle __dyndbg* sections in module.lds.h

[Jim Cromie]

Up until now, loadable modules have tacitly linked the 2 __dyndbg*
sections into the .ko, as observable in log by enabling module.c's
pr_debugs and loading a module.

But now, we need to explicitly place the header sections in front of
their respective __dyndbg* sections, so we reuse input section names
for the output.

This gives us the placement we need for the header record, which we
can see in the "add-module:"s and elements "0 0" below:

    "0 0" lines are headers: predicate (function==module && !lineno)
    "X debug prints in" are 1 too high, they count headers.
    we are adding tables for empty modules (1st 2 below)

[    7.578873] dyndbg: add-module: ghash_clmulni_intel
[    7.579716] dyndbg:  0 0 ghash_clmulni_intel.ghash_clmulni_intel.0
[    7.608995] dyndbg:   1 debug prints in module ghash_clmulni_intel
[    8.078085] dyndbg: add-module: rapl
[    8.078977] dyndbg:  0 0 rapl.rapl.0
[    8.079584] dyndbg:   1 debug prints in module rapl
[    8.082009] RAPL PMU: API unit is 2^-32 Joules, 0 fixed counters, 10737418240 ms ovfl timer
[    8.099294] dyndbg: add-module: intel_rapl_common
[    8.100177] dyndbg:  0 0 intel_rapl_common.intel_rapl_common.0
[    8.101026] dyndbg:  1 1 intel_rapl_common.rapl_remove_package.1279
[    8.101931] dyndbg:  2 2 intel_rapl_common.rapl_detect_domains.1245
[    8.102836] dyndbg:  3 3 intel_rapl_common.rapl_detect_domains.1242
[    8.103778] dyndbg:  4 4 intel_rapl_common.rapl_package_register_powercap.1159
[    8.104960] dyndbg:  5 5 intel_rapl_common.rapl_package_register_powercap.1145
[    8.106246] dyndbg:  6 6 intel_rapl_common.rapl_package_register_powercap.1114
[    8.107302] dyndbg:  7 7 intel_rapl_common.rapl_package_register_powercap.1108
[    8.108338] dyndbg:  8 8 intel_rapl_common.rapl_update_domain_data.1083
[    8.109278] dyndbg:  9 9 intel_rapl_common.rapl_check_unit_atom.824
[    8.110255] dyndbg:  10 10 intel_rapl_common.rapl_check_unit_core.796
[    8.111361] dyndbg:  11 11 intel_rapl_common.rapl_read_data_raw.722
[    8.112301] dyndbg:  12 12 intel_rapl_common.contraint_to_pl.303
[    8.113276] dyndbg:  13 debug prints in module intel_rapl_common
[    8.130452] dyndbg: add-module: intel_rapl_msr
[    8.131140] dyndbg:  0 0 intel_rapl_msr.intel_rapl_msr.0
[    8.132026] dyndbg:  1 1 intel_rapl_msr.rapl_msr_probe.172
[    8.132818] dyndbg:  2 2 intel_rapl_msr.rapl_msr_read_raw.104
[    8.133794] dyndbg:   3 debug prints in module intel_rapl_msr

This gives us the property we need:

   fixed offset from &__dyndbg[N] to &__dyndbg[0]
   container_of gets &header
   header has ptr to __dyndbg_sites[]
   we can (in principle) drop __dyndbg.site ptr
   (after we adapt header to keep it)

TODO:

At this point, for loaded modules, ddebug_add_module() sees the header
as 0'th element, as we need in order to drop site (and regain worst
case footprint parity).

It could/should properly init this header to support the _sites[n]
lookup for loaded mods.  Or at least handle it explicitly.  Or at
least see what proc-show does with it currently.

Note that for builtins, decided by (__start <= dp < __stop), we use
__start___dyndbg_sites[N] directly, and dont need the header.

But maybe we should use it anyway, double-checking/BUGing when wrong.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/asm-generic/module.lds.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/module.lds.h b/include/asm-generic/module.lds.h
index f210d5c1b78b..328c48dfc88c 100644
--- a/include/asm-generic/module.lds.h
+++ b/include/asm-generic/module.lds.h
@@ -4,7 +4,17 @@
 
 /*
  * <asm/module.lds.h> can specify arch-specific sections for linking modules.
- * Empty for the asm-generic header.
+ *
+ * For loadable modules with CONFIG_DYNAMIC_DEBUG, we need to keep the
+ * 2 __dyndbg* ELF sections, which are loaded by module.c
+ *
+ * Pack the 2 __dyndbg* input sections with their respective
+ * .gnu.linkonce. header records into 2 output sections, with those
+ * header records in the 0th element.
  */
+SECTIONS {
+__dyndbg_sites	: ALIGN(8) { *(.gnu.linkonce.dyndbg_site) *(__dyndbg_sites) }
+__dyndbg	: ALIGN(8) { *(.gnu.linkonce.dyndbg)	  *(__dyndbg) }
+}
 
 #endif /* __ASM_GENERIC_MODULE_LDS_H */
-- 
2.31.1

[RFC PATCH v5 20/28] dyndbg: ddebug_add_module() handle headers.

[Jim Cromie]

Now that header records are in the __dyndbg* sections,
ddebug_add_module() sees them when they're present (when adding
loadable modules and the 1st builtin, but not 2nd..Nth).  Teach
ddebug_add_module() to recognize and account for them.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9d9cb36f40a6..462d364fc788 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1036,7 +1036,18 @@ static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 	int i;
 
 	v3pr_info("add-module: %s.%d sites\n", modname, numdbgs);
-	if (!numdbgs) {
+
+	if (numdbgs && is_dyndbg_header_pair(tab, sites)) {
+
+		v3pr_info(" header: %d %s.%s.%d\n", tab[0]._index, modname,
+			  tab[0].site->function, tab[0].lineno);
+		WARN_ON(tab[0].site != &sites[0]);
+		if (numdbgs <= 1) {
+			v3pr_info(" skip header %s\n", modname);
+			return 0;
+		}
+
+	} else if (!numdbgs) {
 		v3pr_info(" skip %s\n", modname);
 		return 0;
 	}
-- 
2.31.1

[RFC PATCH v5 21/28] dyndbg: validate ddebug_site_get invariants

[Jim Cromie]

This commit adds several BUG_ONs to assert all the invariants needed
to support the reliance on the "back-N-to-header-overto-site-out-N"
use of the 2 __dyndbg* vectors (with their included headers).

RFC: I don't think we want this permanently; BUG_ON/panic seems kinda
overkill, but its useful to know if it survives lkp auto-testing.

- dp is (struct _ddebug*) to the callsite, passed in.
- dh is (struct _ddebug*) to the header. derived from dp & _index.
  known by BUG_ON(!is_dyndbg_header(dh))
  this is the "up-N-to-header" from dp.
- dh has good site pointer, to __dyndbg_sites[]
  by BUG_ON(!is_dyndbg_header_pair())

There are 2 main cases to validate: builtin and loadable modules.

For loadable modules, we will depend upon the headers presence, and
its site pointer to the vector of _ddebug_sites[], expressed as:

  BUG_ON(&dh->site[dp->_index] != dp->site);

Builtin pr-debugs have the additional property:

  !!(&__start___dyndbg <= dp < __stop___dyndbg),

We could use this property directly to return site, but since builtin
modules also have a header record, we can use that instead.  So the
1st BUG_ON is hoisted out of the !builtin branch, and asserted just
before return.  Also hoist dh derivation, making it a declaration +
initialization + BUG_ON instead.

NB: grep -- '->site' will confirm that site is now used just for
BUG_ON assertions, so we are close to the drop.

NEXT

To drop site pointer from struct _ddebug, we need:

- recast header as a different struct, unionized with _ddebug.
- preserve site pointer there.
- drop from struct _ddebug.
- fix and fold back any fallout from size reduction.

OR defer that, and proceed with compressing __dyndbg_sites[], then
replacing ddebug_site_get's guts (with all the BUG_ONs) with a
decompress and _index.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 462d364fc788..af9791258f8f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -123,6 +123,8 @@ do {								\
 #define vpr_info(fmt, ...)	vnpr_info(1, fmt, ##__VA_ARGS__)
 #define v2pr_info(fmt, ...)	vnpr_info(2, fmt, ##__VA_ARGS__)
 #define v3pr_info(fmt, ...)	vnpr_info(3, fmt, ##__VA_ARGS__)
+#define v4pr_info(fmt, ...)	vnpr_info(4, fmt, ##__VA_ARGS__)
+#define v5pr_info(fmt, ...)	vnpr_info(5, fmt, ##__VA_ARGS__)
 
 static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
 {
@@ -146,7 +148,34 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
 
 static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 {
-	return dp->site; /* passthru abstraction */
+	struct _ddebug *dh = dp - (dp->_index);
+
+	WARN_ON(!is_dyndbg_header_pair(dh, dh->site));
+
+	if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
+
+		v5pr_info("get: %s is builtin: %d %d %s:%s:%d\n",
+			  dp->site->modname, dp->_index, (int)(dp - dh),
+			  dh->site[dp->_index].filename,
+			  dh->site[dp->_index].function, dp->lineno);
+
+		WARN_ON(dp != &__start___dyndbg[dp->_index]);
+
+		WARN_ON(!(dp->_index == (dp - dh) &&
+			 dp->_index == (dp - __start___dyndbg) &&
+			 dp->_index == (&__start___dyndbg_sites[dp->_index]
+					- &__start___dyndbg_sites[0])));
+
+		WARN_ON(&__start___dyndbg_sites[dp->_index] != dp->site);
+	} else {
+		v4pr_info("get: %s is loaded: %d %s:%s:%d\n",
+			  dp->site->modname, dp->_index,
+			  dh->site[dp->_index].filename,
+			  dh->site[dp->_index].function, dp->lineno);
+	}
+	WARN_ON(&dh->site[dp->_index] != dp->site);
+
+	return dp->site;
 }
 static inline void ddebug_site_put(struct _ddebug *dp)
 {
-- 
2.31.1

[RFC PATCH v5 22/28] dyndbg: fix NULL deref after deleting sites

[Jim Cromie]

After `echo module main +D > control` zeros the site pointer for
main's callsites, `cat control` causes a NULL deref in
ddebug_site_get().  Fix this with:

- in vpr_infos, avoid dp->site->module, use dh->sites[dp->_index]
- qualify WARN_ONs that test against dp->site.

Also return dp->site, which may be null.  This restores the
abbreviated control output of deleted sites, rather than pretending it
wasnt deleted.

Deleting sites isn't an important feature, and its current form will
be obsolete when the site pointer gets dropped.  Its also pointless if
the site data is in compressed blocks.  But its still worthwhile to
maintain !site robustness for a bit.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index af9791258f8f..d0477450ec0c 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -154,8 +154,8 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 
 	if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
 
-		v5pr_info("get: %s is builtin: %d %d %s:%s:%d\n",
-			  dp->site->modname, dp->_index, (int)(dp - dh),
+		v5pr_info("get: %s is builtin: %d %s:%s:%d\n",
+			  dh->site[dp->_index].modname, dp->_index,
 			  dh->site[dp->_index].filename,
 			  dh->site[dp->_index].function, dp->lineno);
 
@@ -165,15 +165,16 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 			 dp->_index == (dp - __start___dyndbg) &&
 			 dp->_index == (&__start___dyndbg_sites[dp->_index]
 					- &__start___dyndbg_sites[0])));
-
-		WARN_ON(&__start___dyndbg_sites[dp->_index] != dp->site);
+		if (dp->site)
+			WARN_ON(&__start___dyndbg_sites[dp->_index] != dp->site);
 	} else {
 		v4pr_info("get: %s is loaded: %d %s:%s:%d\n",
-			  dp->site->modname, dp->_index,
+			  dh->site[dp->_index].modname, dp->_index,
 			  dh->site[dp->_index].filename,
 			  dh->site[dp->_index].function, dp->lineno);
 	}
-	WARN_ON(&dh->site[dp->_index] != dp->site);
+	if (dp->site)
+		WARN_ON(&dh->site[dp->_index] != dp->site);
 
 	return dp->site;
 }
-- 
2.31.1

[RFC PATCH v5 23/28] dyndbg: dont show header records in control

[Jim Cromie]

header record pairs are special in that the filename member no longer
points at string data, but back at the data structure.

Theres no reason to expose this in control file, and no reason to even
print a line for the header, since its not part of the interface.

Printing a "# header" line is a decent alternative, but separate.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index d0477450ec0c..b83187e72ee0 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -995,7 +995,9 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
 
 	dc = ddebug_site_get(dp);
 
-	if (dc) {
+	if (dc && is_dyndbg_header_pair(dp, dc)) {
+		/* skip output on header */
+	} else if (dc) {
 		seq_printf(m, "%s:%u [%s]%s =%s \"",
 			   trim_prefix(dc->filename), dp->lineno,
 			   iter->table->mod_name, dc->function,
-- 
2.31.1

[RFC PATCH v5 24/28] dyndbg: make site pointer and checks on it optional (not quite)

[Jim Cromie]

Wrap declaration and uses of _ddebug.site pointer in ifdefs.
Mostly, this eliminates all the WARN_ONs in ddebug_site_get(),
which seem to be mostly proven (they were BUG_ONs).

This adds macro indirection around ifdef SITE_CHK; adding SITE_INIT_
to adjust the .site initializations, and SITE_COND, which abstracts a
logic test used by is_dyndbg_header_pair(i,s).

TODO: breaks when !defined SITE_CHK - the header, being just a struct
_ddebug, also lost the .site member.  Need to specialize the header
struct, then unionize it back in.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h | 20 ++++++++++++++++----
 lib/dynamic_debug.c           | 13 ++++++++++---
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 06ae1d8d8c10..46901b5348ee 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -6,6 +6,18 @@
 #include <linux/jump_label.h>
 #endif
 
+#define SITE_CHK /* include .site pointer and checks on it */
+
+#ifdef SITE_CHK
+#define SITE_CHK_(...)		__VA_ARGS__
+#define SITE_COND(_i, _s)	(_s == _i->site)
+#define SITE_INIT_(_name)	.site = &_name,
+#else
+#define SITE_CHK_(...)
+#define SITE_COND(_i, _s)	(true)
+#define SITE_INIT_(name)
+#endif
+
 /*
  * A pair of these structs are created in 2 special ELF sections
  * (__dyndbg, __dyndbg_sites) for every dynamic debug callsite.
@@ -25,7 +37,7 @@ struct _ddebug_site {
 } __aligned(8);
 
 struct _ddebug {
-	struct _ddebug_site *site;
+	SITE_CHK_(struct _ddebug_site *site;)
 	/* format is always needed, lineno shares word with flags */
 	const char *format;
 	const unsigned lineno:18;
@@ -106,7 +118,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 	};							\
 	static struct _ddebug  __aligned(8)			\
 	__section("__dyndbg") name = {				\
-		.site = &name##_site,				\
+		SITE_INIT_(name##_site)				\
 		.format = (fmt),				\
 		.lineno = __LINE__,				\
 		.flags = _DPRINTK_FLAGS_DEFAULT,		\
@@ -138,13 +150,13 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 	__weak struct _ddebug __used __aligned(8)		\
 		__section(".gnu.linkonce.dyndbg")		\
 	_LINKONCE_dyndbg_header = {				\
-		.site = &_LINKONCE_dyndbg_site_header,		\
+		SITE_INIT_(_LINKONCE_dyndbg_site_header)	\
 		.format = KBUILD_MODNAME			\
 	}
 
 /* The header initializations as a distinguishing predicate */
 #define is_dyndbg_header_pair(iter, sitep)			\
-	(sitep == iter->site					\
+	(SITE_COND(iter, sitep)					\
 	 && (!iter->lineno)					\
 	 && (iter->format == sitep->modname)			\
 	 && (sitep->modname == sitep->function)			\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index b83187e72ee0..9551c0d406b6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -151,7 +151,7 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 	struct _ddebug *dh = dp - (dp->_index);
 
 	WARN_ON(!is_dyndbg_header_pair(dh, dh->site));
-
+#ifdef SITE_CHK
 	if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
 
 		v5pr_info("get: %s is builtin: %d %s:%s:%d\n",
@@ -177,6 +177,9 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 		WARN_ON(&dh->site[dp->_index] != dp->site);
 
 	return dp->site;
+#else
+	return &dh->site[dp->_index];
+#endif
 }
 static inline void ddebug_site_put(struct _ddebug *dp)
 {
@@ -241,6 +244,7 @@ static void ddebug_alter_site(struct _ddebug *dp,
 	} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
 		static_branch_enable(&dp->key.dd_key_true);
 #endif
+#ifdef SITE_CHK
 	/* delete site info for this callsite */
 	if (modifiers->flags & _DPRINTK_FLAGS_DELETE_SITE) {
 		if (dp->site) {
@@ -249,6 +253,7 @@ static void ddebug_alter_site(struct _ddebug *dp,
 			dp->site = NULL;
 		}
 	}
+#endif
 }
 
 /*
@@ -1073,7 +1078,9 @@ static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 
 		v3pr_info(" header: %d %s.%s.%d\n", tab[0]._index, modname,
 			  tab[0].site->function, tab[0].lineno);
-		WARN_ON(tab[0].site != &sites[0]);
+
+		SITE_CHK_(BUG_ON(tab[0].site != &sites[0]));
+
 		if (numdbgs <= 1) {
 			v3pr_info(" skip header %s\n", modname);
 			return 0;
@@ -1251,7 +1258,7 @@ static int __init dynamic_debug_init(void)
 
 	for (i = 0; iter < __stop___dyndbg; iter++, site++, i++) {
 
-		WARN_ON(site != iter->site);
+		SITE_CHK_(WARN_ON(site != iter->site));
 
 		if (strcmp(modname, site->modname)) {
 			modct++;
-- 
2.31.1

[RFC PATCH v5 25/28] dyndbg: swap WARN_ON for BUG_ON see what 0-day says

[Jim Cromie]

HEAD~1 passed 0-day, Im not sure what it does with WARNs,
so make everything fatal, see what dies.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 lib/dynamic_debug.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 9551c0d406b6..b61e4a211819 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -150,7 +150,7 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 {
 	struct _ddebug *dh = dp - (dp->_index);
 
-	WARN_ON(!is_dyndbg_header_pair(dh, dh->site));
+	BUG_ON(!is_dyndbg_header_pair(dh, dh->site));
 #ifdef SITE_CHK
 	if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
 
@@ -159,14 +159,14 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 			  dh->site[dp->_index].filename,
 			  dh->site[dp->_index].function, dp->lineno);
 
-		WARN_ON(dp != &__start___dyndbg[dp->_index]);
+		BUG_ON(dp != &__start___dyndbg[dp->_index]);
 
-		WARN_ON(!(dp->_index == (dp - dh) &&
+		BUG_ON(!(dp->_index == (dp - dh) &&
 			 dp->_index == (dp - __start___dyndbg) &&
 			 dp->_index == (&__start___dyndbg_sites[dp->_index]
 					- &__start___dyndbg_sites[0])));
 		if (dp->site)
-			WARN_ON(&__start___dyndbg_sites[dp->_index] != dp->site);
+			BUG_ON(&__start___dyndbg_sites[dp->_index] != dp->site);
 	} else {
 		v4pr_info("get: %s is loaded: %d %s:%s:%d\n",
 			  dh->site[dp->_index].modname, dp->_index,
@@ -174,7 +174,7 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 			  dh->site[dp->_index].function, dp->lineno);
 	}
 	if (dp->site)
-		WARN_ON(&dh->site[dp->_index] != dp->site);
+		BUG_ON(&dh->site[dp->_index] != dp->site);
 
 	return dp->site;
 #else
@@ -1258,7 +1258,7 @@ static int __init dynamic_debug_init(void)
 
 	for (i = 0; iter < __stop___dyndbg; iter++, site++, i++) {
 
-		SITE_CHK_(WARN_ON(site != iter->site));
+		SITE_CHK_(BUG_ON(site != iter->site));
 
 		if (strcmp(modname, site->modname)) {
 			modct++;
-- 
2.31.1

[RFC PATCH v5 26/28] dyndbg: fixup protect header when deleting site

[Jim Cromie]

fix a null-ptr-deref when .site info is deleted.

 #> echo +D > /proc/dynamic_debug/control

This protects the header.site pointer from zeroing; we need it for all
the SITE_CHK sanity checking.  Probably should protect against
toggling the static_key too (in the same function), but this smaller
change fixes the crash.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 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 b61e4a211819..aa4a476d70ad 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -247,7 +247,7 @@ static void ddebug_alter_site(struct _ddebug *dp,
 #ifdef SITE_CHK
 	/* delete site info for this callsite */
 	if (modifiers->flags & _DPRINTK_FLAGS_DELETE_SITE) {
-		if (dp->site) {
+		if (dp->site && !is_dyndbg_header_pair(dp, dp->site)) {
 			vpr_info("dropping site info %s.%s.%d\n", dp->site->filename,
 				dp->site->function, dp->lineno);
 			dp->site = NULL;
-- 
2.31.1

[RFC PATCH v5 27/28] dyndbg: unionize _ddebug*_headers with struct _ddebug*

[Jim Cromie]

Up until now, to create our header record (pair), we reused struct
_ddebug(|_sites), initializing the pair with "impossible" field values
(for a pr_debug() site), which makes it recognizable as a header not a
pr-debug callsite.

Now we want to specialize the header; differentiate it from its model,
then we can drop _ddebug.site, and use header.site indirectly.

This commit has several elements combining:

Update DEFINE_DYNAMIC_DEBUG_TABLE by replacing "struct _ddebug*" with
"union _ddebug*_header".  This change places the new object types into
our 2 elf sections.

our 2 new union types have 2 jobs:

a.fit properly in the section/array.  We do this by putting the struct
  inside the union, making the unions the same size as their contained
  structs.  This was sufficient to fix a data-misalignment crash on
  2nd loop (ie 1st record after header) in _init.

b.unions need the same field defns as their models, as refd in:
  DEFINE_DYNAMIC_DEBUG_TABLE() - inits
  is_dyndbg_header_pair(i,s)   - validates that init
  use anonymous struct.

RFC: My "anonymous union/struct fu" feels incomplete.  The struct in b
must contain all the fields of its outer union's own contained struct,
maybe even in the same order (tho not obvious yet).

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 46901b5348ee..9ca413985fb2 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -36,6 +36,16 @@ struct _ddebug_site {
 	const char *function;
 } __aligned(8);
 
+/* verbatim copy of struct _ddebug_site. nothing to save here. */
+union _ddebug_site_header {
+	struct _ddebug_site __;		/* inherit footprint */
+	struct {			/* header validation */
+		const char *modname;
+		const char *filename;
+		const char *function;
+	};
+};
+
 struct _ddebug {
 	SITE_CHK_(struct _ddebug_site *site;)
 	/* format is always needed, lineno shares word with flags */
@@ -74,6 +84,21 @@ struct _ddebug {
 #endif
 } __aligned(8);
 
+/*
+ * specialized version of struct _ddebug.  The only field we NEED is
+ * .site, since keeping it here allows dropping from struct _ddebug
+ * itself.  format, lineno are used to validate header records.
+ */
+union _ddebug_header {
+	struct _ddebug __;		/* inherit footprint */
+	struct {			/* header validation */
+		union _ddebug_site_header *site;
+		const char *format;
+		const unsigned lineno:18;
+		unsigned _index:14;
+		unsigned int flags:8;
+	};
+};
 
 #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
 
@@ -136,10 +161,10 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
  */
 #define DEFINE_DYNAMIC_DEBUG_TABLE()				\
 	/* forward decl, allowing loopback pointer */		\
-	__weak struct _ddebug __used __aligned(8)		\
+	__weak union _ddebug_header __used __aligned(8)		\
 		__section(".gnu.linkonce.dyndbg")		\
 		_LINKONCE_dyndbg_header;			\
-	__weak struct _ddebug_site __used __aligned(8)		\
+	__weak union _ddebug_site_header __used __aligned(8)	\
 		__section(".gnu.linkonce.dyndbg_site")		\
 	_LINKONCE_dyndbg_site_header = {			\
 		.modname = KBUILD_MODNAME,			\
@@ -147,7 +172,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 		/* forced pointer loopback, for distinction */	\
 		.filename = (void *) &_LINKONCE_dyndbg_header	\
 	};							\
-	__weak struct _ddebug __used __aligned(8)		\
+	__weak union _ddebug_header __used __aligned(8)		\
 		__section(".gnu.linkonce.dyndbg")		\
 	_LINKONCE_dyndbg_header = {				\
 		SITE_INIT_(_LINKONCE_dyndbg_site_header)	\
-- 
2.31.1

[RFC PATCH v5 28/28] dyndbg: RFC drop _ddebug.site pointer

[Jim Cromie]

In .h, flip the switch on #define SITE_CHK.  This finally removes the
_ddebug.site pointer, and all uses of it.  Due to this, the .site
cross-link done by DEFINE_DYNAMIC_DEBUG_METADATA() is lost to the
compiler, so suppress the resulting warnings by adding a "__used" to
the _ddebug_site records initialized there.

In .c, fix a bunch of fallout:
- mostly casts from struct to/from union
- convert a BUG_ON to a WARN,
  it happens ~20x per `cat control`, probably headers on loaded modules
- fixup missing _ddebug_header.site,
  bad initialization of some sort.

This seems to work, but should be cleaner.  RFC

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h |  4 ++--
 lib/dynamic_debug.c           | 25 +++++++++++++++++--------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 9ca413985fb2..7f2b255a25ae 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -6,7 +6,7 @@
 #include <linux/jump_label.h>
 #endif
 
-#define SITE_CHK /* include .site pointer and checks on it */
+// #define SITE_CHK /* include .site pointer and checks on it */
 
 #ifdef SITE_CHK
 #define SITE_CHK_(...)		__VA_ARGS__
@@ -135,7 +135,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 			 const char *fmt, ...);
 
 #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
-	static struct _ddebug_site  __aligned(8)		\
+	static struct _ddebug_site  __aligned(8) __used		\
 	__section("__dyndbg_sites") name##_site = {		\
 		.modname = KBUILD_MODNAME,			\
 		.filename = __FILE__,				\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index aa4a476d70ad..db759c998c54 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -148,9 +148,12 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
 
 static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 {
-	struct _ddebug *dh = dp - (dp->_index);
+	union _ddebug_header *dh = (union _ddebug_header *) (dp - dp->_index);
+
+	if (!is_dyndbg_header_pair(dh, dh->site)) {
+		v3pr_info("get: header fail\n");
+	}
 
-	BUG_ON(!is_dyndbg_header_pair(dh, dh->site));
 #ifdef SITE_CHK
 	if (dp >= __start___dyndbg && dp < __stop___dyndbg) {
 
@@ -178,7 +181,7 @@ static struct _ddebug_site *ddebug_site_get(struct _ddebug *dp)
 
 	return dp->site;
 #else
-	return &dh->site[dp->_index];
+	return (struct _ddebug_site *) &dh->site[dp->_index];
 #endif
 }
 static inline void ddebug_site_put(struct _ddebug *dp)
@@ -1070,17 +1073,23 @@ static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 			       const char *modname)
 {
 	struct ddebug_table *dt;
+	union _ddebug_header *dh = (union _ddebug_header *) &tab[0];
 	int i;
 
 	v3pr_info("add-module: %s.%d sites\n", modname, numdbgs);
 
-	if (numdbgs && is_dyndbg_header_pair(tab, sites)) {
+	if (numdbgs && is_dyndbg_header_pair(dh, sites)) {
 
-		v3pr_info(" header: %d %s.%s.%d\n", tab[0]._index, modname,
-			  tab[0].site->function, tab[0].lineno);
+		v3pr_info(" header: %d %s.\n", dh->_index, modname);
 
-		SITE_CHK_(BUG_ON(tab[0].site != &sites[0]));
+		SITE_CHK_(BUG_ON(dh->site[0] != &sites[0]));
 
+		/* for !SITE_CHK, need to set site ptr, fixup missed init! */
+		if (!dh->site) {
+			dh->site = (union _ddebug_site_header *) sites;
+			v3pr_info(" header: %d %s.%s.%d\n", dh->_index, modname,
+				  dh->site->function, dh->lineno);
+		}
 		if (numdbgs <= 1) {
 			v3pr_info(" skip header %s\n", modname);
 			return 0;
@@ -1110,7 +1119,7 @@ static int __ddebug_add_module(struct _ddebug *tab, struct _ddebug_site *sites,
 	for (i = 0; i < numdbgs; i++, base++) {
 		tab[i]._index = base;
 		v3pr_info(" %d %d %s.%s.%d\n", i, base, modname,
-			  tab[i].site->function, tab[i].lineno);
+			  sites[i].function, tab[i].lineno);
 	}
 
 	mutex_lock(&ddebug_lock);
-- 
2.31.1