Message ID | 20220412154817.2728324-1-irogers@google.com (mailing list archive) |
---|---|
Headers | show |
Series | Tidy up symbol end fixup | expand |
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 >
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 */
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 */
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 */
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(-)