linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3)
@ 2022-04-16  0:40 Namhyung Kim
  2022-04-16  0:40 ` [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end() Namhyung Kim
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Namhyung Kim @ 2022-04-16  0:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers, Michael Petlan
  Cc: Peter Zijlstra, Ingo Molnar, Masami Hiramatsu, Song Liu,
	Will Deacon, Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

Hello,

This work is a follow-up of Ian's previous one at
  https://lore.kernel.org/all/20220412154817.2728324-1-irogers@google.com/

Fixing up more symbol ends as introduced in:
  https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/

it 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 fixup logic so that special kernel cases aren't
applied in the common case.

v3 changes)
 * rename is_kernel to is_kallsyms
 * move the logic to generic function
 * remove arch-specific functions

Thanks,
Namhyung


Namhyung Kim (3):
  perf symbol: Pass is_kallsyms to symbols__fixup_end()
  perf symbol: Update symbols__fixup_end()
  perf symbol: Remove arch__symbols__fixup_end()

 tools/perf/arch/arm64/util/machine.c   | 21 ---------------
 tools/perf/arch/powerpc/util/Build     |  1 -
 tools/perf/arch/powerpc/util/machine.c | 25 -----------------
 tools/perf/arch/s390/util/machine.c    | 16 -----------
 tools/perf/util/symbol-elf.c           |  2 +-
 tools/perf/util/symbol.c               | 37 +++++++++++++++++++-------
 tools/perf/util/symbol.h               |  3 +--
 7 files changed, 29 insertions(+), 76 deletions(-)
 delete mode 100644 tools/perf/arch/powerpc/util/machine.c


base-commit: 41204da4c16071be9090940b18f566832d46becc
-- 
2.36.0.rc0.470.gd361397f0d-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end()
  2022-04-16  0:40 [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Namhyung Kim
@ 2022-04-16  0:40 ` Namhyung Kim
  2022-04-16 14:59   ` Ian Rogers
  2022-04-16  0:40 ` [PATCH 2/3] perf symbol: Update symbols__fixup_end() Namhyung Kim
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Namhyung Kim @ 2022-04-16  0:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers, Michael Petlan
  Cc: Peter Zijlstra, Ingo Molnar, Masami Hiramatsu, Song Liu,
	Will Deacon, Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

The symbol fixup is necessary for symbols in kallsyms since they don't
have size info.  So we use the next symbol's address to calculate the
size.  Now it's also used for user binaries because sometimes they
miss size for hand-written asm functions.

There's a arch-specific function to handle kallsyms differently but
currently it cannot distinguish kallsyms from others.  Pass this
information explicitly to handle it properly.  Note that those arch
functions will be moved to the generic function so I didn't added it
to the arch-functions.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol-elf.c | 2 +-
 tools/perf/util/symbol.c     | 7 ++++---
 tools/perf/util/symbol.h     | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 31cd59a2b66e..ecd377938eea 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, false);
 		symbols__fixup_duplicate(&dso->symbols);
 		if (kmap) {
 			/*
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dea0fc495185..1b85cc1422a9 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -217,7 +217,8 @@ 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_kallsyms __maybe_unused)
 {
 	struct rb_node *nd, *prevnd = rb_first_cached(symbols);
 	struct symbol *curr, *prev;
@@ -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, 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, false);
 	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..5fcdd1f94c56 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_kallsyms);
 void maps__fixup_end(struct maps *maps);
 
 typedef int (*mapfn_t)(u64 start, u64 len, u64 pgoff, void *data);
-- 
2.36.0.rc0.470.gd361397f0d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] perf symbol: Update symbols__fixup_end()
  2022-04-16  0:40 [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Namhyung Kim
  2022-04-16  0:40 ` [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end() Namhyung Kim
@ 2022-04-16  0:40 ` Namhyung Kim
  2022-04-16  0:40 ` [PATCH 3/3] perf symbol: Remove arch__symbols__fixup_end() Namhyung Kim
  2022-04-25 20:59 ` [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Ian Rogers
  3 siblings, 0 replies; 8+ messages in thread
From: Namhyung Kim @ 2022-04-16  0:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers, Michael Petlan
  Cc: Peter Zijlstra, Ingo Molnar, Masami Hiramatsu, Song Liu,
	Will Deacon, Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

Now arch-specific functions all do the same thing.  When it fixes the
symbol address it needs to check the boundary between the kernel image
and modules.  For the last symbol in the previous region, it cannot
know the exact size as it's discarded already.  Thus it just uses a
small page size (4096) and rounds it up like the last symbol.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 1b85cc1422a9..623094e866fd 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -217,8 +217,8 @@ void symbols__fixup_duplicate(struct rb_root_cached *symbols)
 	}
 }
 
-void symbols__fixup_end(struct rb_root_cached *symbols,
-			bool is_kallsyms __maybe_unused)
+/* Update zero-sized symbols using the address of the next symbol */
+void symbols__fixup_end(struct rb_root_cached *symbols, bool is_kallsyms)
 {
 	struct rb_node *nd, *prevnd = rb_first_cached(symbols);
 	struct symbol *curr, *prev;
@@ -232,8 +232,29 @@ 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);
+		/*
+		 * On some architecture kernel text segment start is located at
+		 * some low memory address, while modules are located at high
+		 * memory addresses (or vice versa).  The gap between end of
+		 * kernel text segment 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 (kallsyms).
+		 *
+		 * In kallsyms, it determines module symbols using '[' character
+		 * like in:
+		 *   ffffffffc1937000 T hdmi_driver_init  [snd_hda_codec_hdmi]
+		 */
+		if (prev->end == prev->start) {
+			/* Last kernel/module symbol mapped to end of page */
+			if (is_kallsyms && (!strchr(prev->name, '[') !=
+					    !strchr(curr->name, '[')))
+				prev->end = roundup(prev->end + 4096, 4096);
+			else
+				prev->end = curr->start;
+
+			pr_debug4("%s sym:%s end:%#" PRIx64 "\n",
+				  __func__, prev->name, prev->end);
+		}
 	}
 
 	/* Last entry */
-- 
2.36.0.rc0.470.gd361397f0d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] perf symbol: Remove arch__symbols__fixup_end()
  2022-04-16  0:40 [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Namhyung Kim
  2022-04-16  0:40 ` [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end() Namhyung Kim
  2022-04-16  0:40 ` [PATCH 2/3] perf symbol: Update symbols__fixup_end() Namhyung Kim
@ 2022-04-16  0:40 ` Namhyung Kim
  2022-04-25 20:59 ` [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Ian Rogers
  3 siblings, 0 replies; 8+ messages in thread
From: Namhyung Kim @ 2022-04-16  0:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers, Michael Petlan
  Cc: Peter Zijlstra, Ingo Molnar, Masami Hiramatsu, Song Liu,
	Will Deacon, Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

Now the generic code can handle kallsyms fixup properly so no need to
keep the arch-functions anymore.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/arch/arm64/util/machine.c   | 21 ---------------------
 tools/perf/arch/powerpc/util/Build     |  1 -
 tools/perf/arch/powerpc/util/machine.c | 25 -------------------------
 tools/perf/arch/s390/util/machine.c    | 16 ----------------
 tools/perf/util/symbol.c               |  5 -----
 tools/perf/util/symbol.h               |  1 -
 6 files changed, 69 deletions(-)
 delete mode 100644 tools/perf/arch/powerpc/util/machine.c

diff --git a/tools/perf/arch/arm64/util/machine.c b/tools/perf/arch/arm64/util/machine.c
index d2ce31e28cd7..41c1596e5207 100644
--- a/tools/perf/arch/arm64/util/machine.c
+++ b/tools/perf/arch/arm64/util/machine.c
@@ -8,27 +8,6 @@
 #include "callchain.h"
 #include "record.h"
 
-/* On arm64, kernel text segment starts at high memory address,
- * for example 0xffff 0000 8xxx xxxx. Modules start at a low memory
- * address, like 0xffff 0000 00ax xxxx. When only small amount of
- * memory is used by modules, gap between end of module's text segment
- * and start of kernel text segment may reach 2G.
- * Therefore do not fill this gap and do not assign it to the kernel dso map.
- */
-
-#define SYMBOL_LIMIT (1 << 12) /* 4K */
-
-void arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
-{
-	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);
-}
-
 void arch__add_leaf_frame_record_opts(struct record_opts *opts)
 {
 	opts->sample_user_regs |= sample_reg_masks[PERF_REG_ARM64_LR].mask;
diff --git a/tools/perf/arch/powerpc/util/Build b/tools/perf/arch/powerpc/util/Build
index 8a79c4126e5b..0115f3166568 100644
--- a/tools/perf/arch/powerpc/util/Build
+++ b/tools/perf/arch/powerpc/util/Build
@@ -1,5 +1,4 @@
 perf-y += header.o
-perf-y += machine.o
 perf-y += kvm-stat.o
 perf-y += perf_regs.o
 perf-y += mem-events.o
diff --git a/tools/perf/arch/powerpc/util/machine.c b/tools/perf/arch/powerpc/util/machine.c
deleted file mode 100644
index e652a1aa8132..000000000000
--- a/tools/perf/arch/powerpc/util/machine.c
+++ /dev/null
@@ -1,25 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <inttypes.h>
-#include <stdio.h>
-#include <string.h>
-#include <internal/lib.h> // page_size
-#include "debug.h"
-#include "symbol.h"
-
-/* On powerpc kernel text segment start at memory addresses, 0xc000000000000000
- * whereas the modules are located at very high memory addresses,
- * for example 0xc00800000xxxxxxx. The gap between end of kernel text segment
- * and beginning of first module's text segment is very high.
- * 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)
-{
-	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..98bc3f39d5f3 100644
--- a/tools/perf/arch/s390/util/machine.c
+++ b/tools/perf/arch/s390/util/machine.c
@@ -35,19 +35,3 @@ int arch__fix_module_text_start(u64 *start, u64 *size, const char *name)
 
 	return 0;
 }
-
-/* On s390 kernel text segment start is located at very low memory addresses,
- * for example 0x10000. Modules are located at very high memory addresses,
- * for example 0x3ff xxxx xxxx. The gap between end of kernel text segment
- * 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)
-{
-	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 623094e866fd..f72baf636724 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -101,11 +101,6 @@ static int prefix_underscores_count(const char *str)
 	return tail - str;
 }
 
-void __weak arch__symbols__fixup_end(struct symbol *p, struct symbol *c)
-{
-	p->end = c->start;
-}
-
 const char * __weak arch__normalize_symbol_name(const char *name)
 {
 	return name;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 5fcdd1f94c56..0b893dcc8ea6 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -241,7 +241,6 @@ 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);
 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.36.0.rc0.470.gd361397f0d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end()
  2022-04-16  0:40 ` [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end() Namhyung Kim
@ 2022-04-16 14:59   ` Ian Rogers
  2022-04-25 19:14     ` Namhyung Kim
  0 siblings, 1 reply; 8+ messages in thread
From: Ian Rogers @ 2022-04-16 14:59 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Michael Petlan,
	Peter Zijlstra, Ingo Molnar, Masami Hiramatsu, Song Liu,
	Will Deacon, Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

On Fri, Apr 15, 2022 at 8:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The symbol fixup is necessary for symbols in kallsyms since they don't
> have size info.  So we use the next symbol's address to calculate the
> size.  Now it's also used for user binaries because sometimes they
> miss size for hand-written asm functions.
>
> There's a arch-specific function to handle kallsyms differently but
> currently it cannot distinguish kallsyms from others.  Pass this
> information explicitly to handle it properly.  Note that those arch
> functions will be moved to the generic function so I didn't added it
> to the arch-functions.

Thanks Namhyung, in:
https://lore.kernel.org/linux-perf-users/20220412154817.2728324-3-irogers@google.com/
I used "dso->kernel != DSO_SPACE__USER" in symbol-elf to make this
more than just kallsyms as presumably kernel code is the issue. Do we
know elf kernel code has correctly sized symbols?

Thanks,
Ian

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/symbol-elf.c | 2 +-
>  tools/perf/util/symbol.c     | 7 ++++---
>  tools/perf/util/symbol.h     | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 31cd59a2b66e..ecd377938eea 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, false);
>                 symbols__fixup_duplicate(&dso->symbols);
>                 if (kmap) {
>                         /*
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index dea0fc495185..1b85cc1422a9 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -217,7 +217,8 @@ 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_kallsyms __maybe_unused)
>  {
>         struct rb_node *nd, *prevnd = rb_first_cached(symbols);
>         struct symbol *curr, *prev;
> @@ -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, 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, false);
>         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..5fcdd1f94c56 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_kallsyms);
>  void maps__fixup_end(struct maps *maps);
>
>  typedef int (*mapfn_t)(u64 start, u64 len, u64 pgoff, void *data);
> --
> 2.36.0.rc0.470.gd361397f0d-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end()
  2022-04-16 14:59   ` Ian Rogers
@ 2022-04-25 19:14     ` Namhyung Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Namhyung Kim @ 2022-04-25 19:14 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Michael Petlan,
	Peter Zijlstra, Ingo Molnar, Song Liu, Will Deacon, Mark Rutland,
	John Garry, Mathieu Poirier, Leo Yan, Michael Ellerman,
	Heiko Carstens, LKML, linux-perf-users, linuxppc-dev, linux-s390

Hi Ian,

On Sat, Apr 16, 2022 at 7:59 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Apr 15, 2022 at 8:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The symbol fixup is necessary for symbols in kallsyms since they don't
> > have size info.  So we use the next symbol's address to calculate the
> > size.  Now it's also used for user binaries because sometimes they
> > miss size for hand-written asm functions.
> >
> > There's a arch-specific function to handle kallsyms differently but
> > currently it cannot distinguish kallsyms from others.  Pass this
> > information explicitly to handle it properly.  Note that those arch
> > functions will be moved to the generic function so I didn't added it
> > to the arch-functions.
>
> Thanks Namhyung, in:
> https://lore.kernel.org/linux-perf-users/20220412154817.2728324-3-irogers@google.com/
> I used "dso->kernel != DSO_SPACE__USER" in symbol-elf to make this
> more than just kallsyms as presumably kernel code is the issue. Do we
> know elf kernel code has correctly sized symbols?

Yeah, IIUC the whole point of the symbol end fixup is because the
kallsyms doesn't have the symbol size info.  Every ELF binaries
should have the size except for some hand-written asm functions
which missed adding it manually.  I guess that's the reason it was
added to other DSO loading paths.

Also considering "[" (and "]") in the symbol name is specific to
kallsyms which has both kernel and module symbols together.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3)
  2022-04-16  0:40 [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Namhyung Kim
                   ` (2 preceding siblings ...)
  2022-04-16  0:40 ` [PATCH 3/3] perf symbol: Remove arch__symbols__fixup_end() Namhyung Kim
@ 2022-04-25 20:59 ` Ian Rogers
  2022-04-28 13:52   ` Arnaldo Carvalho de Melo
  3 siblings, 1 reply; 8+ messages in thread
From: Ian Rogers @ 2022-04-25 20:59 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Michael Petlan,
	Peter Zijlstra, Ingo Molnar, Masami Hiramatsu, Song Liu,
	Will Deacon, Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

On Fri, Apr 15, 2022 at 5:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> This work is a follow-up of Ian's previous one at
>   https://lore.kernel.org/all/20220412154817.2728324-1-irogers@google.com/
>
> Fixing up more symbol ends as introduced in:
>   https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
>
> it 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 fixup logic so that special kernel cases aren't
> applied in the common case.
>
> v3 changes)
>  * rename is_kernel to is_kallsyms
>  * move the logic to generic function
>  * remove arch-specific functions
>
> Thanks,
> Namhyung

Thanks Namhyung! The series:

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

> Namhyung Kim (3):
>   perf symbol: Pass is_kallsyms to symbols__fixup_end()
>   perf symbol: Update symbols__fixup_end()
>   perf symbol: Remove arch__symbols__fixup_end()
>
>  tools/perf/arch/arm64/util/machine.c   | 21 ---------------
>  tools/perf/arch/powerpc/util/Build     |  1 -
>  tools/perf/arch/powerpc/util/machine.c | 25 -----------------
>  tools/perf/arch/s390/util/machine.c    | 16 -----------
>  tools/perf/util/symbol-elf.c           |  2 +-
>  tools/perf/util/symbol.c               | 37 +++++++++++++++++++-------
>  tools/perf/util/symbol.h               |  3 +--
>  7 files changed, 29 insertions(+), 76 deletions(-)
>  delete mode 100644 tools/perf/arch/powerpc/util/machine.c
>
>
> base-commit: 41204da4c16071be9090940b18f566832d46becc
> --
> 2.36.0.rc0.470.gd361397f0d-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3)
  2022-04-25 20:59 ` [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Ian Rogers
@ 2022-04-28 13:52   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-04-28 13:52 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Namhyung Kim, Jiri Olsa, Michael Petlan, Peter Zijlstra,
	Ingo Molnar, Masami Hiramatsu, Song Liu, Will Deacon,
	Mark Rutland, John Garry, Mathieu Poirier, Leo Yan,
	Michael Ellerman, Heiko Carstens, LKML, linux-perf-users,
	linuxppc-dev, linux-s390

Em Mon, Apr 25, 2022 at 01:59:03PM -0700, Ian Rogers escreveu:
> On Fri, Apr 15, 2022 at 5:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > This work is a follow-up of Ian's previous one at
> >   https://lore.kernel.org/all/20220412154817.2728324-1-irogers@google.com/
> >
> > Fixing up more symbol ends as introduced in:
> >   https://lore.kernel.org/lkml/20220317135536.805-1-mpetlan@redhat.com/
> >
> > it 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 fixup logic so that special kernel cases aren't
> > applied in the common case.
> >
> > v3 changes)
> >  * rename is_kernel to is_kallsyms
> >  * move the logic to generic function
> >  * remove arch-specific functions
> >
> > Thanks,
> > Namhyung
> 
> Thanks Namhyung! The series:
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied to perf/urgent.

- Arnaldo

 
> > Namhyung Kim (3):
> >   perf symbol: Pass is_kallsyms to symbols__fixup_end()
> >   perf symbol: Update symbols__fixup_end()
> >   perf symbol: Remove arch__symbols__fixup_end()
> >
> >  tools/perf/arch/arm64/util/machine.c   | 21 ---------------
> >  tools/perf/arch/powerpc/util/Build     |  1 -
> >  tools/perf/arch/powerpc/util/machine.c | 25 -----------------
> >  tools/perf/arch/s390/util/machine.c    | 16 -----------
> >  tools/perf/util/symbol-elf.c           |  2 +-
> >  tools/perf/util/symbol.c               | 37 +++++++++++++++++++-------
> >  tools/perf/util/symbol.h               |  3 +--
> >  7 files changed, 29 insertions(+), 76 deletions(-)
> >  delete mode 100644 tools/perf/arch/powerpc/util/machine.c
> >
> >
> > base-commit: 41204da4c16071be9090940b18f566832d46becc
> > --
> > 2.36.0.rc0.470.gd361397f0d-goog
> >

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-04-28 13:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-16  0:40 [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Namhyung Kim
2022-04-16  0:40 ` [PATCH 1/3] perf symbol: Pass is_kallsyms to symbols__fixup_end() Namhyung Kim
2022-04-16 14:59   ` Ian Rogers
2022-04-25 19:14     ` Namhyung Kim
2022-04-16  0:40 ` [PATCH 2/3] perf symbol: Update symbols__fixup_end() Namhyung Kim
2022-04-16  0:40 ` [PATCH 3/3] perf symbol: Remove arch__symbols__fixup_end() Namhyung Kim
2022-04-25 20:59 ` [PATCH 0/3] perf tools: Tidy up symbol end fixup (v3) Ian Rogers
2022-04-28 13:52   ` Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).