2022-04-07 23:34:35

by Ian Rogers

[permalink] [raw]
Subject: [PATCH 0/5] Tidy up symbol end fixup

Fixing up more symbol ends as introduced in:
https://lore.kernel.org/lkml/[email protected]/
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.

Minor fix to perf annotate to not stall when stderr is full.

Ian Rogers (5):
perf annotate: Drop objdump stderr
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/annotate.c | 1 +
tools/perf/util/symbol-elf.c | 2 +-
tools/perf/util/symbol.c | 14 ++++++++------
tools/perf/util/symbol.h | 4 ++--
7 files changed, 36 insertions(+), 21 deletions(-)

--
2.35.1.1178.g4f1659d476-goog


2022-04-07 23:34:41

by Ian Rogers

[permalink] [raw]
Subject: [PATCH 1/5] perf annotate: Drop objdump stderr

If objdump writes to stderr it can block waiting for it to be read. As
perf doesn't read stderr then progress stops with perf waiting for
stdout output.

Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/annotate.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e4c641b240df..82cc396ef516 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2047,6 +2047,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
objdump_process.argv = objdump_argv;
objdump_process.out = -1;
objdump_process.err = -1;
+ objdump_process.no_stderr = 1;
if (start_command(&objdump_process)) {
pr_err("Failure starting to run %s\n", command);
err = -1;
--
2.35.1.1178.g4f1659d476-goog

2022-04-07 23:35:06

by Ian Rogers

[permalink] [raw]
Subject: [PATCH 3/5] perf symbols: Add is_kernel argument to fixup end

Kernel fixups may be special so add a flag to quickly detect if they are
necessary.

Signed-off-by: Ian Rogers <[email protected]>
---
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 | 9 +++++----
tools/perf/util/symbol.h | 4 ++--
6 files changed, 15 insertions(+), 10 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..087cdf2a58c9 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)
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

2022-04-07 23:35:41

by Ian Rogers

[permalink] [raw]
Subject: [PATCH 2/5] perf symbols: Always do architecture specific fixups

The change:
https://lore.kernel.org/lkml/[email protected]/
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.

Signed-off-by: Ian Rogers <[email protected]>
---
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

2022-04-07 23:35:43

by Ian Rogers

[permalink] [raw]
Subject: [PATCH 4/5] perf symbol: By default only fix zero length symbols

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/[email protected]/
where non-zero length symbols were expanded to the start of the current
symbol.

Signed-off-by: Ian Rogers <[email protected]>
---
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 087cdf2a58c9..59c562316d75 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

2022-04-07 23:35:48

by Ian Rogers

[permalink] [raw]
Subject: [PATCH 5/5] perf symbols: More specific architecture end fixing

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/[email protected]/

Signed-off-by: Ian Rogers <[email protected]>
---
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

2022-04-09 07:48:23

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 0/5] Tidy up symbol end fixup

Hi Ian,

On Thu, Apr 7, 2022 at 4:05 PM Ian Rogers <[email protected]> wrote:
>
> Fixing up more symbol ends as introduced in:
> https://lore.kernel.org/lkml/[email protected]/
> 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.

I'm not sure what was the actual problem the patch tried to solve.
It seems like a specific problem on powerpc + kprobes and now
it affects all other architectures.

In the above commit, optinsn_slot and kprobe_optinsn_page will
have the same address and optinsn_slot cannot be fixed up.
But I guess the kprobe_optinsn_page still can be fixed up and
you can use the symbol instead, no?

To me, it'd be better to revert the change and add a special
handling for the kprobe insn pages as they appear as
modules.

Also, all the arch symbols fixup routine seem to do the same
then we might move it to the general logic.

Thanks,
Namhyung

>
> Minor fix to perf annotate to not stall when stderr is full.
>
> Ian Rogers (5):
> perf annotate: Drop objdump stderr
> 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/annotate.c | 1 +
> tools/perf/util/symbol-elf.c | 2 +-
> tools/perf/util/symbol.c | 14 ++++++++------
> tools/perf/util/symbol.h | 4 ++--
> 7 files changed, 36 insertions(+), 21 deletions(-)
>
> --
> 2.35.1.1178.g4f1659d476-goog
>

2022-04-10 21:00:01

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 1/5] perf annotate: Drop objdump stderr

Em Thu, Apr 07, 2022 at 04:04:59PM -0700, Ian Rogers escreveu:
> If objdump writes to stderr it can block waiting for it to be read. As
> perf doesn't read stderr then progress stops with perf waiting for
> stdout output.

Thanks, applied.

- Arnaldo


> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/annotate.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index e4c641b240df..82cc396ef516 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2047,6 +2047,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> objdump_process.argv = objdump_argv;
> objdump_process.out = -1;
> objdump_process.err = -1;
> + objdump_process.no_stderr = 1;
> if (start_command(&objdump_process)) {
> pr_err("Failure starting to run %s\n", command);
> err = -1;
> --
> 2.35.1.1178.g4f1659d476-goog

--

- Arnaldo

2022-04-11 08:53:45

by Ian Rogers

[permalink] [raw]
Subject: Re: [PATCH 0/5] Tidy up symbol end fixup

On Fri, Apr 8, 2022 at 10:15 AM Namhyung Kim <[email protected]> wrote:
>
> Hi Ian,
>
> On Thu, Apr 7, 2022 at 4:05 PM Ian Rogers <[email protected]> wrote:
> >
> > Fixing up more symbol ends as introduced in:
> > https://lore.kernel.org/lkml/[email protected]/
> > 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.
>
> I'm not sure what was the actual problem the patch tried to solve.
> It seems like a specific problem on powerpc + kprobes and now
> it affects all other architectures.

Thanks Namhyung, this patch set reverts that.

> In the above commit, optinsn_slot and kprobe_optinsn_page will
> have the same address and optinsn_slot cannot be fixed up.
> But I guess the kprobe_optinsn_page still can be fixed up and
> you can use the symbol instead, no?
>
> To me, it'd be better to revert the change and add a special
> handling for the kprobe insn pages as they appear as
> modules.
>
> Also, all the arch symbols fixup routine seem to do the same
> then we might move it to the general logic.

Agreed. It isn't possible for me to test that behavior and so the
change tries to make the behavior clearer and to isolate Michael's
change to PowerPC. I like the idea of removing the weak symbols and
making the behavior common. I also don't know why we're trying to fix
broken elf file symbols and don't just let the assembly/compiler
writers fix that.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Minor fix to perf annotate to not stall when stderr is full.
> >
> > Ian Rogers (5):
> > perf annotate: Drop objdump stderr
> > 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/annotate.c | 1 +
> > tools/perf/util/symbol-elf.c | 2 +-
> > tools/perf/util/symbol.c | 14 ++++++++------
> > tools/perf/util/symbol.h | 4 ++--
> > 7 files changed, 36 insertions(+), 21 deletions(-)
> >
> > --
> > 2.35.1.1178.g4f1659d476-goog
> >

2022-04-12 21:11:13

by kernel test robot

[permalink] [raw]
Subject: [perf symbols] 309f68878e: perf-sanity-tests.perf.make_fail



Greeting,

FYI, we noticed the following commit (built with gcc-11):

commit: 309f68878e3f738e7a27d5ef354e2c35880fc047 ("[PATCH 3/5] perf symbols: Add is_kernel argument to fixup end")
url: https://github.com/intel-lab-lkp/linux/commits/Ian-Rogers/Tidy-up-symbol-end-fixup/20220408-070756
base: https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git 7bebfe9dd802b80abff5a43e00ab68d98893a22c
patch link: https://lore.kernel.org/linux-arm-kernel/[email protected]

in testcase: perf-sanity-tests
version:
with following parameters:

perf_compiler: clang
ucode: 0xec



on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):




If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>



==> /tmp/stderr <==
util/symbol.c: In function ‘dso__load_bfd_symbols’:
util/symbol.c:1663:2: error: too few arguments to function ‘symbols__fixup_end’
symbols__fixup_end(&dso->symbols);
^~~~~~~~~~~~~~~~~~
util/symbol.c:222:6: note: declared here
void symbols__fixup_end(struct rb_root_cached *symbols, bool is_kernel)
^~~~~~~~~~~~~~~~~~
make[4]: *** [/usr/src/perf_selftests-x86_64-rhel-8.3-func-309f68878e3f738e7a27d5ef354e2c35880fc047/tools/build/Makefile.build:97: util/symbol.o] Error 1
make[4]: *** Waiting for unfinished jobs....



To reproduce:

git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file

# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.



--
0-DAY CI Kernel Test Service
https://01.org/lkp



Attachments:
(No filename) (1.95 kB)
config-5.18.0-rc1-00013-g309f68878e3f (169.84 kB)
job-script (5.59 kB)
dmesg.xz (33.13 kB)
perf-sanity-tests (16.06 kB)
job.yaml (4.65 kB)
reproduce (272.00 B)
output (19.20 kB)
Download all attachments