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.
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
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/[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.
>
> 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 */
On Tue, Apr 12, 2022 at 3:12 PM Namhyung Kim <[email protected]> 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/[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.
> >
> > 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 */
On Tue, Apr 12, 2022 at 4:48 PM Ian Rogers <[email protected]> wrote:
>
> On Tue, Apr 12, 2022 at 3:12 PM Namhyung Kim <[email protected]> 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/[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.
> > >
> > > 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 */