[PATCH v6 0/4] tracing: improve symbolic printing

[PATCH v6 0/4] tracing: improve symbolic printing

[Johannes Berg]

Hi,

Alright, this took forever, it never quite made it to the top of
my list ... Two years ago Steven reported a crash in v5, and now
I finally really looked into it and realized it was because of
section placement.


v2 was:
 - rebased on 6.9-rc1
 - always search for __print_sym() and get rid of the DYNPRINT flag
   and associated code; I think ideally we'll just remove the older
   __print_symbolic() entirely
 - use ':' as the separator instead of "//" since that makes searching
   for it much easier and it's still not a valid char in an identifier
 - fix RCU

v3:
 - fix #undef issues
 - fix drop_monitor default
 - rebase on linux-trace/for-next (there were no conflicts)
 - move net patches to 3/4
 - clarify symbol name matching logic (and remove ")" from it)

v4:
 - fix non-module build and possibly dynamic event handling

v5: (https://lore.kernel.org/all/20240614081956.19832-6-johannes@sipsolutions.net/)
 - fix build warning in non-module build

v6: (this version)
 - rebase
 - fix nit from Paolo (I hope, I don't remember changing it
   but it looks fixed!)
 - fix crash due to wrong data placement


To recap, it's annoying to have

 irq/65-iwlwifi:-401   [000]    22.790000: kfree_skb:  ...  reason: 0x20000

and much nicer to see

 irq/65-iwlwifi:-401   [000]    22.790000: kfree_skb:  ...  reason: RX_DROP_MONITOR

but this doesn't work now because __print_symbolic() can only
deal with a hard-coded list (which is actually really big.)

So here's __print_sym() which doesn't build the list into the
kernel image, but creates it at runtime. For userspace, it
will look the same as __print_symbolic() (it literally shows
__print_symbolic() to userspace) so no changes are needed,
but the actual list of values exposed to userspace in there
is built dynamically. For SKB drop reasons, this then has all
the reasons known when userspace queries the trace format.

I guess patches 3/4 should go through net-next, so not sure
how to handle this patch series. Or perhaps, as this will likely
not cause conflicts (in fact I've been rebasing it for years now),
go through tracing anyway with an Ack from netdev? But I can
also just wait for the trace patch(es) to land and resubmit the
net patches after. Assuming this looks good at all :-)

Thanks,
johannes

[PATCH v6 1/4] tracing: add __print_sym() to replace __print_symbolic()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The way __print_symbolic() works is limited and inefficient
in multiple ways:
 - you can only use it with a static list of symbols, but
   e.g. the SKB dropreasons are now a dynamic list

 - it builds the list in memory _three_ times, so it takes
   a lot of memory:
   - The print_fmt contains the list (since it's passed to
     the macro there). This actually contains the names
     _twice_, which is fixed up at runtime.
   - TRACE_DEFINE_ENUM() puts a 24-byte struct trace_eval_map
     for every entry, plus the string pointed to by it, which
     cannot be deduplicated with the strings in the print_fmt
   - The in-kernel symbolic printing creates yet another list
     of struct trace_print_flags for trace_print_symbols_seq()

 - it also requires runtime fixup during init, which is a lot
   of string parsing due to the print_fmt fixup

Introduce __print_sym() to - over time - replace the old one.
We can easily extend this also to __print_flags later, but I
cared only about the SKB dropreasons for now, which has only
__print_symbolic().

This new __print_sym() requires only a single list of items,
created by TRACE_DEFINE_SYM_LIST(), or can even use another
already existing list by using TRACE_DEFINE_SYM_FNS() with
lookup and show methods.

Then, instead of doing an init-time fixup, just do this at the
time when userspace reads the print_fmt. This way, dynamically
updated lists are possible.

For userspace, nothing actually changes, because the print_fmt
is shown exactly the same way the old __print_symbolic() was.

This adds about 4k .text in my test builds, but that'll be
more than paid for by the actual conversions.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v6: don't place data needed at runtime in .init sections,
    fixing the crash Steven reported with v1:
    https://lore.kernel.org/r/20240819182340.3bd23d67@gandalf.local.home/
    (yes, it's been over 2 years ... my bad)
---
 include/asm-generic/vmlinux.lds.h          |  3 +
 include/linux/module.h                     |  2 +
 include/linux/trace_events.h               |  7 ++
 include/linux/tracepoint.h                 | 20 +++++
 include/trace/stages/init.h                | 55 ++++++++++++
 include/trace/stages/stage2_data_offsets.h |  6 ++
 include/trace/stages/stage3_trace_output.h |  9 ++
 include/trace/stages/stage7_class_define.h |  3 +
 kernel/module/main.c                       |  3 +
 kernel/trace/trace_events.c                | 99 +++++++++++++++++++++-
 kernel/trace/trace_output.c                | 45 ++++++++++
 11 files changed, 250 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b2988aa12f66..e48327fafdfe 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -276,8 +276,10 @@
 	. = ALIGN(8);							\
 	BOUNDED_SECTION(_ftrace_events)					\
 	BOUNDED_SECTION_BY(_ftrace_eval_map, _ftrace_eval_maps)
+#define FTRACE_SYM_DEFS()	BOUNDED_SECTION(_ftrace_sym_defs)
 #else
 #define FTRACE_EVENTS()
+#define FTRACE_SYM_DEFS()
 #endif
 
 #ifdef CONFIG_TRACING
@@ -391,6 +393,7 @@
 	TRACE_PRINTKS()							\
 	BPF_RAW_TP()							\
 	TRACEPOINT_STR()						\
+	FTRACE_SYM_DEFS()						\
 	KUNIT_TABLE()
 
 /*
diff --git a/include/linux/module.h b/include/linux/module.h
index 96cc98568eea..a02833ac421b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -516,6 +516,8 @@ struct module {
 	unsigned int num_trace_events;
 	struct trace_eval_map **trace_evals;
 	unsigned int num_trace_evals;
+	struct trace_sym_def **trace_sym_defs;
+	unsigned int num_trace_sym_defs;
 #endif
 #ifdef CONFIG_DYNAMIC_FTRACE
 	unsigned int num_ftrace_callsites;
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 5cbd09c8be8d..80c34ef89feb 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -29,6 +29,13 @@ const char *trace_print_symbols_seq(struct trace_seq *p, unsigned long val,
 				    const struct trace_print_flags *symbol_array,
 				    size_t symbol_array_size);
 
+const char *trace_print_sym_seq(struct trace_seq *p, unsigned long long val,
+				const char *(*lookup)(unsigned long long val));
+const char *trace_sym_lookup(const struct trace_sym_entry *list,
+			     size_t len, unsigned long long value);
+void trace_sym_show(struct seq_file *m,
+		    const struct trace_sym_entry *list, size_t len);
+
 #if BITS_PER_LONG == 32
 const char *trace_print_flags_seq_u64(struct trace_seq *p, const char *delim,
 		      unsigned long long flags,
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index e0d838c9ce93..05c45a72756c 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -32,6 +32,24 @@ struct trace_eval_map {
 	unsigned long		eval_value;
 };
 
+struct trace_sym_def {
+	const char		*system;
+	const char		*symbol_id;
+	/* may return NULL, called under rcu_read_lock() */
+	const char *		(*lookup)(unsigned long long);
+	/*
+	 * Must print the list: ', { val, "name"}, ...'
+	 * with no trailing comma, but with the leading ', '
+	 * to simplify things:
+	 */
+	void 			(*show)(struct seq_file *);
+};
+
+struct trace_sym_entry {
+	unsigned long long	value;
+	const char		*name;
+};
+
 #define TRACEPOINT_DEFAULT_PRIO	10
 
 extern int
@@ -163,6 +181,8 @@ extern void syscall_unregfunc(void);
 
 #define TRACE_DEFINE_ENUM(x)
 #define TRACE_DEFINE_SIZEOF(x)
+#define TRACE_DEFINE_SYM_FNS(...)
+#define TRACE_DEFINE_SYM_LIST(...)
 
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
 static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
diff --git a/include/trace/stages/init.h b/include/trace/stages/init.h
index 000bcfc8dd2e..6285a03a52b2 100644
--- a/include/trace/stages/init.h
+++ b/include/trace/stages/init.h
@@ -23,6 +23,61 @@ TRACE_MAKE_SYSTEM_STR();
 	__section("_ftrace_eval_map")			\
 	*TRACE_SYSTEM##_##a = &__##TRACE_SYSTEM##_##a
 
+/*
+ * Define a symbol for __print_sym by giving lookup and
+ * show functions. See &struct trace_sym_def.
+ */
+#undef TRACE_DEFINE_SYM_FNS
+#define TRACE_DEFINE_SYM_FNS(_symbol_id, _lookup, _show)		\
+	_TRACE_DEFINE_SYM_FNS(TRACE_SYSTEM, _symbol_id, _lookup, _show)
+#define _TRACE_DEFINE_SYM_FNS(_system, _symbol_id, _lookup, _show)	\
+	__TRACE_DEFINE_SYM_FNS(_system, _symbol_id, _lookup, _show)
+#define __TRACE_DEFINE_SYM_FNS(_system, _symbol_id, _lookup, _show)	\
+	___TRACE_DEFINE_SYM_FNS(_system ## _ ## _symbol_id, _symbol_id,	\
+				_lookup, _show)
+#define ___TRACE_DEFINE_SYM_FNS(_name, _symbol_id, _lookup, _show)	\
+	static struct trace_sym_def					\
+	__trace_sym_def_ ## _name = {					\
+		.system = TRACE_SYSTEM_STRING,				\
+		.symbol_id = #_symbol_id,				\
+		.lookup = _lookup,					\
+		.show = _show,						\
+	};								\
+	static struct trace_sym_def __used				\
+	__section("_ftrace_sym_defs")					\
+	*__trace_sym_def_p_ ## _name = &__trace_sym_def_ ## _name
+
+/*
+ * Define a symbol for __print_sym by giving lookup and
+ * show functions. See &struct trace_sym_def.
+ */
+#undef TRACE_DEFINE_SYM_LIST
+#define TRACE_DEFINE_SYM_LIST(_symbol_id, ...)				\
+	_TRACE_DEFINE_SYM_LIST(TRACE_SYSTEM, _symbol_id, __VA_ARGS__)
+#define _TRACE_DEFINE_SYM_LIST(_system, _symbol_id, ...)		\
+	__TRACE_DEFINE_SYM_LIST(_system, _symbol_id, __VA_ARGS__)
+#define __TRACE_DEFINE_SYM_LIST(_system, _symbol_id, ...)		\
+	___TRACE_DEFINE_SYM_LIST(_system ## _ ## _symbol_id, _symbol_id,\
+				 __VA_ARGS__)
+#define ___TRACE_DEFINE_SYM_LIST(_name, _symbol_id, ...)		\
+	static struct trace_sym_entry					\
+	__trace_sym_list_ ## _name[] = { __VA_ARGS__ };			\
+	static const char *						\
+	__trace_sym_lookup_ ## _name(unsigned long long value)		\
+	{								\
+		return trace_sym_lookup(__trace_sym_list_ ## _name,	\
+			ARRAY_SIZE(__trace_sym_list_ ## _name), value);	\
+	}								\
+	static void							\
+	__trace_sym_show_ ## _name(struct seq_file *m)			\
+	{								\
+		trace_sym_show(m, __trace_sym_list_ ## _name,		\
+			       ARRAY_SIZE(__trace_sym_list_ ## _name));	\
+	}								\
+	___TRACE_DEFINE_SYM_FNS(_name, _symbol_id,			\
+				__trace_sym_lookup_ ## _name,		\
+				__trace_sym_show_ ## _name)
+
 #undef TRACE_DEFINE_SIZEOF
 #define TRACE_DEFINE_SIZEOF(a)				\
 	static struct trace_eval_map __used __initdata	\
diff --git a/include/trace/stages/stage2_data_offsets.h b/include/trace/stages/stage2_data_offsets.h
index 8b0cff06d346..5afd9de7deb3 100644
--- a/include/trace/stages/stage2_data_offsets.h
+++ b/include/trace/stages/stage2_data_offsets.h
@@ -5,6 +5,12 @@
 #undef TRACE_DEFINE_ENUM
 #define TRACE_DEFINE_ENUM(a)
 
+#undef TRACE_DEFINE_SYM_FNS
+#define TRACE_DEFINE_SYM_FNS(_symbol_id, _lookup, _show)
+
+#undef TRACE_DEFINE_SYM_LIST
+#define TRACE_DEFINE_SYM_LIST(_symbol_id, ...)
+
 #undef TRACE_DEFINE_SIZEOF
 #define TRACE_DEFINE_SIZEOF(a)
 
diff --git a/include/trace/stages/stage3_trace_output.h b/include/trace/stages/stage3_trace_output.h
index 181b81335781..09304259a8c1 100644
--- a/include/trace/stages/stage3_trace_output.h
+++ b/include/trace/stages/stage3_trace_output.h
@@ -79,6 +79,15 @@
 		trace_print_symbols_seq(p, value, symbols, ARRAY_SIZE(symbols));	\
 	})
 
+#undef __print_sym
+#define __print_sym(value, symbol_id)					\
+	___print_sym(TRACE_SYSTEM, value, symbol_id)
+#define ___print_sym(sys, value, symbol_id)				\
+	____print_sym(sys, value, symbol_id)
+#define ____print_sym(sys, value, symbol_id)				\
+	trace_print_sym_seq(p, value,					\
+		__trace_sym_def_p_ ## sys ## _ ## symbol_id->lookup)
+
 #undef __print_flags_u64
 #undef __print_symbolic_u64
 #if BITS_PER_LONG == 32
diff --git a/include/trace/stages/stage7_class_define.h b/include/trace/stages/stage7_class_define.h
index 47008897a795..936b9fa08cea 100644
--- a/include/trace/stages/stage7_class_define.h
+++ b/include/trace/stages/stage7_class_define.h
@@ -45,6 +45,9 @@
 #define __event_in_softirq()	(REC->common_flags & 0x10)
 #define __event_in_irq()	(REC->common_flags & 0x18)
 
+#undef __print_sym
+#define __print_sym(value, symbol_id)	__print_sym(value:symbol_id)
+
 /*
  * The below is not executed in the kernel. It is only what is
  * displayed in the print format for userspace to parse.
diff --git a/kernel/module/main.c b/kernel/module/main.c
index d0e1e0bd2ad0..8ab386a37376 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2735,6 +2735,9 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 	mod->trace_evals = section_objs(info, "_ftrace_eval_map",
 					sizeof(*mod->trace_evals),
 					&mod->num_trace_evals);
+	mod->trace_sym_defs = section_objs(info, "_ftrace_sym_defs",
+					   sizeof(*mod->trace_sym_defs),
+					   &mod->num_trace_sym_defs);
 #endif
 #ifdef CONFIG_TRACING
 	mod->trace_bprintk_fmt_start = section_objs(info, "__trace_printk_fmt",
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 1d39eaf6a0f7..abf38e3095c9 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2109,6 +2109,102 @@ static void *f_next(struct seq_file *m, void *v, loff_t *pos)
 		return node;
 }
 
+extern struct trace_sym_def *__start_ftrace_sym_defs[];
+extern struct trace_sym_def *__stop_ftrace_sym_defs[];
+
+/* note: @name is not NUL-terminated */
+static void show_sym_list(struct seq_file *m, struct trace_event_call *call,
+			  const char *name, unsigned int name_len)
+{
+	struct trace_sym_def **sym_defs;
+	unsigned int n_sym_defs, i;
+
+	if ((call->flags & TRACE_EVENT_FL_DYNAMIC) || !call->module) {
+		sym_defs = __start_ftrace_sym_defs;
+		n_sym_defs = __stop_ftrace_sym_defs - __start_ftrace_sym_defs;
+	} else {
+#ifdef CONFIG_MODULES
+		struct module *mod = call->module;
+
+		sym_defs = mod->trace_sym_defs;
+		n_sym_defs = mod->num_trace_sym_defs;
+#else
+		return;
+#endif /* CONFIG_MODULES */
+	}
+
+	for (i = 0; i < n_sym_defs; i++) {
+		unsigned int sym_len;
+
+		if (!sym_defs[i])
+			continue;
+		if (sym_defs[i]->system != call->class->system)
+			continue;
+		sym_len = strlen(sym_defs[i]->symbol_id);
+		if (name_len != sym_len)
+			continue;
+		if (strncmp(sym_defs[i]->symbol_id, name, sym_len))
+			continue;
+		if (sym_defs[i]->show)
+			sym_defs[i]->show(m);
+		break;
+	}
+}
+
+static void show_print_fmt(struct seq_file *m, struct trace_event_call *call)
+{
+	char *ptr = call->print_fmt;
+	bool in_print_sym = false;
+	int quote = 0;
+
+	seq_puts(m, "\nprint fmt: ");
+	while (*ptr) {
+		if (*ptr == '\\') {
+			seq_putc(m, *ptr);
+			ptr++;
+			/* paranoid */
+			if (!*ptr)
+				break;
+			goto next;
+		}
+		if (*ptr == '"') {
+			quote ^= 1;
+			goto next;
+		}
+		if (quote)
+			goto next;
+
+		if (in_print_sym && *ptr != ':')
+			goto next;
+
+		if (in_print_sym && *ptr == ':') {
+			const char *name;
+
+			ptr++;
+			name = ptr;
+			/* skip the name */
+			while (*ptr && *ptr != ')')
+				ptr++;
+			/* and show the actual list inline now */
+			show_sym_list(m, call, name, ptr - name);
+			in_print_sym = false;
+			continue;
+		}
+
+		if (strncmp(ptr, "__print_sym(", 12) == 0) {
+			ptr += 12;
+			seq_puts(m, "__print_symbolic(");
+			in_print_sym = true;
+			continue;
+		}
+next:
+		seq_putc(m, *ptr);
+		ptr++;
+	}
+
+	seq_putc(m, '\n');
+}
+
 static int f_show(struct seq_file *m, void *v)
 {
 	struct trace_event_file *file = event_file_data(m->private);
@@ -2128,8 +2224,7 @@ static int f_show(struct seq_file *m, void *v)
 		return 0;
 
 	case FORMAT_PRINTFMT:
-		seq_printf(m, "\nprint fmt: %s\n",
-			   call->print_fmt);
+		show_print_fmt(m, call);
 		return 0;
 	}
 
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index a5ad76175d10..0850b3ad5707 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -131,6 +131,51 @@ trace_print_symbols_seq(struct trace_seq *p, unsigned long val,
 }
 EXPORT_SYMBOL(trace_print_symbols_seq);
 
+const char *trace_sym_lookup(const struct trace_sym_entry *list,
+			     size_t len, unsigned long long value)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++) {
+		if (list[i].value == value)
+			return list[i].name;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(trace_sym_lookup);
+
+void trace_sym_show(struct seq_file *m,
+		    const struct trace_sym_entry *list, size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++)
+		seq_printf(m, ", { %lld, \"%s\" }",
+			   list[i].value, list[i].name);
+}
+EXPORT_SYMBOL(trace_sym_show);
+
+const char *
+trace_print_sym_seq(struct trace_seq *p, unsigned long long val,
+		    const char *(*lookup)(unsigned long long val))
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	const char *name;
+
+	rcu_read_lock();
+	name = lookup(val);
+	if (name)
+		trace_seq_puts(p, name);
+	else
+		trace_seq_printf(p, "0x%llx", val);
+	rcu_read_unlock();
+
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+EXPORT_SYMBOL(trace_print_sym_seq);
+
 #if BITS_PER_LONG == 32
 const char *
 trace_print_flags_seq_u64(struct trace_seq *p, const char *delim,
-- 
2.55.0

[PATCH v6 2/4] tracing/timer: use __print_sym()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Use the new __print_sym() in the timer tracing, just to show
how to convert something. This adds ~80 bytes of .text for a
saving of ~1.5K of data in my builds.

Note the format changes from

print fmt: "success=%d dependency=%s", REC->success, __print_symbolic(REC->dependency, { 0, "NONE" }, { (1 << 0), "POSIX_TIMER" }, { (1 << 1), "PERF_EVENTS" }, { (1 << 2), "SCHED" }, { (1 << 3), "CLOCK_UNSTABLE" }, { (1 << 4), "RCU" }, { (1 << 5), "RCU_EXP" })

to

print fmt: "success=%d dependency=%s", REC->success, __print_symbolic(REC->dependency, { 0, "NONE" }, { 1, "POSIX_TIMER" }, { 2, "PERF_EVENTS" }, { 4, "SCHED" }, { 8, "CLOCK_UNSTABLE" }, { 16, "RCU" }, { 32, "RCU_EXP" })

since the values are now just printed in the show function as
pure decimal values.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/trace/events/timer.h | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index ca82fd62dc30..9a43f4fedaf6 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -441,26 +441,18 @@ TRACE_EVENT(itimer_expire,
 #undef tick_dep_mask_name
 #undef tick_dep_name_end
 
-/* The MASK will convert to their bits and they need to be processed too */
-#define tick_dep_name(sdep) TRACE_DEFINE_ENUM(TICK_DEP_BIT_##sdep); \
-	TRACE_DEFINE_ENUM(TICK_DEP_MASK_##sdep);
-#define tick_dep_name_end(sdep)  TRACE_DEFINE_ENUM(TICK_DEP_BIT_##sdep); \
-	TRACE_DEFINE_ENUM(TICK_DEP_MASK_##sdep);
-/* NONE only has a mask defined for it */
-#define tick_dep_mask_name(sdep) TRACE_DEFINE_ENUM(TICK_DEP_MASK_##sdep);
+#define tick_dep_name(sdep) { TICK_DEP_MASK_##sdep, #sdep },
+#define tick_dep_mask_name(sdep) { TICK_DEP_MASK_##sdep, #sdep },
+#define tick_dep_name_end(sdep) { TICK_DEP_MASK_##sdep, #sdep }
 
-TICK_DEP_NAMES
+TRACE_DEFINE_SYM_LIST(tick_dep_names, TICK_DEP_NAMES);
 
 #undef tick_dep_name
 #undef tick_dep_mask_name
 #undef tick_dep_name_end
 
-#define tick_dep_name(sdep) { TICK_DEP_MASK_##sdep, #sdep },
-#define tick_dep_mask_name(sdep) { TICK_DEP_MASK_##sdep, #sdep },
-#define tick_dep_name_end(sdep) { TICK_DEP_MASK_##sdep, #sdep }
-
 #define show_tick_dep_name(val)				\
-	__print_symbolic(val, TICK_DEP_NAMES)
+	__print_sym(val, tick_dep_names)
 
 TRACE_EVENT(tick_stop,
 
-- 
2.55.0

[PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The __print_symbolic() could only ever print the core
drop reasons, since that's the way the infrastructure
works. Now that we have __print_sym() with all the
advantages mentioned in that commit, convert to that
and get all the drop reasons from all subsystems. As
we already have a list of them, that's really easy.

This is a little bit of .text (~100 bytes in my build)
and saves a lot of .data (~17k).

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/dropreason.h   |  5 +++++
 include/trace/events/skb.h | 13 +++---------
 net/core/skbuff.c          | 43 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/include/net/dropreason.h b/include/net/dropreason.h
index 1df60645fb27..dc4a60130c09 100644
--- a/include/net/dropreason.h
+++ b/include/net/dropreason.h
@@ -42,6 +42,11 @@ struct drop_reason_list {
 extern const struct drop_reason_list __rcu *
 drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM];
 
+#ifdef CONFIG_TRACEPOINTS
+const char *drop_reason_lookup(unsigned long long value);
+void drop_reason_show(struct seq_file *m);
+#endif
+
 void drop_reasons_register_subsys(enum skb_drop_reason_subsys subsys,
 				  const struct drop_reason_list *list);
 void drop_reasons_unregister_subsys(enum skb_drop_reason_subsys subsys);
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 2945aa7fe9a7..991bf172a6ea 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -8,15 +8,9 @@
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 #include <linux/tracepoint.h>
+#include <net/dropreason.h>
 
-#undef FN
-#define FN(reason)	TRACE_DEFINE_ENUM(SKB_DROP_REASON_##reason);
-DEFINE_DROP_REASON(FN, FN)
-
-#undef FN
-#undef FNe
-#define FN(reason)	{ SKB_DROP_REASON_##reason, #reason },
-#define FNe(reason)	{ SKB_DROP_REASON_##reason, #reason }
+TRACE_DEFINE_SYM_FNS(drop_reason, drop_reason_lookup, drop_reason_show);
 
 /*
  * Tracepoint for free an sk_buff:
@@ -47,8 +41,7 @@ TRACE_EVENT(kfree_skb,
 	TP_printk("skbaddr=%p rx_sk=%p protocol=%u location=%pS reason: %s",
 		  __entry->skbaddr, __entry->rx_sk, __entry->protocol,
 		  __entry->location,
-		  __print_symbolic(__entry->reason,
-				   DEFINE_DROP_REASON(FN, FNe)))
+		  __print_sym(__entry->reason, drop_reason))
 );
 
 #undef FN
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ab195b99c853..0c2d43797587 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -154,6 +154,49 @@ drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM] = {
 };
 EXPORT_SYMBOL(drop_reasons_by_subsys);
 
+#ifdef CONFIG_TRACEPOINTS
+const char *drop_reason_lookup(unsigned long long value)
+{
+	unsigned long long subsys_id = value >> SKB_DROP_REASON_SUBSYS_SHIFT;
+	u32 reason = value & ~SKB_DROP_REASON_SUBSYS_MASK;
+	const struct drop_reason_list *subsys;
+
+	if (subsys_id >= SKB_DROP_REASON_SUBSYS_NUM)
+		return NULL;
+
+	subsys = rcu_dereference(drop_reasons_by_subsys[subsys_id]);
+	if (!subsys)
+		return NULL;
+	if (reason >= subsys->n_reasons)
+		return NULL;
+	return subsys->reasons[reason];
+}
+
+void drop_reason_show(struct seq_file *m)
+{
+	u32 subsys_id;
+
+	rcu_read_lock();
+	for (subsys_id = 0; subsys_id < SKB_DROP_REASON_SUBSYS_NUM; subsys_id++) {
+		const struct drop_reason_list *subsys;
+		u32 i;
+
+		subsys = rcu_dereference(drop_reasons_by_subsys[subsys_id]);
+		if (!subsys)
+			continue;
+
+		for (i = 0; i < subsys->n_reasons; i++) {
+			if (!subsys->reasons[i])
+				continue;
+			seq_printf(m, ", { %u, \"%s\" }",
+				   (subsys_id << SKB_DROP_REASON_SUBSYS_SHIFT) | i,
+				   subsys->reasons[i]);
+		}
+	}
+	rcu_read_unlock();
+}
+#endif
+
 /**
  * drop_reasons_register_subsys - register another drop reason subsystem
  * @subsys: the subsystem to register, must not be the core
-- 
2.55.0

[PATCH v6 4/4] net: drop_monitor: use drop_reason_lookup()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Now that we have drop_reason_lookup(), we can just use it for
drop_monitor as well, rather than exporting the list itself.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/dropreason.h |  4 ----
 net/core/drop_monitor.c  | 20 +++++---------------
 net/core/skbuff.c        |  6 +++---
 3 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/include/net/dropreason.h b/include/net/dropreason.h
index dc4a60130c09..4680da5964ef 100644
--- a/include/net/dropreason.h
+++ b/include/net/dropreason.h
@@ -38,10 +38,6 @@ struct drop_reason_list {
 	size_t n_reasons;
 };
 
-/* Note: due to dynamic registrations, access must be under RCU */
-extern const struct drop_reason_list __rcu *
-drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM];
-
 #ifdef CONFIG_TRACEPOINTS
 const char *drop_reason_lookup(unsigned long long value);
 void drop_reason_show(struct seq_file *m);
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index abaf108ac4db..5b685f918cc2 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -612,9 +612,8 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
 				     size_t payload_len)
 {
 	struct net_dm_skb_cb *cb = NET_DM_SKB_CB(skb);
-	const struct drop_reason_list *list = NULL;
-	unsigned int subsys, subsys_reason;
 	char buf[NET_DM_MAX_SYMBOL_LEN];
+	const char *reason_str;
 	struct nlattr *attr;
 	void *hdr;
 	int rc;
@@ -632,19 +631,10 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
 		goto nla_put_failure;
 
 	rcu_read_lock();
-	subsys = u32_get_bits(cb->reason, SKB_DROP_REASON_SUBSYS_MASK);
-	if (subsys < SKB_DROP_REASON_SUBSYS_NUM)
-		list = rcu_dereference(drop_reasons_by_subsys[subsys]);
-	subsys_reason = cb->reason & ~SKB_DROP_REASON_SUBSYS_MASK;
-	if (!list ||
-	    subsys_reason >= list->n_reasons ||
-	    !list->reasons[subsys_reason] ||
-	    strlen(list->reasons[subsys_reason]) > NET_DM_MAX_REASON_LEN) {
-		list = rcu_dereference(drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_CORE]);
-		subsys_reason = SKB_DROP_REASON_NOT_SPECIFIED;
-	}
-	if (nla_put_string(msg, NET_DM_ATTR_REASON,
-			   list->reasons[subsys_reason])) {
+	reason_str = drop_reason_lookup(cb->reason);
+	if (unlikely(!reason_str))
+		reason_str = drop_reason_lookup(SKB_DROP_REASON_NOT_SPECIFIED);
+	if (nla_put_string(msg, NET_DM_ATTR_REASON, reason_str)) {
 		rcu_read_unlock();
 		goto nla_put_failure;
 	}
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 0c2d43797587..d74022457893 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -148,13 +148,11 @@ static const struct drop_reason_list drop_reasons_core = {
 	.n_reasons = ARRAY_SIZE(drop_reasons),
 };
 
-const struct drop_reason_list __rcu *
+static const struct drop_reason_list __rcu *
 drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM] = {
 	[SKB_DROP_REASON_SUBSYS_CORE] = RCU_INITIALIZER(&drop_reasons_core),
 };
-EXPORT_SYMBOL(drop_reasons_by_subsys);
 
-#ifdef CONFIG_TRACEPOINTS
 const char *drop_reason_lookup(unsigned long long value)
 {
 	unsigned long long subsys_id = value >> SKB_DROP_REASON_SUBSYS_SHIFT;
@@ -171,7 +169,9 @@ const char *drop_reason_lookup(unsigned long long value)
 		return NULL;
 	return subsys->reasons[reason];
 }
+EXPORT_SYMBOL(drop_reason_lookup);
 
+#ifdef CONFIG_TRACEPOINTS
 void drop_reason_show(struct seq_file *m)
 {
 	u32 subsys_id;
-- 
2.55.0

Re: [PATCH v6 0/4] tracing: improve symbolic printing

[Jakub Kicinski]

On Mon, 21 Sep 2026 12:06:31 +0200 Johannes Berg wrote:
> Hi,
> 
> Alright, this took forever, it never quite made it to the top of
> my list ... Two years ago Steven reported a crash in v5, and now
> I finally really looked into it and realized it was because of
> section placement.

Breaks ovs test which tries to catch the drop reasons:

https://netdev-ctrl.bots.linux.dev/logs/vmksft/net-dbg/results/833743/10-openvswitch-sh/stdout

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Matthieu Baerts]

Hi Johannes,

(+Cc openvswitch devs)

On 21/09/2026 12:06, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> The __print_symbolic() could only ever print the core
> drop reasons, since that's the way the infrastructure
> works. Now that we have __print_sym() with all the
> advantages mentioned in that commit, convert to that
> and get all the drop reasons from all subsystems. As
> we already have a list of them, that's really easy.
> 
> This is a little bit of .text (~100 bytes in my build)
> and saves a lot of .data (~17k).
Thank you for working on that! But it looks like it breaks the
openvswitch test:

https://netdev-ctrl.bots.linux.dev/logview.html?f=%2Flogs%2Fvmksft%2Fnet%2Fresults%2F833743%2F9-openvswitch-sh%2Fstdout#L168

Maybe the test needs to be adapted to get the same info differently?
(and adding CONFIG_TRACEPOINTS to the selftest config file)

Cheers,
Matt

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Ilya Maximets]

On 9/22/26 12:03 AM, Matthieu Baerts wrote:
> Hi Johannes,
> 
> (+Cc openvswitch devs)
> 
> On 21/09/2026 12:06, Johannes Berg wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>>
>> The __print_symbolic() could only ever print the core
>> drop reasons, since that's the way the infrastructure
>> works. Now that we have __print_sym() with all the
>> advantages mentioned in that commit, convert to that
>> and get all the drop reasons from all subsystems. As
>> we already have a list of them, that's really easy.
>>
>> This is a little bit of .text (~100 bytes in my build)
>> and saves a lot of .data (~17k).
> Thank you for working on that! But it looks like it breaks the
> openvswitch test:
> 
> https://netdev-ctrl.bots.linux.dev/logview.html?f=%2Flogs%2Fvmksft%2Fnet%2Fresults%2F833743%2F9-openvswitch-sh%2Fstdout#L168
> 
> Maybe the test needs to be adapted to get the same info differently?
> (and adding CONFIG_TRACEPOINTS to the selftest config file)
The parsing in the test will definitely need to be updated, i.e.,
the numbers swapped with the names of the drop reasons.

IIUC, this change only affects the printing and doesn't affect debugging
tools like retis that attempt to surface the drop reasons.  But, maybe
Adrian and Antoine (CCed) may want to have a glance as well.

Best regards, Ilya Maximets.

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Johannes Berg]

Hi,

Yeah, Jakub pointed this out too ... 

It's because the test explicitly tries to figure out the right _number_
that openvswitch will use in the drop reasons - had I not slept on this
infrastructure for two years, the test would probably have been written
a lot simpler to start with ;-)

> > https://netdev-ctrl.bots.linux.dev/logview.html?f=%2Flogs%2Fvmksft%2Fnet%2Fresults%2F833743%2F9-openvswitch-sh%2Fstdout#L168
> > 
> > Maybe the test needs to be adapted to get the same info differently?
> > (and adding CONFIG_TRACEPOINTS to the selftest config file)
> The parsing in the test will definitely need to be updated, i.e.,
> the numbers swapped with the names of the drop reasons.

Indeed. Something like this (untested right now, didn't manage to spin
up a test yet):

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index a31f7fb6882d..9b8edfcd2d1a 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -234,7 +234,7 @@ ovs_drop_reason_count()
 	local reason=$1
 
 	local perf_output=`perf script -i ${ovs_dir}/perf.data -F trace:event,trace`
-	local pattern="skb:kfree_skb:.*reason: $reason"
+	local pattern="skb:kfree_skb:.*reason: $reason$"
 
 	return `echo "$perf_output" | grep "$pattern" | wc -l`
 }
@@ -790,15 +790,6 @@ test_psample() {
 # - drop packets and verify the right drop reason is reported
 test_drop_reason() {
 	which perf >/dev/null 2>&1 || return $ksft_skip
-	which pahole >/dev/null 2>&1 || return $ksft_skip
-
-	ovs_drop_subsys=$(pahole -C skb_drop_reason_subsys |
-			      awk '/OPENVSWITCH/ { print $3; }' |
-			      tr -d ,)
-	if [ -z "$ovs_drop_subsys" ]; then
-		info "failed to get OVS drop subsys ID"
-		return $ksft_skip
-	fi
 
 	sbx_add "test_drop_reason" || return $?
 
@@ -842,7 +833,7 @@ test_drop_reason() {
 		"in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20,proto=1),icmp()" 'drop'
 
 	ovs_drop_record_and_run "test_drop_reason" ip netns exec client ping -c 2 172.31.110.20
-	ovs_drop_reason_count 0x${ovs_drop_subsys}0001 # OVS_DROP_FLOW_ACTION
+	ovs_drop_reason_count OVS_DROP_LAST_ACTION
 	if [[ "$?" -ne "2" ]]; then
 		info "Did not detect expected drops: $?"
 		return 1
@@ -859,7 +850,7 @@ test_drop_reason() {
 
 	ovs_drop_record_and_run \
             "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000
-	ovs_drop_reason_count 0x${ovs_drop_subsys}0004 # OVS_DROP_EXPLICIT_ACTION_ERROR
+	ovs_drop_reason_count OVS_DROP_EXPLICIT_WITH_ERROR
 	if [[ "$?" -ne "1" ]]; then
 		info "Did not detect expected explicit error drops: $?"
 		return 1
@@ -867,7 +858,7 @@ test_drop_reason() {
 
 	ovs_drop_record_and_run \
             "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000
-	ovs_drop_reason_count 0x${ovs_drop_subsys}0003 # OVS_DROP_EXPLICIT_ACTION
+	ovs_drop_reason_count OVS_DROP_EXPLICIT
 	if [[ "$?" -ne "1" ]]; then
 		info "Did not detect expected explicit drops: $?"
 		return 1


> IIUC, this change only affects the printing and doesn't affect debugging
> tools like retis that attempt to surface the drop reasons.  But, maybe
> Adrian and Antoine (CCed) may want to have a glance as well.

Not sure how those work? But unless it's interacting with the text
output of trace-cmd report or perf like here, it probably won't care?

johannes

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Johannes Berg]

On Tue, 2026-09-22 at 00:36 +0200, Johannes Berg wrote:
> 
> Indeed. Something like this (untested right now, didn't manage to spin
> up a test yet):
> 
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index a31f7fb6882d..9b8edfcd2d1a 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -234,7 +234,7 @@ ovs_drop_reason_count()
>  	local reason=$1
>  
>  	local perf_output=`perf script -i ${ovs_dir}/perf.data -F trace:event,trace`
> -	local pattern="skb:kfree_skb:.*reason: $reason"
> +	local pattern="skb:kfree_skb:.*reason: $reason$"
>  
>  	return `echo "$perf_output" | grep "$pattern" | wc -l`
>  }
> @@ -790,15 +790,6 @@ test_psample() {
>  # - drop packets and verify the right drop reason is reported
>  test_drop_reason() {
>  	which perf >/dev/null 2>&1 || return $ksft_skip
> -	which pahole >/dev/null 2>&1 || return $ksft_skip
> -
> -	ovs_drop_subsys=$(pahole -C skb_drop_reason_subsys |
> -			      awk '/OPENVSWITCH/ { print $3; }' |
> -			      tr -d ,)
> -	if [ -z "$ovs_drop_subsys" ]; then
> -		info "failed to get OVS drop subsys ID"
> -		return $ksft_skip
> -	fi
>  
>  	sbx_add "test_drop_reason" || return $?
>  
> @@ -842,7 +833,7 @@ test_drop_reason() {
>  		"in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20,proto=1),icmp()" 'drop'
>  
>  	ovs_drop_record_and_run "test_drop_reason" ip netns exec client ping -c 2 172.31.110.20
> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0001 # OVS_DROP_FLOW_ACTION
> +	ovs_drop_reason_count OVS_DROP_LAST_ACTION
>  	if [[ "$?" -ne "2" ]]; then
>  		info "Did not detect expected drops: $?"
>  		return 1
> @@ -859,7 +850,7 @@ test_drop_reason() {
>  
>  	ovs_drop_record_and_run \
>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000
> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0004 # OVS_DROP_EXPLICIT_ACTION_ERROR
> +	ovs_drop_reason_count OVS_DROP_EXPLICIT_WITH_ERROR
>  	if [[ "$?" -ne "1" ]]; then
>  		info "Did not detect expected explicit error drops: $?"
>  		return 1
> @@ -867,7 +858,7 @@ test_drop_reason() {
>  
>  	ovs_drop_record_and_run \
>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000
> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0003 # OVS_DROP_EXPLICIT_ACTION
> +	ovs_drop_reason_count OVS_DROP_EXPLICIT
>  	if [[ "$?" -ne "1" ]]; then
>  		info "Did not detect expected explicit drops: $?"
>  		return 1
> 

No longer untested, that works.

I might resend tomorrow, but we'll have to wait for Steven to comment on
patches 1-3 anyway.

johannes

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Antoine Tenart]

On Tue, Sep 22, 2026 at 12:23:55AM +0200, Ilya Maximets wrote:
> On 9/22/26 12:03 AM, Matthieu Baerts wrote:
> > Hi Johannes,
> > 
> > (+Cc openvswitch devs)
> > 
> > On 21/09/2026 12:06, Johannes Berg wrote:
> >> From: Johannes Berg <johannes.berg@intel.com>
> >>
> >> The __print_symbolic() could only ever print the core
> >> drop reasons, since that's the way the infrastructure
> >> works. Now that we have __print_sym() with all the
> >> advantages mentioned in that commit, convert to that
> >> and get all the drop reasons from all subsystems. As
> >> we already have a list of them, that's really easy.
> >>
> >> This is a little bit of .text (~100 bytes in my build)
> >> and saves a lot of .data (~17k).
> > Thank you for working on that! But it looks like it breaks the
> > openvswitch test:
> > 
> > https://netdev-ctrl.bots.linux.dev/logview.html?f=%2Flogs%2Fvmksft%2Fnet%2Fresults%2F833743%2F9-openvswitch-sh%2Fstdout#L168
> > 
> > Maybe the test needs to be adapted to get the same info differently?
> > (and adding CONFIG_TRACEPOINTS to the selftest config file)
> The parsing in the test will definitely need to be updated, i.e.,
> the numbers swapped with the names of the drop reasons.
> 
> IIUC, this change only affects the printing and doesn't affect debugging
> tools like retis that attempt to surface the drop reasons.  But, maybe
> Adrian and Antoine (CCed) may want to have a glance as well.

Yes, that's fine.

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Ilya Maximets]

On 9/22/26 9:18 AM, Antoine Tenart wrote:
> On Tue, Sep 22, 2026 at 12:23:55AM +0200, Ilya Maximets wrote:
>> On 9/22/26 12:03 AM, Matthieu Baerts wrote:
>>> Hi Johannes,
>>>
>>> (+Cc openvswitch devs)
>>>
>>> On 21/09/2026 12:06, Johannes Berg wrote:
>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>
>>>> The __print_symbolic() could only ever print the core
>>>> drop reasons, since that's the way the infrastructure
>>>> works. Now that we have __print_sym() with all the
>>>> advantages mentioned in that commit, convert to that
>>>> and get all the drop reasons from all subsystems. As
>>>> we already have a list of them, that's really easy.
>>>>
>>>> This is a little bit of .text (~100 bytes in my build)
>>>> and saves a lot of .data (~17k).
>>> Thank you for working on that! But it looks like it breaks the
>>> openvswitch test:
>>>
>>> https://netdev-ctrl.bots.linux.dev/logview.html?f=%2Flogs%2Fvmksft%2Fnet%2Fresults%2F833743%2F9-openvswitch-sh%2Fstdout#L168
>>>
>>> Maybe the test needs to be adapted to get the same info differently?
>>> (and adding CONFIG_TRACEPOINTS to the selftest config file)
>> The parsing in the test will definitely need to be updated, i.e.,
>> the numbers swapped with the names of the drop reasons.
>>
>> IIUC, this change only affects the printing and doesn't affect debugging
>> tools like retis that attempt to surface the drop reasons.  But, maybe
>> Adrian and Antoine (CCed) may want to have a glance as well.
> 
> Yes, that's fine.

Ack.  Good to know!

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Ilya Maximets]

On 9/22/26 1:20 AM, Johannes Berg wrote:
> On Tue, 2026-09-22 at 00:36 +0200, Johannes Berg wrote:
>>
>> Indeed. Something like this (untested right now, didn't manage to spin
>> up a test yet):
>>
>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> index a31f7fb6882d..9b8edfcd2d1a 100755
>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> @@ -234,7 +234,7 @@ ovs_drop_reason_count()
>>  	local reason=$1
>>  
>>  	local perf_output=`perf script -i ${ovs_dir}/perf.data -F trace:event,trace`
>> -	local pattern="skb:kfree_skb:.*reason: $reason"
>> +	local pattern="skb:kfree_skb:.*reason: $reason$"
>>  
>>  	return `echo "$perf_output" | grep "$pattern" | wc -l`
>>  }
>> @@ -790,15 +790,6 @@ test_psample() {
>>  # - drop packets and verify the right drop reason is reported
>>  test_drop_reason() {
>>  	which perf >/dev/null 2>&1 || return $ksft_skip
>> -	which pahole >/dev/null 2>&1 || return $ksft_skip
>> -
>> -	ovs_drop_subsys=$(pahole -C skb_drop_reason_subsys |
>> -			      awk '/OPENVSWITCH/ { print $3; }' |
>> -			      tr -d ,)
>> -	if [ -z "$ovs_drop_subsys" ]; then
>> -		info "failed to get OVS drop subsys ID"
>> -		return $ksft_skip
>> -	fi
>>  
>>  	sbx_add "test_drop_reason" || return $?
>>  
>> @@ -842,7 +833,7 @@ test_drop_reason() {
>>  		"in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20,proto=1),icmp()" 'drop'
>>  
>>  	ovs_drop_record_and_run "test_drop_reason" ip netns exec client ping -c 2 172.31.110.20
>> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0001 # OVS_DROP_FLOW_ACTION
>> +	ovs_drop_reason_count OVS_DROP_LAST_ACTION
>>  	if [[ "$?" -ne "2" ]]; then
>>  		info "Did not detect expected drops: $?"
>>  		return 1
>> @@ -859,7 +850,7 @@ test_drop_reason() {
>>  
>>  	ovs_drop_record_and_run \
>>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000
>> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0004 # OVS_DROP_EXPLICIT_ACTION_ERROR
>> +	ovs_drop_reason_count OVS_DROP_EXPLICIT_WITH_ERROR
>>  	if [[ "$?" -ne "1" ]]; then
>>  		info "Did not detect expected explicit error drops: $?"
>>  		return 1
>> @@ -867,7 +858,7 @@ test_drop_reason() {
>>  
>>  	ovs_drop_record_and_run \
>>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000
>> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0003 # OVS_DROP_EXPLICIT_ACTION
>> +	ovs_drop_reason_count OVS_DROP_EXPLICIT
>>  	if [[ "$?" -ne "1" ]]; then
>>  		info "Did not detect expected explicit drops: $?"
>>  		return 1
>>
> 
> No longer untested, that works.

Looks nicer than parsing obscure numbers indeed!

Matthieu mentioned we'll need CONFIG_TRACEPOINTS in the selftest config
shard: tools/testing/selftests/net/openvswitch/config

Is that a new dependency or was it always there we just missed adding it
to the config before?  (it's included in the common net config, so that
is probably the reason why CI doesn't fail)

Best regards, Ilya Maximets.

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Johannes Berg]

On Tue, 2026-09-22 at 11:01 +0200, Ilya Maximets wrote:
> 
> Matthieu mentioned we'll need CONFIG_TRACEPOINTS in the selftest config
> shard: tools/testing/selftests/net/openvswitch/config

Right, I was going to comment on that but then while I was writing and
testing all your emails came in and I forgot.

> Is that a new dependency or was it always there we just missed adding it
> to the config before?  (it's included in the common net config, so that
> is probably the reason why CI doesn't fail)

It can't really be new, perf to capture this was always used, I'm just
changing how the output is formatted.

Perhaps better to have that as a separate fix/cleanup?

johannes

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Adrián Moreno]

On Tue, Sep 22, 2026 at 12:23:55AM +0200, Ilya Maximets wrote:
> On 9/22/26 12:03 AM, Matthieu Baerts wrote:
> > Hi Johannes,
> >
> > (+Cc openvswitch devs)
> >
> > On 21/09/2026 12:06, Johannes Berg wrote:
> >> From: Johannes Berg <johannes.berg@intel.com>
> >>
> >> The __print_symbolic() could only ever print the core
> >> drop reasons, since that's the way the infrastructure
> >> works. Now that we have __print_sym() with all the
> >> advantages mentioned in that commit, convert to that
> >> and get all the drop reasons from all subsystems. As
> >> we already have a list of them, that's really easy.
> >>
> >> This is a little bit of .text (~100 bytes in my build)
> >> and saves a lot of .data (~17k).
> > Thank you for working on that! But it looks like it breaks the
> > openvswitch test:
> >
> > https://netdev-ctrl.bots.linux.dev/logview.html?f=%2Flogs%2Fvmksft%2Fnet%2Fresults%2F833743%2F9-openvswitch-sh%2Fstdout#L168
> >
> > Maybe the test needs to be adapted to get the same info differently?
> > (and adding CONFIG_TRACEPOINTS to the selftest config file)
> The parsing in the test will definitely need to be updated, i.e.,
> the numbers swapped with the names of the drop reasons.
>
> IIUC, this change only affects the printing and doesn't affect debugging
> tools like retis that attempt to surface the drop reasons.  But, maybe
> Adrian and Antoine (CCed) may want to have a glance as well.
>

I don't think this will affect retis in a bad way. We collect the list
of drop reasons by inspecting the BTF enums. It generally works but we
need to keep track of new subsystems enums being added so a
dynamically-generated full-list of drop reasons is interesting.

IIUC, the only way for userspace to read this list is by looking at the
event format and parse the printf line, right? Any ideas to make this
more machine-friendly?

Thanks
--
Adrián

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Adrián Moreno]

On Tue, Sep 22, 2026 at 11:01:10AM +0200, Ilya Maximets wrote:
> On 9/22/26 1:20 AM, Johannes Berg wrote:
> > On Tue, 2026-09-22 at 00:36 +0200, Johannes Berg wrote:
> >>
> >> Indeed. Something like this (untested right now, didn't manage to spin
> >> up a test yet):
> >>
> >> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> >> index a31f7fb6882d..9b8edfcd2d1a 100755
> >> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> >> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> >> @@ -234,7 +234,7 @@ ovs_drop_reason_count()
> >>  	local reason=$1
> >>
> >>  	local perf_output=`perf script -i ${ovs_dir}/perf.data -F trace:event,trace`
> >> -	local pattern="skb:kfree_skb:.*reason: $reason"
> >> +	local pattern="skb:kfree_skb:.*reason: $reason$"
> >>
> >>  	return `echo "$perf_output" | grep "$pattern" | wc -l`
> >>  }
> >> @@ -790,15 +790,6 @@ test_psample() {
> >>  # - drop packets and verify the right drop reason is reported
> >>  test_drop_reason() {
> >>  	which perf >/dev/null 2>&1 || return $ksft_skip
> >> -	which pahole >/dev/null 2>&1 || return $ksft_skip
> >> -
> >> -	ovs_drop_subsys=$(pahole -C skb_drop_reason_subsys |
> >> -			      awk '/OPENVSWITCH/ { print $3; }' |
> >> -			      tr -d ,)
> >> -	if [ -z "$ovs_drop_subsys" ]; then
> >> -		info "failed to get OVS drop subsys ID"
> >> -		return $ksft_skip
> >> -	fi
> >>
> >>  	sbx_add "test_drop_reason" || return $?
> >>
> >> @@ -842,7 +833,7 @@ test_drop_reason() {
> >>  		"in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20,proto=1),icmp()" 'drop'
> >>
> >>  	ovs_drop_record_and_run "test_drop_reason" ip netns exec client ping -c 2 172.31.110.20
> >> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0001 # OVS_DROP_FLOW_ACTION
> >> +	ovs_drop_reason_count OVS_DROP_LAST_ACTION
> >>  	if [[ "$?" -ne "2" ]]; then
> >>  		info "Did not detect expected drops: $?"
> >>  		return 1
> >> @@ -859,7 +850,7 @@ test_drop_reason() {
> >>
> >>  	ovs_drop_record_and_run \
> >>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000
> >> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0004 # OVS_DROP_EXPLICIT_ACTION_ERROR
> >> +	ovs_drop_reason_count OVS_DROP_EXPLICIT_WITH_ERROR
> >>  	if [[ "$?" -ne "1" ]]; then
> >>  		info "Did not detect expected explicit error drops: $?"
> >>  		return 1
> >> @@ -867,7 +858,7 @@ test_drop_reason() {
> >>
> >>  	ovs_drop_record_and_run \
> >>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000
> >> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0003 # OVS_DROP_EXPLICIT_ACTION
> >> +	ovs_drop_reason_count OVS_DROP_EXPLICIT
> >>  	if [[ "$?" -ne "1" ]]; then
> >>  		info "Did not detect expected explicit drops: $?"
> >>  		return 1
> >>
> >
> > No longer untested, that works.
>
> Looks nicer than parsing obscure numbers indeed!

+1, way prettier indeed.

>
> Matthieu mentioned we'll need CONFIG_TRACEPOINTS in the selftest config
> shard: tools/testing/selftests/net/openvswitch/config
>
> Is that a new dependency or was it always there we just missed adding it
> to the config before?  (it's included in the common net config, so that
> is probably the reason why CI doesn't fail)
>
> Best regards, Ilya Maximets.
>

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Johannes Berg]

On Tue, 2026-09-22 at 07:35 -0700, Adrián Moreno wrote:
> > 
> > IIUC, this change only affects the printing and doesn't affect debugging
> > tools like retis that attempt to surface the drop reasons.  But, maybe
> > Adrian and Antoine (CCed) may want to have a glance as well.
> > 
> 
> I don't think this will affect retis in a bad way. We collect the list
> of drop reasons by inspecting the BTF enums. It generally works but we
> need to keep track of new subsystems enums being added

Right, that wouldn't really change.

> so a dynamically-generated full-list of drop reasons is interesting.

> IIUC, the only way for userspace to read this list is by looking at the
> event format and parse the printf line, right? Any ideas to make this
> more machine-friendly?

It's intended to be machine friendly "enough", trace-cmd/perf (via
libtraceevent or so I think?) parse and use this? But it's also only
available when tracing is enabled.

I think if you really wanted to have a full list dynamically exposed at
runtime across all kernels, then perhaps dropmonitor or some other
netlink interface could expose it? Or maybe another debugfs/sysfs file?
I don't know what you need it for, but I think it's out of scope for
this patchset, and could even be done without it?

johannes

Re: [PATCH v6 3/4] net: dropreason: use new __print_sym() in tracing

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> On 9/22/26 1:20 AM, Johannes Berg wrote:
>> On Tue, 2026-09-22 at 00:36 +0200, Johannes Berg wrote:
>>>
>>> Indeed. Something like this (untested right now, didn't manage to spin
>>> up a test yet):
>>>
>>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>>> index a31f7fb6882d..9b8edfcd2d1a 100755
>>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>>> @@ -234,7 +234,7 @@ ovs_drop_reason_count()
>>>  	local reason=$1
>>>  
>>>  	local perf_output=`perf script -i ${ovs_dir}/perf.data -F trace:event,trace`
>>> -	local pattern="skb:kfree_skb:.*reason: $reason"
>>> +	local pattern="skb:kfree_skb:.*reason: $reason$"
>>>  
>>>  	return `echo "$perf_output" | grep "$pattern" | wc -l`
>>>  }
>>> @@ -790,15 +790,6 @@ test_psample() {
>>>  # - drop packets and verify the right drop reason is reported
>>>  test_drop_reason() {
>>>  	which perf >/dev/null 2>&1 || return $ksft_skip
>>> -	which pahole >/dev/null 2>&1 || return $ksft_skip
>>> -
>>> -	ovs_drop_subsys=$(pahole -C skb_drop_reason_subsys |
>>> -			      awk '/OPENVSWITCH/ { print $3; }' |
>>> -			      tr -d ,)
>>> -	if [ -z "$ovs_drop_subsys" ]; then
>>> -		info "failed to get OVS drop subsys ID"
>>> -		return $ksft_skip
>>> -	fi
>>>  
>>>  	sbx_add "test_drop_reason" || return $?
>>>  
>>> @@ -842,7 +833,7 @@ test_drop_reason() {
>>>  		"in_port(2),eth(),eth_type(0x0800),ipv4(src=172.31.110.20,proto=1),icmp()" 'drop'
>>>  
>>>  	ovs_drop_record_and_run "test_drop_reason" ip netns exec client ping -c 2 172.31.110.20
>>> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0001 # OVS_DROP_FLOW_ACTION
>>> +	ovs_drop_reason_count OVS_DROP_LAST_ACTION
>>>  	if [[ "$?" -ne "2" ]]; then
>>>  		info "Did not detect expected drops: $?"
>>>  		return 1
>>> @@ -859,7 +850,7 @@ test_drop_reason() {
>>>  
>>>  	ovs_drop_record_and_run \
>>>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 6000
>>> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0004 # OVS_DROP_EXPLICIT_ACTION_ERROR
>>> +	ovs_drop_reason_count OVS_DROP_EXPLICIT_WITH_ERROR
>>>  	if [[ "$?" -ne "1" ]]; then
>>>  		info "Did not detect expected explicit error drops: $?"
>>>  		return 1
>>> @@ -867,7 +858,7 @@ test_drop_reason() {
>>>  
>>>  	ovs_drop_record_and_run \
>>>              "test_drop_reason" ip netns exec client nc -i 1 -zuv 172.31.110.20 7000
>>> -	ovs_drop_reason_count 0x${ovs_drop_subsys}0003 # OVS_DROP_EXPLICIT_ACTION
>>> +	ovs_drop_reason_count OVS_DROP_EXPLICIT
>>>  	if [[ "$?" -ne "1" ]]; then
>>>  		info "Did not detect expected explicit drops: $?"
>>>  		return 1
>>>
>> 
>> No longer untested, that works.
>
> Looks nicer than parsing obscure numbers indeed!

+1 !

> Matthieu mentioned we'll need CONFIG_TRACEPOINTS in the selftest config
> shard: tools/testing/selftests/net/openvswitch/config
>
> Is that a new dependency or was it always there we just missed adding it
> to the config before?  (it's included in the common net config, so that
> is probably the reason why CI doesn't fail)

Yes, we missed it the first time around.

> Best regards, Ilya Maximets.