[PATCH v2 0/4] Tidy up symbol end fixup

[PATCH v2 0/4] Tidy up symbol end fixup

[Ian Rogers]

Fixing up more symbol ends as introduced in:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
caused perf annotate to run into memory limits - every symbol holds
all the disassembled code in the annotation, and so making symbols
ends further away dramatically increased memory usage (40MB to
 >1GB). Modify the symbol end logic so that special kernel cases aren't
applied in the common case.

v2. Drops a merged patch. Fixes a build issue with libbfd enabled.

Ian Rogers (4):
  perf symbols: Always do architecture specific fixups
  perf symbols: Add is_kernel argument to fixup end
  perf symbol: By default only fix zero length symbols
  perf symbols: More specific architecture end fixing

 tools/perf/arch/arm64/util/machine.c   | 14 +++++++++-----
 tools/perf/arch/powerpc/util/machine.c | 10 +++++++---
 tools/perf/arch/s390/util/machine.c    | 12 ++++++++----
 tools/perf/util/symbol-elf.c           |  2 +-
 tools/perf/util/symbol.c               | 16 +++++++++-------
 tools/perf/util/symbol.h               |  4 ++--
 6 files changed, 36 insertions(+), 22 deletions(-)

-- 
2.35.1.1178.g4f1659d476-goog

[PATCH v2 0/4] Tidy up symbol end fixup

[Ian Rogers]

Fixing up more symbol ends as introduced in:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
caused perf annotate to run into memory limits - every symbol holds
all the disassembled code in the annotation, and so making symbols
ends further away dramatically increased memory usage (40MB to
 >1GB). Modify the symbol end logic so that special kernel cases aren't
applied in the common case.

v2. Drops a merged patch. Fixes a build issue with libbfd enabled.

Ian Rogers (4):
  perf symbols: Always do architecture specific fixups
  perf symbols: Add is_kernel argument to fixup end
  perf symbol: By default only fix zero length symbols
  perf symbols: More specific architecture end fixing

 tools/perf/arch/arm64/util/machine.c   | 14 +++++++++-----
 tools/perf/arch/powerpc/util/machine.c | 10 +++++++---
 tools/perf/arch/s390/util/machine.c    | 12 ++++++++----
 tools/perf/util/symbol-elf.c           |  2 +-
 tools/perf/util/symbol.c               | 16 +++++++++-------
 tools/perf/util/symbol.h               |  4 ++--
 6 files changed, 36 insertions(+), 22 deletions(-)

-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] perf symbols: Always do architecture specific fixups

[Ian Rogers]

The change:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
modified the condition for architecture specific fixups motivated by a
PowerPC case. So that architectures can independently modify their
condition, move the if into the called architecture symbols__fixup_end
function and always call it.
---
 tools/perf/arch/arm64/util/machine.c   | 14 ++++++++------
 tools/perf/arch/powerpc/util/machine.c | 14 ++++++++------
 tools/perf/arch/s390/util/machine.c    | 14 ++++++++------
 tools/perf/util/symbol.c               |  6 +++---
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index d2ce31e28cd7..1cc33b323c3f 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -20,13 +20,15 @@
 
 void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
+	if (p->end == p->start || p->end != c->start) {
+		if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
 			(strchr(p->name, '[') == NULL && strchr(c->name, '[')))
-		/* Limit range of last symbol in module and kernel */
-		p->end += SYMBOL_LIMIT;
-	else
-		p->end = c->start;
-	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+			/* Limit range of last symbol in module and kernel */
+			p->end += SYMBOL_LIMIT;
+		else
+			p->end = c->start;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	}
 }
 
 void arch__add_leaf_frame_record_opts(struct record_opts *opts)
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
index e652a1aa8132..88a8abf98a57 100644
--- a/tools/perf/arch/powerpc/util/machine.c
+++ b/tools/perf/arch/powerpc/util/machine.c
@@ -16,10 +16,12 @@
 
 void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-		/* Limit the range of last kernel symbol */
-		p->end += page_size;
-	else
-		p->end = c->start;
-	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	if (p->end == p->start || p->end != c->start) {
+		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
+			/* Limit the range of last kernel symbol */
+			p->end += page_size;
+		else
+			p->end = c->start;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	}
 }
diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
index 7644a4f6d4a4..0b750738ec68 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -44,10 +44,12 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
  */
 void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-		/* Last kernel symbol mapped to end of page */
-		p->end = roundup(p->end, page_size);
-	else
-		p->end = c->start;
-	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	if (p->end == p->start || p->end != c->start) {
+		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
+			/* Last kernel symbol mapped to end of page */
+			p->end = roundup(p->end, page_size);
+		else
+			p->end = c->start;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	}
 }
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dea0fc495185..394ad493c343 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -103,7 +103,8 @@ static int prefix_underscores_count(const char *str)
 
 void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	p->end = c->start;
+	if (p->end == p->start || p->end != c->start)
+		p->end = c->start;
 }
 
 const char * __weak arch__normalize_symbol_name(const char *name)
@@ -231,8 +232,7 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
 		prev = curr;
 		curr = rb_entry(nd, struct symbol, rb_node);
 
-		if (prev->end == prev->start || prev->end != curr->start)
-			arch__symbols__fixup_end(prev, curr);
+		arch__symbols__fixup_end(prev, curr);
 	}
 
 	/* Last entry */
-- 
2.35.1.1178.g4f1659d476-goog

[PATCH v2 1/4] perf symbols: Always do architecture specific fixups

[Ian Rogers]

The change:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
modified the condition for architecture specific fixups motivated by a
PowerPC case. So that architectures can independently modify their
condition, move the if into the called architecture symbols__fixup_end
function and always call it.
---
 tools/perf/arch/arm64/util/machine.c   | 14 ++++++++------
 tools/perf/arch/powerpc/util/machine.c | 14 ++++++++------
 tools/perf/arch/s390/util/machine.c    | 14 ++++++++------
 tools/perf/util/symbol.c               |  6 +++---
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index d2ce31e28cd7..1cc33b323c3f 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -20,13 +20,15 @@
 
 void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
+	if (p->end == p->start || p->end != c->start) {
+		if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
 			(strchr(p->name, '[') == NULL && strchr(c->name, '[')))
-		/* Limit range of last symbol in module and kernel */
-		p->end += SYMBOL_LIMIT;
-	else
-		p->end = c->start;
-	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+			/* Limit range of last symbol in module and kernel */
+			p->end += SYMBOL_LIMIT;
+		else
+			p->end = c->start;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	}
 }
 
 void arch__add_leaf_frame_record_opts(struct record_opts *opts)
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
index e652a1aa8132..88a8abf98a57 100644
--- a/tools/perf/arch/powerpc/util/machine.c
+++ b/tools/perf/arch/powerpc/util/machine.c
@@ -16,10 +16,12 @@
 
 void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-		/* Limit the range of last kernel symbol */
-		p->end += page_size;
-	else
-		p->end = c->start;
-	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	if (p->end == p->start || p->end != c->start) {
+		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
+			/* Limit the range of last kernel symbol */
+			p->end += page_size;
+		else
+			p->end = c->start;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	}
 }
diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
index 7644a4f6d4a4..0b750738ec68 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -44,10 +44,12 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
  */
 void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-		/* Last kernel symbol mapped to end of page */
-		p->end = roundup(p->end, page_size);
-	else
-		p->end = c->start;
-	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	if (p->end == p->start || p->end != c->start) {
+		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
+			/* Last kernel symbol mapped to end of page */
+			p->end = roundup(p->end, page_size);
+		else
+			p->end = c->start;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	}
 }
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dea0fc495185..394ad493c343 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -103,7 +103,8 @@ static int prefix_underscores_count(const char *str)
 
 void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
 {
-	p->end = c->start;
+	if (p->end == p->start || p->end != c->start)
+		p->end = c->start;
 }
 
 const char * __weak arch__normalize_symbol_name(const char *name)
@@ -231,8 +232,7 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
 		prev = curr;
 		curr = rb_entry(nd, struct symbol, rb_node);
 
-		if (prev->end == prev->start || prev->end != curr->start)
-			arch__symbols__fixup_end(prev, curr);
+		arch__symbols__fixup_end(prev, curr);
 	}
 
 	/* Last entry */
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] perf symbols: Add is_kernel argument to fixup end

[Ian Rogers]

Kernel fixups may be special so add a flag to quickly detect if they are
necessary.
---
 tools/perf/arch/arm64/util/machine.c   |  3 ++-
 tools/perf/arch/powerpc/util/machine.c |  4 +++-
 tools/perf/arch/s390/util/machine.c    |  3 ++-
 tools/perf/util/symbol-elf.c           |  2 +-
 tools/perf/util/symbol.c               | 11 ++++++-----
 tools/perf/util/symbol.h               |  4 ++--
 6 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index 1cc33b323c3f..54fb41de9931 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -18,7 +18,8 @@
 
 #define SYMBOL_LIMIT (1 << 12) /* 4K */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+			      bool is_kernel __maybe_unused)
 {
 	if (p->end == p->start || p->end != c->start) {
 		if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
index 88a8abf98a57..a732601f951e 100644
--- a/tools/perf/arch/powerpc/util/machine.c
+++ b/tools/perf/arch/powerpc/util/machine.c
@@ -14,7 +14,9 @@
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+			      bool is_kernel __maybe_unused)
+)
 {
 	if (p->end == p->start || p->end != c->start) {
 		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
index 0b750738ec68..8622a1b78748 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -42,7 +42,8 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
  * and beginning of first module's text segment is very big.
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+			      bool is_kernel __maybe_unused)
 {
 	if (p->end == p->start || p->end != c->start) {
 		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 31cd59a2b66e..6598a5d9f223 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1290,7 +1290,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 	 * For misannotated, zeroed, ASM function sizes.
 	 */
 	if (nr > 0) {
-		symbols__fixup_end(&dso->symbols);
+		symbols__fixup_end(&dso->symbols, dso->kernel != DSO_SPACE__USER);
 		symbols__fixup_duplicate(&dso->symbols);
 		if (kmap) {
 			/*
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 394ad493c343..cc4c46563802 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -101,7 +101,8 @@ static int prefix_underscores_count(const char *str)
 	return tail - str;
 }
 
-void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+				     bool is_kernel __maybe_unused)
 {
 	if (p->end == p->start || p->end != c->start)
 		p->end = c->start;
@@ -218,7 +219,7 @@ void symbols__fixup_duplicate(struct rb_root_cached *symbols)
 	}
 }
 
-void symbols__fixup_end(struct rb_root_cached *symbols)
+void symbols__fixup_end(struct rb_root_cached *symbols, bool is_kernel)
 {
 	struct rb_node *nd, *prevnd = rb_first_cached(symbols);
 	struct symbol *curr, *prev;
@@ -232,7 +233,7 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
 		prev = curr;
 		curr = rb_entry(nd, struct symbol, rb_node);
 
-		arch__symbols__fixup_end(prev, curr);
+		arch__symbols__fixup_end(prev, curr, is_kernel);
 	}
 
 	/* Last entry */
@@ -1467,7 +1468,7 @@ int __dso__load_kallsyms(struct dso *dso, const char *filename,
 	if (kallsyms__delta(kmap, filename, &delta))
 		return -1;
 
-	symbols__fixup_end(&dso->symbols);
+	symbols__fixup_end(&dso->symbols, /*is_kernel=*/true);
 	symbols__fixup_duplicate(&dso->symbols);
 
 	if (dso->kernel == DSO_SPACE__KERNEL_GUEST)
@@ -1659,7 +1660,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
 #undef bfd_asymbol_section
 #endif
 
-	symbols__fixup_end(&dso->symbols);
+	symbols__fixup_end(&dso->symbols, dso->kernel != DSO_SPACE__USER);
 	symbols__fixup_duplicate(&dso->symbols);
 	dso->adjust_symbols = 1;
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index fbf866d82dcc..8428f24e67df 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -203,7 +203,7 @@ void __symbols__insert(struct rb_root_cached *symbols, struct symbol *sym,
 		       bool kernel);
 void symbols__insert(struct rb_root_cached *symbols, struct symbol *sym);
 void symbols__fixup_duplicate(struct rb_root_cached *symbols);
-void symbols__fixup_end(struct rb_root_cached *symbols);
+void symbols__fixup_end(struct rb_root_cached *symbols, bool is_kernel);
 void maps__fixup_end(struct maps *maps);
 
 typedef int (*mapfn_t)(u64 start, u64 len, u64 pgoff, void *data);
@@ -241,7 +241,7 @@ const char *arch__normalize_symbol_name(const char *name);
 #define SYMBOL_A 0
 #define SYMBOL_B 1
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c);
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel);
 int arch__compare_symbol_names(const char *namea, const char *nameb);
 int arch__compare_symbol_names_n(const char *namea, const char *nameb,
 				 unsigned int n);
-- 
2.35.1.1178.g4f1659d476-goog

[PATCH v2 2/4] perf symbols: Add is_kernel argument to fixup end

[Ian Rogers]

Kernel fixups may be special so add a flag to quickly detect if they are
necessary.
---
 tools/perf/arch/arm64/util/machine.c   |  3 ++-
 tools/perf/arch/powerpc/util/machine.c |  4 +++-
 tools/perf/arch/s390/util/machine.c    |  3 ++-
 tools/perf/util/symbol-elf.c           |  2 +-
 tools/perf/util/symbol.c               | 11 ++++++-----
 tools/perf/util/symbol.h               |  4 ++--
 6 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index 1cc33b323c3f..54fb41de9931 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -18,7 +18,8 @@
 
 #define SYMBOL_LIMIT (1 << 12) /* 4K */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+			      bool is_kernel __maybe_unused)
 {
 	if (p->end == p->start || p->end != c->start) {
 		if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
index 88a8abf98a57..a732601f951e 100644
--- a/tools/perf/arch/powerpc/util/machine.c
+++ b/tools/perf/arch/powerpc/util/machine.c
@@ -14,7 +14,9 @@
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+			      bool is_kernel __maybe_unused)
+)
 {
 	if (p->end == p->start || p->end != c->start) {
 		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
index 0b750738ec68..8622a1b78748 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -42,7 +42,8 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
  * and beginning of first module's text segment is very big.
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+			      bool is_kernel __maybe_unused)
 {
 	if (p->end == p->start || p->end != c->start) {
 		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 31cd59a2b66e..6598a5d9f223 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1290,7 +1290,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 	 * For misannotated, zeroed, ASM function sizes.
 	 */
 	if (nr > 0) {
-		symbols__fixup_end(&dso->symbols);
+		symbols__fixup_end(&dso->symbols, dso->kernel != DSO_SPACE__USER);
 		symbols__fixup_duplicate(&dso->symbols);
 		if (kmap) {
 			/*
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 394ad493c343..cc4c46563802 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -101,7 +101,8 @@ static int prefix_underscores_count(const char *str)
 	return tail - str;
 }
 
-void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
+void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
+				     bool is_kernel __maybe_unused)
 {
 	if (p->end == p->start || p->end != c->start)
 		p->end = c->start;
@@ -218,7 +219,7 @@ void symbols__fixup_duplicate(struct rb_root_cached *symbols)
 	}
 }
 
-void symbols__fixup_end(struct rb_root_cached *symbols)
+void symbols__fixup_end(struct rb_root_cached *symbols, bool is_kernel)
 {
 	struct rb_node *nd, *prevnd = rb_first_cached(symbols);
 	struct symbol *curr, *prev;
@@ -232,7 +233,7 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
 		prev = curr;
 		curr = rb_entry(nd, struct symbol, rb_node);
 
-		arch__symbols__fixup_end(prev, curr);
+		arch__symbols__fixup_end(prev, curr, is_kernel);
 	}
 
 	/* Last entry */
@@ -1467,7 +1468,7 @@ int __dso__load_kallsyms(struct dso *dso, const char *filename,
 	if (kallsyms__delta(kmap, filename, &delta))
 		return -1;
 
-	symbols__fixup_end(&dso->symbols);
+	symbols__fixup_end(&dso->symbols, /*is_kernel=*/true);
 	symbols__fixup_duplicate(&dso->symbols);
 
 	if (dso->kernel == DSO_SPACE__KERNEL_GUEST)
@@ -1659,7 +1660,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
 #undef bfd_asymbol_section
 #endif
 
-	symbols__fixup_end(&dso->symbols);
+	symbols__fixup_end(&dso->symbols, dso->kernel != DSO_SPACE__USER);
 	symbols__fixup_duplicate(&dso->symbols);
 	dso->adjust_symbols = 1;
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index fbf866d82dcc..8428f24e67df 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -203,7 +203,7 @@ void __symbols__insert(struct rb_root_cached *symbols, struct symbol *sym,
 		       bool kernel);
 void symbols__insert(struct rb_root_cached *symbols, struct symbol *sym);
 void symbols__fixup_duplicate(struct rb_root_cached *symbols);
-void symbols__fixup_end(struct rb_root_cached *symbols);
+void symbols__fixup_end(struct rb_root_cached *symbols, bool is_kernel);
 void maps__fixup_end(struct maps *maps);
 
 typedef int (*mapfn_t)(u64 start, u64 len, u64 pgoff, void *data);
@@ -241,7 +241,7 @@ const char *arch__normalize_symbol_name(const char *name);
 #define SYMBOL_A 0
 #define SYMBOL_B 1
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c);
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel);
 int arch__compare_symbol_names(const char *namea, const char *nameb);
 int arch__compare_symbol_names_n(const char *namea, const char *nameb,
 				 unsigned int n);
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] perf symbol: By default only fix zero length symbols

[Ian Rogers]

For architectures without a specific end fixup (ie not arm64, powerpc,
s390) only fix up the end of zero length symbols. This reverts the
behavior introduced by:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
where non-zero length symbols were expanded to the start of the current
symbol.
---
 tools/perf/util/symbol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index cc4c46563802..62163b45cf3f 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -104,7 +104,8 @@ static int prefix_underscores_count(const char *str)
 void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
 				     bool is_kernel __maybe_unused)
 {
-	if (p->end == p->start || p->end != c->start)
+	/* If the previous symbol is zero length, make its end the start of the current symbol. */
+	if (p->end == p->start)
 		p->end = c->start;
 }
 
-- 
2.35.1.1178.g4f1659d476-goog

[PATCH v2 3/4] perf symbol: By default only fix zero length symbols

[Ian Rogers]

For architectures without a specific end fixup (ie not arm64, powerpc,
s390) only fix up the end of zero length symbols. This reverts the
behavior introduced by:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
where non-zero length symbols were expanded to the start of the current
symbol.
---
 tools/perf/util/symbol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index cc4c46563802..62163b45cf3f 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -104,7 +104,8 @@ static int prefix_underscores_count(const char *str)
 void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
 				     bool is_kernel __maybe_unused)
 {
-	if (p->end == p->start || p->end != c->start)
+	/* If the previous symbol is zero length, make its end the start of the current symbol. */
+	if (p->end == p->start)
 		p->end = c->start;
 }
 
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] perf symbols: More specific architecture end fixing

[Ian Rogers]

Make the conditions used for symbol fixup closer to the comments. The
logic aims to combine the current conditions as last modified in:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
---
 tools/perf/arch/arm64/util/machine.c   | 19 ++++++++++---------
 tools/perf/arch/powerpc/util/machine.c | 18 +++++++++---------
 tools/perf/arch/s390/util/machine.c    | 17 +++++++++--------
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index 54fb41de9931..f258c1ebac03 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -18,16 +18,17 @@
 
 #define SYMBOL_LIMIT (1 << 12) /* 4K */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
-			      bool is_kernel __maybe_unused)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel)
 {
-	if (p->end == p->start || p->end != c->start) {
-		if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
-			(strchr(p->name, '[') == NULL && strchr(c->name, '[')))
-			/* Limit range of last symbol in module and kernel */
-			p->end += SYMBOL_LIMIT;
-		else
-			p->end = c->start;
+	if (is_kernel && (p->end == p->start || p->end != c->start) &&
+	    ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
+	     (strchr(p->name, '[') == NULL && strchr(c->name, '[')))) {
+		/* Limit range of last symbol in module and kernel */
+		p->end += SYMBOL_LIMIT;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	} else if (p->end == p->start) {
+		/* Expand empty symbols. */
+		p->end = c->start;
 		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
 	}
 }
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
index a732601f951e..689a48040b64 100644
--- a/tools/perf/arch/powerpc/util/machine.c
+++ b/tools/perf/arch/powerpc/util/machine.c
@@ -14,16 +14,16 @@
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
-			      bool is_kernel __maybe_unused)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel)
 )
 {
-	if (p->end == p->start || p->end != c->start) {
-		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-			/* Limit the range of last kernel symbol */
-			p->end += page_size;
-		else
-			p->end = c->start;
-		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	if (is_kernel && (p->end == p->start || p->end != c->start) &&
+	    strchr(p->name, '[') == NULL && strchr(c->name, '[')) {
+		/* Limit the range of last kernel symbol */
+		p->end += page_size;
+	} else if (p->end == p->start) {
+		/* Expand empty symbols. */
+		p->end = c->start;
 	}
+	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
 }
diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
index 8622a1b78748..790d98a39c83 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -42,15 +42,16 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
  * and beginning of first module's text segment is very big.
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
-			      bool is_kernel __maybe_unused)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel)
 {
-	if (p->end == p->start || p->end != c->start) {
-		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-			/* Last kernel symbol mapped to end of page */
-			p->end = roundup(p->end, page_size);
-		else
-			p->end = c->start;
+	if (is_kernel && (p->end == p->start || p->end != c->start) &&
+	    strchr(p->name, '[') == NULL && strchr(c->name, '[')) {
+		/* Last kernel symbol mapped to end of page */
+		p->end = roundup(p->end, page_size);
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	} else if (p->end == p->start) {
+		/* Expand empty symbols. */
+		p->end = c->start;
 		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
 	}
 }
-- 
2.35.1.1178.g4f1659d476-goog

[PATCH v2 4/4] perf symbols: More specific architecture end fixing

[Ian Rogers]

Make the conditions used for symbol fixup closer to the comments. The
logic aims to combine the current conditions as last modified in:
https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
---
 tools/perf/arch/arm64/util/machine.c   | 19 ++++++++++---------
 tools/perf/arch/powerpc/util/machine.c | 18 +++++++++---------
 tools/perf/arch/s390/util/machine.c    | 17 +++++++++--------
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index 54fb41de9931..f258c1ebac03 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -18,16 +18,17 @@
 
 #define SYMBOL_LIMIT (1 << 12) /* 4K */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
-			      bool is_kernel __maybe_unused)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel)
 {
-	if (p->end == p->start || p->end != c->start) {
-		if ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
-			(strchr(p->name, '[') == NULL && strchr(c->name, '[')))
-			/* Limit range of last symbol in module and kernel */
-			p->end += SYMBOL_LIMIT;
-		else
-			p->end = c->start;
+	if (is_kernel && (p->end == p->start || p->end != c->start) &&
+	    ((strchr(p->name, '[') && strchr(c->name, '[') == NULL) ||
+	     (strchr(p->name, '[') == NULL && strchr(c->name, '[')))) {
+		/* Limit range of last symbol in module and kernel */
+		p->end += SYMBOL_LIMIT;
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	} else if (p->end == p->start) {
+		/* Expand empty symbols. */
+		p->end = c->start;
 		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
 	}
 }
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
index a732601f951e..689a48040b64 100644
--- a/tools/perf/arch/powerpc/util/machine.c
+++ b/tools/perf/arch/powerpc/util/machine.c
@@ -14,16 +14,16 @@
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
 
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
-			      bool is_kernel __maybe_unused)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel)
 )
 {
-	if (p->end == p->start || p->end != c->start) {
-		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-			/* Limit the range of last kernel symbol */
-			p->end += page_size;
-		else
-			p->end = c->start;
-		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	if (is_kernel && (p->end == p->start || p->end != c->start) &&
+	    strchr(p->name, '[') == NULL && strchr(c->name, '[')) {
+		/* Limit the range of last kernel symbol */
+		p->end += page_size;
+	} else if (p->end == p->start) {
+		/* Expand empty symbols. */
+		p->end = c->start;
 	}
+	pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
 }
diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
index 8622a1b78748..790d98a39c83 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -42,15 +42,16 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
  * and beginning of first module's text segment is very big.
  * Therefore do not fill this gap and do not assign it to the kernel dso map.
  */
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c,
-			      bool is_kernel __maybe_unused)
+void arch__symbols__fixup_end(struct symbol *p, struct symbol *c, bool is_kernel)
 {
-	if (p->end == p->start || p->end != c->start) {
-		if (strchr(p->name, '[') == NULL && strchr(c->name, '['))
-			/* Last kernel symbol mapped to end of page */
-			p->end = roundup(p->end, page_size);
-		else
-			p->end = c->start;
+	if (is_kernel && (p->end == p->start || p->end != c->start) &&
+	    strchr(p->name, '[') == NULL && strchr(c->name, '[')) {
+		/* Last kernel symbol mapped to end of page */
+		p->end = roundup(p->end, page_size);
+		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
+	} else if (p->end == p->start) {
+		/* Expand empty symbols. */
+		p->end = c->start;
 		pr_debug4("%s sym:%s end:%#" PRIx64 "\n", __func__, p->name, p->end);
 	}
 }
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Ian Rogers]

On Tue, Apr 12, 2022 at 8:48 AM Ian Rogers <irogers@google.com> wrote:
>
> Fixing up more symbol ends as introduced in:
> https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> caused perf annotate to run into memory limits - every symbol holds
> all the disassembled code in the annotation, and so making symbols
> ends further away dramatically increased memory usage (40MB to
>  >1GB). Modify the symbol end logic so that special kernel cases aren't
> applied in the common case.
>
> v2. Drops a merged patch. Fixes a build issue with libbfd enabled.
>
> Ian Rogers (4):
>   perf symbols: Always do architecture specific fixups
>   perf symbols: Add is_kernel argument to fixup end
>   perf symbol: By default only fix zero length symbols
>   perf symbols: More specific architecture end fixing
>
>  tools/perf/arch/arm64/util/machine.c   | 14 +++++++++-----
>  tools/perf/arch/powerpc/util/machine.c | 10 +++++++---
>  tools/perf/arch/s390/util/machine.c    | 12 ++++++++----
>  tools/perf/util/symbol-elf.c           |  2 +-
>  tools/perf/util/symbol.c               | 16 +++++++++-------
>  tools/perf/util/symbol.h               |  4 ++--
>  6 files changed, 36 insertions(+), 22 deletions(-)

Missed the:

Signed-off-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> --
> 2.35.1.1178.g4f1659d476-goog
>

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Ian Rogers]

On Tue, Apr 12, 2022 at 8:48 AM Ian Rogers <irogers@google.com> wrote:
>
> Fixing up more symbol ends as introduced in:
> https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> caused perf annotate to run into memory limits - every symbol holds
> all the disassembled code in the annotation, and so making symbols
> ends further away dramatically increased memory usage (40MB to
>  >1GB). Modify the symbol end logic so that special kernel cases aren't
> applied in the common case.
>
> v2. Drops a merged patch. Fixes a build issue with libbfd enabled.
>
> Ian Rogers (4):
>   perf symbols: Always do architecture specific fixups
>   perf symbols: Add is_kernel argument to fixup end
>   perf symbol: By default only fix zero length symbols
>   perf symbols: More specific architecture end fixing
>
>  tools/perf/arch/arm64/util/machine.c   | 14 +++++++++-----
>  tools/perf/arch/powerpc/util/machine.c | 10 +++++++---
>  tools/perf/arch/s390/util/machine.c    | 12 ++++++++----
>  tools/perf/util/symbol-elf.c           |  2 +-
>  tools/perf/util/symbol.c               | 16 +++++++++-------
>  tools/perf/util/symbol.h               |  4 ++--
>  6 files changed, 36 insertions(+), 22 deletions(-)

Missed the:

Signed-off-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> --
> 2.35.1.1178.g4f1659d476-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Namhyung Kim]

Hi Ian,

On Tue, Apr 12, 2022 at 08:48:13AM -0700, Ian Rogers wrote:
> Fixing up more symbol ends as introduced in:
> https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> caused perf annotate to run into memory limits - every symbol holds
> all the disassembled code in the annotation, and so making symbols
> ends further away dramatically increased memory usage (40MB to
>  >1GB). Modify the symbol end logic so that special kernel cases aren't
> applied in the common case.
> 
> v2. Drops a merged patch. Fixes a build issue with libbfd enabled.

How about just like this?  We can get rid of arch functions as they
mostly do the same thing (kernel vs module boundary check).

Not tested. ;-)

Thanks,
Namhyung

--------------8<-------------

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dea0fc495185..df41d7266d91 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -35,6 +35,7 @@
 #include "path.h"
 #include <linux/ctype.h>
 #include <linux/zalloc.h>
+#include <internal/lib.h>  // page_size
 
 #include <elf.h>
 #include <limits.h>
@@ -231,8 +226,16 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
                prev = curr;
                curr = rb_entry(nd, struct symbol, rb_node);
 
-               if (prev->end == prev->start || prev->end != curr->start)
-                       arch__symbols__fixup_end(prev, curr);
+               if (prev->end == prev->start) {
+                       /* Last kernel symbol mapped to end of page */
+                       if (!strchr(prev->name, '[') != !strchr(curr->name, '['))
+                               prev->end = roundup(prev->end + 1, page_size);
+                       else
+                               prev->end = curr->start;
+
+                       pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
+                                 __func__, prev->name, prev->end);
+               }
        }
 
        /* Last entry */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Namhyung Kim]

Hi Ian,

On Tue, Apr 12, 2022 at 08:48:13AM -0700, Ian Rogers wrote:
> Fixing up more symbol ends as introduced in:
> https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> caused perf annotate to run into memory limits - every symbol holds
> all the disassembled code in the annotation, and so making symbols
> ends further away dramatically increased memory usage (40MB to
>  >1GB). Modify the symbol end logic so that special kernel cases aren't
> applied in the common case.
> 
> v2. Drops a merged patch. Fixes a build issue with libbfd enabled.

How about just like this?  We can get rid of arch functions as they
mostly do the same thing (kernel vs module boundary check).

Not tested. ;-)

Thanks,
Namhyung

--------------8<-------------

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dea0fc495185..df41d7266d91 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -35,6 +35,7 @@
 #include "path.h"
 #include <linux/ctype.h>
 #include <linux/zalloc.h>
+#include <internal/lib.h>  // page_size
 
 #include <elf.h>
 #include <limits.h>
@@ -231,8 +226,16 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
                prev = curr;
                curr = rb_entry(nd, struct symbol, rb_node);
 
-               if (prev->end == prev->start || prev->end != curr->start)
-                       arch__symbols__fixup_end(prev, curr);
+               if (prev->end == prev->start) {
+                       /* Last kernel symbol mapped to end of page */
+                       if (!strchr(prev->name, '[') != !strchr(curr->name, '['))
+                               prev->end = roundup(prev->end + 1, page_size);
+                       else
+                               prev->end = curr->start;
+
+                       pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
+                                 __func__, prev->name, prev->end);
+               }
        }
 
        /* Last entry */

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Ian Rogers]

On Tue, Apr 12, 2022 at 3:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, Apr 12, 2022 at 08:48:13AM -0700, Ian Rogers wrote:
> > Fixing up more symbol ends as introduced in:
> > https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> > caused perf annotate to run into memory limits - every symbol holds
> > all the disassembled code in the annotation, and so making symbols
> > ends further away dramatically increased memory usage (40MB to
> >  >1GB). Modify the symbol end logic so that special kernel cases aren't
> > applied in the common case.
> >
> > v2. Drops a merged patch. Fixes a build issue with libbfd enabled.
>
> How about just like this?  We can get rid of arch functions as they
> mostly do the same thing (kernel vs module boundary check).
>
> Not tested. ;-)
>
> Thanks,
> Namhyung
>
> --------------8<-------------
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index dea0fc495185..df41d7266d91 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -35,6 +35,7 @@
>  #include "path.h"
>  #include <linux/ctype.h>
>  #include <linux/zalloc.h>
> +#include <internal/lib.h>  // page_size
>
>  #include <elf.h>
>  #include <limits.h>
> @@ -231,8 +226,16 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
>                 prev = curr;
>                 curr = rb_entry(nd, struct symbol, rb_node);
>
> -               if (prev->end == prev->start || prev->end != curr->start)
> -                       arch__symbols__fixup_end(prev, curr);
> +               if (prev->end == prev->start) {
> +                       /* Last kernel symbol mapped to end of page */

I like the simpler logic but don't like applying this in symbol-elf
given the comment says it is kernel specific - so we could keep the
is_kernel change.

> +                       if (!strchr(prev->name, '[') != !strchr(curr->name, '['))

I find this condition not to be intention revealing. On ARM there is
also an || for the condition reversed. When this is in an is_kernel
block then I think it is clear this is kernel hack, so I think it is
good to comment on what the condition is for.

> +                               prev->end = roundup(prev->end + 1, page_size);

Currently the roundup varies per architecture, but it is not clear to
me that it matters.

> +                       else

I think we should comment here that we're extending zero sized symbols
to the start of the next symbol.

> +                               prev->end = curr->start;
> +
> +                       pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
> +                                 __func__, prev->name, prev->end);
> +               }

Thanks,
Ian

>         }
>
>         /* Last entry */

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Ian Rogers]

On Tue, Apr 12, 2022 at 3:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, Apr 12, 2022 at 08:48:13AM -0700, Ian Rogers wrote:
> > Fixing up more symbol ends as introduced in:
> > https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> > caused perf annotate to run into memory limits - every symbol holds
> > all the disassembled code in the annotation, and so making symbols
> > ends further away dramatically increased memory usage (40MB to
> >  >1GB). Modify the symbol end logic so that special kernel cases aren't
> > applied in the common case.
> >
> > v2. Drops a merged patch. Fixes a build issue with libbfd enabled.
>
> How about just like this?  We can get rid of arch functions as they
> mostly do the same thing (kernel vs module boundary check).
>
> Not tested. ;-)
>
> Thanks,
> Namhyung
>
> --------------8<-------------
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index dea0fc495185..df41d7266d91 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -35,6 +35,7 @@
>  #include "path.h"
>  #include <linux/ctype.h>
>  #include <linux/zalloc.h>
> +#include <internal/lib.h>  // page_size
>
>  #include <elf.h>
>  #include <limits.h>
> @@ -231,8 +226,16 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
>                 prev = curr;
>                 curr = rb_entry(nd, struct symbol, rb_node);
>
> -               if (prev->end == prev->start || prev->end != curr->start)
> -                       arch__symbols__fixup_end(prev, curr);
> +               if (prev->end == prev->start) {
> +                       /* Last kernel symbol mapped to end of page */

I like the simpler logic but don't like applying this in symbol-elf
given the comment says it is kernel specific - so we could keep the
is_kernel change.

> +                       if (!strchr(prev->name, '[') != !strchr(curr->name, '['))

I find this condition not to be intention revealing. On ARM there is
also an || for the condition reversed. When this is in an is_kernel
block then I think it is clear this is kernel hack, so I think it is
good to comment on what the condition is for.

> +                               prev->end = roundup(prev->end + 1, page_size);

Currently the roundup varies per architecture, but it is not clear to
me that it matters.

> +                       else

I think we should comment here that we're extending zero sized symbols
to the start of the next symbol.

> +                               prev->end = curr->start;
> +
> +                       pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
> +                                 __func__, prev->name, prev->end);
> +               }

Thanks,
Ian

>         }
>
>         /* Last entry */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Namhyung Kim]

On Tue, Apr 12, 2022 at 4:48 PM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Apr 12, 2022 at 3:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Tue, Apr 12, 2022 at 08:48:13AM -0700, Ian Rogers wrote:
> > > Fixing up more symbol ends as introduced in:
> > > https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> > > caused perf annotate to run into memory limits - every symbol holds
> > > all the disassembled code in the annotation, and so making symbols
> > > ends further away dramatically increased memory usage (40MB to
> > >  >1GB). Modify the symbol end logic so that special kernel cases aren't
> > > applied in the common case.
> > >
> > > v2. Drops a merged patch. Fixes a build issue with libbfd enabled.
> >
> > How about just like this?  We can get rid of arch functions as they
> > mostly do the same thing (kernel vs module boundary check).
> >
> > Not tested. ;-)
> >
> > Thanks,
> > Namhyung
> >
> > --------------8<-------------
> >
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index dea0fc495185..df41d7266d91 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -35,6 +35,7 @@
> >  #include "path.h"
> >  #include <linux/ctype.h>
> >  #include <linux/zalloc.h>
> > +#include <internal/lib.h>  // page_size
> >
> >  #include <elf.h>
> >  #include <limits.h>
> > @@ -231,8 +226,16 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
> >                 prev = curr;
> >                 curr = rb_entry(nd, struct symbol, rb_node);
> >
> > -               if (prev->end == prev->start || prev->end != curr->start)
> > -                       arch__symbols__fixup_end(prev, curr);
> > +               if (prev->end == prev->start) {
> > +                       /* Last kernel symbol mapped to end of page */
>
> I like the simpler logic but don't like applying this in symbol-elf
> given the comment says it is kernel specific - so we could keep the
> is_kernel change.

I'm fine with the change. :)

>
> > +                       if (!strchr(prev->name, '[') != !strchr(curr->name, '['))
>
> I find this condition not to be intention revealing. On ARM there is
> also an || for the condition reversed. When this is in an is_kernel
> block then I think it is clear this is kernel hack, so I think it is
> good to comment on what the condition is for.

Yeah, usually modules are loaded after the kernel image but
it seems ARM could load them before the kernel.
So I made the change not to call strchr() again.

But we might need to consider the special "[__builtin_kprobes]"
symbols.

>
> > +                               prev->end = roundup(prev->end + 1, page_size);
>
> Currently the roundup varies per architecture, but it is not clear to
> me that it matters.

Yeah, it would be the same as the logic for the last entry to be
more conservative.

>
> > +                       else
>
> I think we should comment here that we're extending zero sized symbols
> to the start of the next symbol.

Sounds good.

Thanks,
Namhyung


>
> > +                               prev->end = curr->start;
> > +
> > +                       pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
> > +                                 __func__, prev->name, prev->end);
> > +               }
>
> Thanks,
> Ian
>
> >         }
> >
> >         /* Last entry */

Re: [PATCH v2 0/4] Tidy up symbol end fixup

[Namhyung Kim]

On Tue, Apr 12, 2022 at 4:48 PM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Apr 12, 2022 at 3:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Tue, Apr 12, 2022 at 08:48:13AM -0700, Ian Rogers wrote:
> > > Fixing up more symbol ends as introduced in:
> > > https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> > > caused perf annotate to run into memory limits - every symbol holds
> > > all the disassembled code in the annotation, and so making symbols
> > > ends further away dramatically increased memory usage (40MB to
> > >  >1GB). Modify the symbol end logic so that special kernel cases aren't
> > > applied in the common case.
> > >
> > > v2. Drops a merged patch. Fixes a build issue with libbfd enabled.
> >
> > How about just like this?  We can get rid of arch functions as they
> > mostly do the same thing (kernel vs module boundary check).
> >
> > Not tested. ;-)
> >
> > Thanks,
> > Namhyung
> >
> > --------------8<-------------
> >
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index dea0fc495185..df41d7266d91 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -35,6 +35,7 @@
> >  #include "path.h"
> >  #include <linux/ctype.h>
> >  #include <linux/zalloc.h>
> > +#include <internal/lib.h>  // page_size
> >
> >  #include <elf.h>
> >  #include <limits.h>
> > @@ -231,8 +226,16 @@ void symbols__fixup_end(struct rb_root_cached *symbols)
> >                 prev = curr;
> >                 curr = rb_entry(nd, struct symbol, rb_node);
> >
> > -               if (prev->end == prev->start || prev->end != curr->start)
> > -                       arch__symbols__fixup_end(prev, curr);
> > +               if (prev->end == prev->start) {
> > +                       /* Last kernel symbol mapped to end of page */
>
> I like the simpler logic but don't like applying this in symbol-elf
> given the comment says it is kernel specific - so we could keep the
> is_kernel change.

I'm fine with the change. :)

>
> > +                       if (!strchr(prev->name, '[') != !strchr(curr->name, '['))
>
> I find this condition not to be intention revealing. On ARM there is
> also an || for the condition reversed. When this is in an is_kernel
> block then I think it is clear this is kernel hack, so I think it is
> good to comment on what the condition is for.

Yeah, usually modules are loaded after the kernel image but
it seems ARM could load them before the kernel.
So I made the change not to call strchr() again.

But we might need to consider the special "[__builtin_kprobes]"
symbols.

>
> > +                               prev->end = roundup(prev->end + 1, page_size);
>
> Currently the roundup varies per architecture, but it is not clear to
> me that it matters.

Yeah, it would be the same as the logic for the last entry to be
more conservative.

>
> > +                       else
>
> I think we should comment here that we're extending zero sized symbols
> to the start of the next symbol.

Sounds good.

Thanks,
Namhyung


>
> > +                               prev->end = curr->start;
> > +
> > +                       pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
> > +                                 __func__, prev->name, prev->end);
> > +               }
>
> Thanks,
> Ian
>
> >         }
> >
> >         /* Last entry */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel