tracepoints: Add warnings for unused tracepoints and trace events

Every trace event can take up to 5K of memory in text and metadata regardless if they are used or not. Trace events should not be created if they are not used. Currently there's over a hundred events in the kernel that are defined but unused, either because their callers were removed without removing the trace event with it, or a config hides the trace event caller but not the trace event itself. And in some cases, trace events were simply added but were never called for whatever reason. The number of unused trace events continues to grow.

This patch series aims to fix this.

The first patch creates a new section called __tracepoint_check, where all callers of a tracepoint creates a variable that is placed in this section with a pointer to the tracepoint they use. The scripts/sorttable.c is modified to read the __tracepoint_check section. It sorts it, and then reads the __tracepoint_ptr section that has all compiled in tracepoints. It makes sure that every tracepoint is found in the check section and if not, it prints a warning message about it. This lists the missing tracepoints at build time.

The secord patch updates sorttable to work for arm64 when compiled with gcc. As gcc's arm64 build doesn't put addresses in their section but saves them off in the RELA sections. This mostly takes the work done that was needed to do the mcount sorting at boot up on arm64.

The third patch adds EXPORT_TRACEPOINT() to the __tracepoint_check section as well. There was several locations that adds tracepoints in the kernel proper that are only used in modules. It was getting quite complex trying to move things around that I just decided to make any tracepoint in a EXPORT_TRACEPOINT "used". I'm using the analogy of static and global functions. An unused static function gets a warning but an unused global one does not.

The last patch updates the trace_ftrace_test_filter boot up self test. That selftest creates a trace event to run a bunch of filter tests on it without actually calling the tracepoint. To quiet the warning, the selftest tracepoint is called within a if (!trace_<event>_enabled()) section, where it will not be optimized out, nor will it be called.

Changes since v3: https://lore.kernel.org/linux-trace-kernel/[email protected]/

- Folded this patch with patch 2: https://lore.kernel.org/[email protected]

- Removed the runtime boot check and only have the build time check (Linus Torvalds).

git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
unused-tracepoints/core

Head SHA1: 474058274955ba3bfec3053171cfa468e4849b5e

Steven Rostedt (4):
tracing: sorttable: Add a tracepoint verification check at build time
tracing: sorttable: Find unused tracepoints for arm64 that uses reloc for address
tracepoint: Do not warn for unused event that is exported
tracing: Call trace_ftrace_test_filter() for the event

----

 include/asm-generic/vmlinux.lds.h  |   1 +
 include/linux/tracepoint.h         |  13 ++
 kernel/trace/Kconfig               |  19 ++
 kernel/trace/trace_events_filter.c |   4 +
 scripts/Makefile                   |   4 +
 scripts/sorttable.c                | 444 ++++++++++++++++++++++++++++++-------
 6 files changed, 399 insertions(+), 86 deletions(-)

From: Steven Rostedt <[email protected]>

If a tracepoint is defined via DECLARE_TRACE() or TRACE_EVENT() but never called (via the trace_<tracepoint>() function), its metadata is still around in memory and not discarded.

When created via TRACE_EVENT() the situation is worse because the TRACE_EVENT() creates metadata that can be around 5k per trace event. Having unused trace events causes several thousand of wasted bytes.

Add a verifier that injects a pointer to the tracepoint structure in the functions that are used and added to a section called __tracepoint_check. For every builtin tracepoint within the tracepoint_ptr section that is used, will have a corresponding pointer back to it in the __tracepoint_check section.

Update the sorttable code to check the tracepoint_check and tracepoint_ptr sections to see what trace events have been created but not used.

List the tracepoints that are not used at build time. Note, this currently only handles tracepoints that are builtin and not in modules.

Enabling this currently with a given config produces:

warning: tracepoint 'sched_move_numa' is unused.
warning: tracepoint 'sched_stick_numa' is unused.
warning: tracepoint 'sched_swap_numa' is unused.
warning: tracepoint 'pelt_hw_tp' is unused.
warning: tracepoint 'pelt_irq_tp' is unused.
warning: tracepoint 'rcu_preempt_task' is unused.
warning: tracepoint 'rcu_unlock_preempted_task' is unused.
warning: tracepoint 'xdp_bulk_tx' is unused.
warning: tracepoint 'xdp_redirect_map' is unused.
warning: tracepoint 'xdp_redirect_map_err' is unused.
warning: tracepoint 'vma_mas_szero' is unused.
warning: tracepoint 'vma_store' is unused.
warning: tracepoint 'hugepage_set_pmd' is unused.
warning: tracepoint 'hugepage_set_pud' is unused.
warning: tracepoint 'hugepage_update_pmd' is unused.
warning: tracepoint 'hugepage_update_pud' is unused.
warning: tracepoint 'block_rq_remap' is unused.
warning: tracepoint 'xhci_dbc_handle_event' is unused.
warning: tracepoint 'xhci_dbc_handle_transfer' is unused.
warning: tracepoint 'xhci_dbc_gadget_ep_queue' is unused.
warning: tracepoint 'xhci_dbc_alloc_request' is unused.
warning: tracepoint 'xhci_dbc_free_request' is unused.
warning: tracepoint 'xhci_dbc_queue_request' is unused.
warning: tracepoint 'xhci_dbc_giveback_request' is unused.
warning: tracepoint 'tcp_ao_wrong_maclen' is unused.
warning: tracepoint 'tcp_ao_mismatch' is unused.
warning: tracepoint 'tcp_ao_key_not_found' is unused.
warning: tracepoint 'tcp_ao_rnext_request' is unused.
warning: tracepoint 'tcp_ao_synack_no_key' is unused.
warning: tracepoint 'tcp_ao_snd_sne_update' is unused.
warning: tracepoint 'tcp_ao_rcv_sne_update' is unused.

Some of the above is totally unused but others are not used due to their "trace_" functions being inside configs, in which case, the defined tracepoints should also be inside those same configs. Others are architecture specific but defined in generic code, where they should either be moved to the architecture or be surrounded by #ifdef for the architectures they are for.

Changes since v3: https://lore.kernel.org/[email protected]

- Folded this patch with patch 2: https://lore.kernel.org/[email protected]

- Removed the runtime boot check and only have the build time check (Linus Torvalds).

---
 include/asm-generic/vmlinux.lds.h |   1 +
 include/linux/tracepoint.h        |  10 ++
 kernel/trace/Kconfig              |  19 +++
 scripts/Makefile                  |   4 +
 scripts/sorttable.c               | 268 +++++++++++++++++++++++++++---
 5 files changed, 279 insertions(+), 23 deletions(-)

index fa5f19b8d53a..600d8b51e315 100644
@@ -708,6 +708,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
 	MCOUNT_REC()							\
 	*(.init.rodata .init.rodata.*)					\
 	FTRACE_EVENTS()							\
+	BOUNDED_SECTION_BY(__tracepoint_check, ___tracepoint_check)	\
 	TRACE_SYSCALLS()						\
 	KPROBE_BLACKLIST()						\
 	ERROR_INJECT_WHITELIST()					\

index 826ce3f8e1f8..2b96c7e94c52 100644
@@ -221,6 +221,14 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		__do_trace_##name(args);				\
 	}
 
+#ifdef CONFIG_TRACEPOINT_VERIFY_USED
+# define TRACEPOINT_CHECK(name)						\
+	static void __used __section("__tracepoint_check") *__trace_check = \
+		&__tracepoint_##name;
+#else
+# define TRACEPOINT_CHECK(name)
+#endif
+
 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -270,6 +278,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto)) \
 	static inline void __do_trace_##name(proto)			\
 	{								\
+		TRACEPOINT_CHECK(name)					\
 		if (cond) {						\
 			guard(preempt_notrace)();			\
 			__DO_TRACE_CALL(name, TP_ARGS(args));		\

@@ -289,6 +298,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto)) \
 	static inline void __do_trace_##name(proto)			\
 	{								\
+		TRACEPOINT_CHECK(name)					\
 		guard(rcu_tasks_trace)();				\
 		__DO_TRACE_CALL(name, TP_ARGS(args));			\
 	}								\

index 35448f7233fe..90ffb83a43dc 100644
@@ -1050,6 +1050,25 @@ config GCOV_PROFILE_FTRACE
 	  Note that on a kernel compiled with this config, ftrace will
 	  run significantly slower.
 
+config TRACEPOINT_VERIFY_USED
+	bool
+	help
+          This option creates a section when tracepoints are used
+	  that hold a pointer to the tracepoint that is used.
+	  This can be used to test if a defined tracepoint is
+	  used or not.
+
+config TRACEPOINT_WARN_ON_UNUSED_BUILD
+	bool "Warn on build if a tracepoint is defined but not used"
+	depends on TRACEPOINTS
+	select TRACEPOINT_VERIFY_USED
+	help
+	  This option checks if every builtin defined tracepoint is
+	  used in the code. If a tracepoint is defined but not used,
+	  it will waste memory as its metadata is still created.
+	  This will cause a warning at build time if the architecture
+	  supports it.
+
 config FTRACE_SELFTEST
 	bool
 

index 46f860529df5..f81947ec9486 100644
@@ -42,6 +42,10 @@ HOSTCFLAGS_sorttable.o += -I$(srctree)/tools/arch/$(SRCARCH)/include
 HOSTCFLAGS_sorttable.o += -DUNWINDER_ORC_ENABLED
 endif
 
+ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+HOSTCFLAGS_sorttable.o += -DPREL32_RELOCATIONS
+endif
+
 ifdef CONFIG_BUILDTIME_MCOUNT_SORT
 HOSTCFLAGS_sorttable.o += -DMCOUNT_SORT_ENABLED
 endif

2.47.2

From: Steven Rostedt <[email protected]>

The addresses in the ARM64 ELF file is stored in the RELA sections similar to the mcount location table. Add support to find the addresses from the RELA section to use to get the actual addresses for checking the tracepoints and the checking variables to show if all tracepoints are used.

---
 scripts/sorttable.c | 186 ++++++++++++++++++++++++++++----------------
 1 file changed, 118 insertions(+), 68 deletions(-)

index ddcbec22ca96..edca5b06d8ce 100644
@@ -578,6 +578,106 @@ static int compare_values_32(const void *a, const void *b)
 
 static int (*compare_values)(const void *a, const void *b);
 
+static char m_err[ERRSTR_MAXSZ];
+static long rela_type;
+static bool sort_reloc;
+static int reloc_shnum;
+
+/* Fill the array with the content of the relocs */
+static int fill_relocs(void *ptr, uint64_t size, Elf_Ehdr *ehdr, uint64_t start_loc)
+{
+	Elf_Shdr *shdr_start;
+	Elf_Rela *rel;
+	unsigned int shnum;
+	unsigned int count = 0;
+	int shentsize;
+	void *array_end = ptr + size;
+
+	shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));
+	shentsize = ehdr_shentsize(ehdr);
+
+	shnum = ehdr_shnum(ehdr);
+	if (shnum == SHN_UNDEF)
+		shnum = shdr_size(shdr_start);
+
+	for (int i = 0; i < shnum; i++) {
+		Elf_Shdr *shdr = get_index(shdr_start, shentsize, i);
+		void *end;
+
+		if (shdr_type(shdr) != SHT_RELA)
+			continue;
+
+		reloc_shnum = i;
+
+		rel = (void *)ehdr + shdr_offset(shdr);
+		end = (void *)rel + shdr_size(shdr);
+
+		for (; (void *)rel < end; rel = (void *)rel + shdr_entsize(shdr)) {
+			uint64_t offset = rela_offset(rel);
+
+			if (offset >= start_loc && offset < start_loc + size) {
+				if (ptr + long_size > array_end) {
+					snprintf(m_err, ERRSTR_MAXSZ,
+						 "Too many relocations");
+					return -1;
+				}
+
+				/* Make sure this has the correct type */
+				if (rela_info(rel) != rela_type) {
+					snprintf(m_err, ERRSTR_MAXSZ,
+						"rela has type %lx but expected %lx\n",
+						(long)rela_info(rel), rela_type);
+					return -1;
+				}
+
+				if (long_size == 4)
+					*(uint32_t *)ptr = rela_addend(rel);
+				else
+					*(uint64_t *)ptr = rela_addend(rel);
+				ptr += long_size;
+				count++;
+			}
+		}
+	}
+	return count;
+}
+
+static uint64_t get_addr_reloc(Elf_Ehdr *ehdr, uint64_t addr)
+{
+	Elf_Shdr *shdr_start;
+	Elf_Shdr *shdr;
+	Elf_Rela *rel;
+	unsigned int shnum;
+	int shentsize;
+	void *end;
+
+	shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));
+	shentsize = ehdr_shentsize(ehdr);
+
+	shnum = ehdr_shnum(ehdr);
+	if (shnum == SHN_UNDEF)
+		shnum = shdr_size(shdr_start);
+
+	shdr = get_index(shdr_start, shentsize, reloc_shnum);
+	if (shdr_type(shdr) != SHT_RELA)
+		return 0;
+
+	rel = (void *)ehdr + shdr_offset(shdr);
+	end = (void *)rel + shdr_size(shdr);
+
+	for (; (void *)rel < end; rel = (void *)rel + shdr_entsize(shdr)) {
+		uint64_t offset = rela_offset(rel);
+
+		if (offset == addr) {
+			if (long_size == 4)
+				return rela_addend(rel);
+			else
+				return rela_addend(rel);
+		}
+	}
+	return 0;
+}
+
 static int fill_addrs(void *ptr, uint64_t size, void *addrs)
 {
 	void *end = ptr + size;

@@ -696,11 +796,6 @@ static int parse_symbols(const char *fname)
 }
 
 static pthread_t mcount_sort_thread;
-static bool sort_reloc;
-
-static long rela_type;
-
-static char m_err[ERRSTR_MAXSZ];
 
 struct elf_mcount_loc {
 	Elf_Ehdr *ehdr;

@@ -708,63 +803,6 @@ struct elf_mcount_loc {
 	uint64_t stop_mcount_loc;
 };
 
-/* Fill the array with the content of the relocs */
-static int fill_relocs(void *ptr, uint64_t size, Elf_Ehdr *ehdr, uint64_t start_loc)
-{
-	Elf_Shdr *shdr_start;
-	Elf_Rela *rel;
-	unsigned int shnum;
-	unsigned int count = 0;
-	int shentsize;
-	void *array_end = ptr + size;
-
-	shdr_start = (Elf_Shdr *)((char *)ehdr + ehdr_shoff(ehdr));
-	shentsize = ehdr_shentsize(ehdr);
-
-	shnum = ehdr_shnum(ehdr);
-	if (shnum == SHN_UNDEF)
-		shnum = shdr_size(shdr_start);
-
-	for (int i = 0; i < shnum; i++) {
-		Elf_Shdr *shdr = get_index(shdr_start, shentsize, i);
-		void *end;
-
-		if (shdr_type(shdr) != SHT_RELA)
-			continue;
-
-		rel = (void *)ehdr + shdr_offset(shdr);
-		end = (void *)rel + shdr_size(shdr);
-
-		for (; (void *)rel < end; rel = (void *)rel + shdr_entsize(shdr)) {
-			uint64_t offset = rela_offset(rel);
-
-			if (offset >= start_loc && offset < start_loc + size) {
-				if (ptr + long_size > array_end) {
-					snprintf(m_err, ERRSTR_MAXSZ,
-						 "Too many relocations");
-					return -1;
-				}
-
-				/* Make sure this has the correct type */
-				if (rela_info(rel) != rela_type) {
-					snprintf(m_err, ERRSTR_MAXSZ,
-						"rela has type %lx but expected %lx\n",
-						(long)rela_info(rel), rela_type);
-					return -1;
-				}
-
-				if (long_size == 4)
-					*(uint32_t *)ptr = rela_addend(rel);
-				else
-					*(uint64_t *)ptr = rela_addend(rel);
-				ptr += long_size;
-				count++;
-			}
-		}
-	}
-	return count;
-}
-
 /* Put the sorted vals back into the relocation elements */
 static void replace_relocs(void *ptr, uint64_t size, Elf_Ehdr *ehdr, uint64_t start_loc)
 {

@@ -957,7 +995,15 @@ static void make_trace_array(struct elf_tracepoint *etrace)
 		return;
 	}
 
-	count = fill_addrs(vals, size, start);
+	if (sort_reloc) {
+		count = fill_relocs(vals, size, ehdr, etrace->start_tracepoint_check);
+		/* gcc may use relocs to save the addresses, but clang does not. */
+		if (!count) {
+			count = fill_addrs(vals, size, start);
+			sort_reloc = 0;
+		}
+	} else
+		count = fill_addrs(vals, size, start);
 
 	compare_values = long_size == 4 ? compare_values_32 : compare_values_64;
 	qsort(vals, count, long_size, compare_values);

@@ -1017,10 +1063,14 @@ static int failed_event(struct elf_tracepoint *etrace, uint64_t addr)
 	if (name_ptr > file_map_end)
 		goto bad_addr;
 
-	if (long_size == 4)
-		addr = r(name_ptr);
-	else
-		addr = r8(name_ptr);
+	if (sort_reloc) {
+		addr = get_addr_reloc(ehdr, addr);
+	} else {
+		if (long_size == 4)
+			addr = r(name_ptr);
+		else
+			addr = r8(name_ptr);
+	}
 
 	sec_addr = shdr_addr(ro_data_sec);
 	sec_offset = shdr_offset(ro_data_sec);

@@ -1473,9 +1523,9 @@ static int do_file(char const *const fname, void *addr)
 
 	switch (r2(&ehdr->e32.e_machine)) {
 	case EM_AARCH64:
-#ifdef MCOUNT_SORT_ENABLED
 		sort_reloc = true;
 		rela_type = 0x403;
+#ifdef MCOUNT_SORT_ENABLED
 		/* arm64 uses patchable function entry placing before function */
 		before_func = 8;
 #endif

2.47.2

From: Steven Rostedt <[email protected]>

There are a few generic events that may only be used by modules. They are defined and then set with EXPORT_TRACEPOINT*(). Mark events that are exported as being used, even though they still waste memory in the kernel proper.

---
 include/linux/tracepoint.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 2b96c7e94c52..8026a0659580 100644
@@ -223,7 +223,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 
 #ifdef CONFIG_TRACEPOINT_VERIFY_USED
 # define TRACEPOINT_CHECK(name)						\
-	static void __used __section("__tracepoint_check") *__trace_check = \
+	static void __used __section("__tracepoint_check") *		\
+	__trace_check_##name =						\
 		&__tracepoint_##name;
 #else
 # define TRACEPOINT_CHECK(name)

@@ -381,10 +382,12 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	__DEFINE_TRACE_EXT(_name, NULL, PARAMS(_proto), PARAMS(_args));
 
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)				\
+	TRACEPOINT_CHECK(name)						\
 	EXPORT_SYMBOL_GPL(__tracepoint_##name);				\
 	EXPORT_SYMBOL_GPL(__traceiter_##name);				\
 	EXPORT_STATIC_CALL_GPL(tp_func_##name)
 #define EXPORT_TRACEPOINT_SYMBOL(name)					\
+	TRACEPOINT_CHECK(name)						\
 	EXPORT_SYMBOL(__tracepoint_##name);				\
 	EXPORT_SYMBOL(__traceiter_##name);				\
 	EXPORT_STATIC_CALL(tp_func_##name)

2.47.2

From: Steven Rostedt <[email protected]>

The trace event filter bootup self test tests a bunch of filter logic against the ftrace_test_filter event, but does not actually call the event. Work is being done to cause a warning if an event is defined but not used. To quiet the warning call the trace event under an if statement where it is disabled so it doesn't get optimized out.

---
 kernel/trace/trace_events_filter.c | 4 ++++
 1 file changed, 4 insertions(+)

index 3885aadc434d..e4581e10782b 100644
@@ -2900,6 +2900,10 @@ static __init int ftrace_test_event_filter(void)
 	if (i == DATA_CNT)
 		printk(KERN_CONT "OK\n");
 
+	/* Need to call ftrace_test_filter to prevent a warning */
+	if (!trace_ftrace_test_filter_enabled())
+		trace_ftrace_test_filter(1, 2, 3, 4, 5, 6, 7, 8);
+
 	return 0;
 }
 

2.47.2

Annoying "one step forward, two steps back" change.

You literally sent a "remove one pointless Kconfig option" patch within ten minutes of sending out this "add two pointless Kconfig options" patch.

Because as long as it's a build-time thing, please just fix the problems it points out, and it should have no real cost to being enabled.

We don't want to ask people questions that don't matter.

Of course, because you *used* to check this at run-time, you put the new "__tracepoint_check" table in a section that is actually loaded into memory, and it appears to be still there.

Just put it in the discard section, the same way we have (for example) the export_symbols table that we also check at build-time without actually ever making it part of the kernel.

Linus

You mean to also get rid of the TRACEPOINT_VERIFY_USED config?

Ah, I wasn't sure how to do that. So you are saying to do:

index 600d8b51e315..365c92e6d0ce 100644
@@ -1048,6 +1047,7 @@ defined(CONFIG_AUTOFDO_CLANG) || defined(CONFIG_PROPELLER_CLANG)
 	*(.modinfo)							\
 	/* ld.bfd warns about .gnu.version* even when not emitted */	\
 	*(.gnu.version*)						\
+	*(.__tracepoint_check)
 
 #define DISCARDS							\
 	/DISCARD/ : {							\

But this will require that I change how it's done, as it doesn't look like it's available when sorttable.c is used.

Looks like it will require a separate application to search the vmlinux.o instead of the vmlinux (which sorttable does).

I could probably take some of the sorttable.c elf parsing and move that into a header that would share the code.

-- Steve

Both of them, I think.

Hmm. Why not just make sorttable then use vmlinux.o?

No need to do it twice. Can't you just work on the original object file before re-linking?

Linus

Anyway, I'll just drop these patches for now as I don't have time to rewrite it. I'll at least put together the unused events that I've already removed and send that out for the next merge window.

-- Steve

I believe the sorttable code needs vmlinux to be finished linked, otherwise it needs to handle all the relocations in vmlinux.o

  $ readelf -r vmlinux

There are no relocations in this file.

Compared to:

   $ readelf -r vmlinux.o| wc -l
   22449337

But we are looking at moving the fix-ups of the event format files from boot time into build time. That is, currently at boot, the enum strings are converted to their numbers and some expanded tags from PAHOLE_TAG gets removed from the format files to not confuse the parsers.

The sorttable.c should be just that, code to sort Linux tables. I think adding the tracepoint check to it was a bit of a hack. Perhaps it should have its own program that focuses on tracepoint changes.

Thus it could find unused trace points and fix up the format strings. I'm not sure how bad the relocations not being resolved will affect that though.

-- Steve