perf report: Do not process non-JIT BPF ksymbol events

[PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Namhyung Kim 11 months, 1 week ago

The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
code when it gets samples in the region but non-JITed code cannot get
samples.  Thus it'd be ok to ignore them.

Actually it caused a performance issue in the perf tools on old ARM
kernels where it can refuse to JIT some BPF codes.  It ended up
splitting the existing kernel map (kallsyms).  And later lookup for a
kernel symbol would create a new kernel map from kallsyms and then
split it again and again. :(

Probably there's a bug in the kernel map/symbol handling in perf tools.
But I think we need to fix this anyway.

Reported-by: Kevin Nomura <nomurak@google.com>
Cc: Song Liu <song@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/machine.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 3f1faf94198dbe56..c7d27384f0736408 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -779,6 +779,10 @@ int machine__process_ksymbol(struct machine *machine __maybe_unused,
 	if (dump_trace)
 		perf_event__fprintf_ksymbol(event, stdout);
 
+	/* no need to process non-JIT BPF as it cannot get samples */
+	if (event->ksymbol.len == 0)
+		return 0;
+
 	if (event->ksymbol.flags & PERF_RECORD_KSYMBOL_FLAGS_UNREGISTER)
 		return machine__process_ksymbol_unregister(machine, event,
 							   sample);
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Namhyung Kim 11 months ago

On Wed, 05 Mar 2025 15:28:38 -0800, Namhyung Kim wrote:
> The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
> it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
> code when it gets samples in the region but non-JITed code cannot get
> samples.  Thus it'd be ok to ignore them.
> 
> Actually it caused a performance issue in the perf tools on old ARM
> kernels where it can refuse to JIT some BPF codes.  It ended up
> splitting the existing kernel map (kallsyms).  And later lookup for a
> kernel symbol would create a new kernel map from kallsyms and then
> split it again and again. :(
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung

Re: [PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Adrian Hunter 11 months, 1 week ago

On 6/03/25 01:28, Namhyung Kim wrote:
> The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
> it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
> code when it gets samples in the region but non-JITed code cannot get
> samples.  Thus it'd be ok to ignore them.
> 
> Actually it caused a performance issue in the perf tools on old ARM
> kernels where it can refuse to JIT some BPF codes.  It ended up
> splitting the existing kernel map (kallsyms).  And later lookup for a
> kernel symbol would create a new kernel map from kallsyms and then
> split it again and again. :(
> 
> Probably there's a bug in the kernel map/symbol handling in perf tools.
> But I think we need to fix this anyway.
> 
> Reported-by: Kevin Nomura <nomurak@google.com>
> Cc: Song Liu <song@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/machine.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 3f1faf94198dbe56..c7d27384f0736408 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -779,6 +779,10 @@ int machine__process_ksymbol(struct machine *machine __maybe_unused,
>  	if (dump_trace)
>  		perf_event__fprintf_ksymbol(event, stdout);
>  
> +	/* no need to process non-JIT BPF as it cannot get samples */
> +	if (event->ksymbol.len == 0)
> +		return 0;

Are all ksymbol events BPF?  Maybe it is OK
for PERF_RECORD_KSYMBOL_TYPE_OOL also.  Perhaps adjust the
comment in that case.

> +
>  	if (event->ksymbol.flags & PERF_RECORD_KSYMBOL_FLAGS_UNREGISTER)
>  		return machine__process_ksymbol_unregister(machine, event,
>  							   sample);

Re: [PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Namhyung Kim 11 months, 1 week ago

Hello,

On Thu, Mar 06, 2025 at 08:25:01AM +0200, Adrian Hunter wrote:
> On 6/03/25 01:28, Namhyung Kim wrote:
> > The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
> > it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
> > code when it gets samples in the region but non-JITed code cannot get
> > samples.  Thus it'd be ok to ignore them.
> > 
> > Actually it caused a performance issue in the perf tools on old ARM
> > kernels where it can refuse to JIT some BPF codes.  It ended up
> > splitting the existing kernel map (kallsyms).  And later lookup for a
> > kernel symbol would create a new kernel map from kallsyms and then
> > split it again and again. :(
> > 
> > Probably there's a bug in the kernel map/symbol handling in perf tools.
> > But I think we need to fix this anyway.
> > 
> > Reported-by: Kevin Nomura <nomurak@google.com>
> > Cc: Song Liu <song@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/machine.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 3f1faf94198dbe56..c7d27384f0736408 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -779,6 +779,10 @@ int machine__process_ksymbol(struct machine *machine __maybe_unused,
> >  	if (dump_trace)
> >  		perf_event__fprintf_ksymbol(event, stdout);
> >  
> > +	/* no need to process non-JIT BPF as it cannot get samples */
> > +	if (event->ksymbol.len == 0)
> > +		return 0;
> 
> Are all ksymbol events BPF?  Maybe it is OK
> for PERF_RECORD_KSYMBOL_TYPE_OOL also.  Perhaps adjust the
> comment in that case.

Probably, but I didn't see OOL with zero length yet.  Is it possible?

Thanks,
Namhyung

Re: [PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Adrian Hunter 11 months, 1 week ago

On 6/03/25 08:45, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Mar 06, 2025 at 08:25:01AM +0200, Adrian Hunter wrote:
>> On 6/03/25 01:28, Namhyung Kim wrote:
>>> The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
>>> it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
>>> code when it gets samples in the region but non-JITed code cannot get
>>> samples.  Thus it'd be ok to ignore them.
>>>
>>> Actually it caused a performance issue in the perf tools on old ARM
>>> kernels where it can refuse to JIT some BPF codes.  It ended up
>>> splitting the existing kernel map (kallsyms).  And later lookup for a
>>> kernel symbol would create a new kernel map from kallsyms and then
>>> split it again and again. :(
>>>
>>> Probably there's a bug in the kernel map/symbol handling in perf tools.
>>> But I think we need to fix this anyway.
>>>
>>> Reported-by: Kevin Nomura <nomurak@google.com>
>>> Cc: Song Liu <song@kernel.org>
>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>>> ---
>>>  tools/perf/util/machine.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>>> index 3f1faf94198dbe56..c7d27384f0736408 100644
>>> --- a/tools/perf/util/machine.c
>>> +++ b/tools/perf/util/machine.c
>>> @@ -779,6 +779,10 @@ int machine__process_ksymbol(struct machine *machine __maybe_unused,
>>>  	if (dump_trace)
>>>  		perf_event__fprintf_ksymbol(event, stdout);
>>>  
>>> +	/* no need to process non-JIT BPF as it cannot get samples */
>>> +	if (event->ksymbol.len == 0)
>>> +		return 0;
>>
>> Are all ksymbol events BPF?  Maybe it is OK
>> for PERF_RECORD_KSYMBOL_TYPE_OOL also.  Perhaps adjust the
>> comment in that case.
> 
> Probably, but I didn't see OOL with zero length yet.  Is it possible?

Probably not

Re: [PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Namhyung Kim 11 months, 1 week ago

On Thu, Mar 06, 2025 at 08:48:20AM +0200, Adrian Hunter wrote:
> On 6/03/25 08:45, Namhyung Kim wrote:
> > Hello,
> > 
> > On Thu, Mar 06, 2025 at 08:25:01AM +0200, Adrian Hunter wrote:
> >> On 6/03/25 01:28, Namhyung Kim wrote:
> >>> The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
> >>> it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
> >>> code when it gets samples in the region but non-JITed code cannot get
> >>> samples.  Thus it'd be ok to ignore them.
> >>>
> >>> Actually it caused a performance issue in the perf tools on old ARM
> >>> kernels where it can refuse to JIT some BPF codes.  It ended up
> >>> splitting the existing kernel map (kallsyms).  And later lookup for a
> >>> kernel symbol would create a new kernel map from kallsyms and then
> >>> split it again and again. :(
> >>>
> >>> Probably there's a bug in the kernel map/symbol handling in perf tools.
> >>> But I think we need to fix this anyway.
> >>>
> >>> Reported-by: Kevin Nomura <nomurak@google.com>
> >>> Cc: Song Liu <song@kernel.org>
> >>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >>> ---
> >>>  tools/perf/util/machine.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> >>> index 3f1faf94198dbe56..c7d27384f0736408 100644
> >>> --- a/tools/perf/util/machine.c
> >>> +++ b/tools/perf/util/machine.c
> >>> @@ -779,6 +779,10 @@ int machine__process_ksymbol(struct machine *machine __maybe_unused,
> >>>  	if (dump_trace)
> >>>  		perf_event__fprintf_ksymbol(event, stdout);
> >>>  
> >>> +	/* no need to process non-JIT BPF as it cannot get samples */
> >>> +	if (event->ksymbol.len == 0)
> >>> +		return 0;
> >>
> >> Are all ksymbol events BPF?  Maybe it is OK
> >> for PERF_RECORD_KSYMBOL_TYPE_OOL also.  Perhaps adjust the
> >> comment in that case.
> > 
> > Probably, but I didn't see OOL with zero length yet.  Is it possible?
> 
> Probably not

Then I think it's ok to leave the comment as is.

Thanks,
Namhyung

Re: [PATCH] perf report: Do not process non-JIT BPF ksymbol events

Posted by Song Liu 11 months, 1 week ago

On Wed, Mar 5, 2025 at 3:28 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The length of PERF_RECORD_KSYMBOL for BPF is a size of JITed code so
> it'd be 0 when it's not JITed.  The ksymbol is needed to symbolize the
> code when it gets samples in the region but non-JITed code cannot get
> samples.  Thus it'd be ok to ignore them.
>
> Actually it caused a performance issue in the perf tools on old ARM
> kernels where it can refuse to JIT some BPF codes.  It ended up
> splitting the existing kernel map (kallsyms).  And later lookup for a
> kernel symbol would create a new kernel map from kallsyms and then
> split it again and again. :(
>
> Probably there's a bug in the kernel map/symbol handling in perf tools.
> But I think we need to fix this anyway.
>
> Reported-by: Kevin Nomura <nomurak@google.com>
> Cc: Song Liu <song@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Song Liu <song@kernel.org>