On Mon, Jul 29, 2024 at 09:48:57AM +0100, Leo Yan wrote:
> On 7/25/2024 4:40 PM, Adrian Hunter wrote:
> > On 25/07/24 09:46, Leo Yan wrote:
> >> When finished to read AUX trace data from mmaped buffer, based on the
> >
> > Here and elsewhere 'mmapped' is more common than 'mmaped' so maybe:
> >
> > mmaped -> mmapped
>
> Will fix.
>
> >> AUX buffer index the core layer needs to search the corresponding PMU
> >> event and re-enable it to continue tracing.
> >>
> >> However, current code only searches the first AUX event. It misses to
> >> search other enabled AUX events, thus, it returns failure if the buffer
> >> index does not belong to the first AUX event.
> >>
> >> This patch extends the auxtrace_record__read_finish() function to
> >> search for every enabled AUX events, so all the mmaped buffer indexes
> >> can be covered.
> >
> > Looking at this again, a couple more things came to mind - see below.
> >
> >>
> >> Signed-off-by: Leo Yan <leo.yan(a)arm.com>
> >> ---
> >> tools/perf/util/auxtrace.c | 25 ++++++++++++++++++++-----
> >> 1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> >> index b99e72f7da88..61835a6a9ea3 100644
> >> --- a/tools/perf/util/auxtrace.c
> >> +++ b/tools/perf/util/auxtrace.c
> >> @@ -670,18 +670,33 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
> >> int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
> >> {
> >> struct evsel *evsel;
> >> + int ret = -EINVAL;
> >>
> >> - if (!itr->evlist || !itr->pmu)
> >> + if (!itr->evlist)
> >> return -EINVAL;
> >>
> >> evlist__for_each_entry(itr->evlist, evsel) {
> >> - if (evsel->core.attr.type == itr->pmu->type) {
> >> + if (evsel__is_aux_event(evsel)) {
> >> if (evsel->disabled)
> >> - return 0;
> >> - return evlist__enable_event_idx(itr->evlist, evsel, idx);
> >> + continue;
> >
> > That will make the evsel->disabled case an error, which
> > might be a problem. Possibly the event can be disabled
> > (e.g. via control fifo) but still have data to read out.
>
> If so, we need to extend evlist__enable_event_idx() or create a new function
> to check if the memory index is belonged to an evsel. If the idx is found for
> an evsel, then we can check the evsel->disabled flag.
>
> >> + /*
> >> + * Multiple AUX events might be opened in a session.
> >> + * Bail out for success case as the AUX event has been
> >> + * found and enabled, otherwise, continue to check if
> >> + * the next AUX event can cover the mmaped buffer with
> >> + * 'idx'.
> >> + */
> >
> > Thinking about this some more, raised some questions:
> >
> > How do we know there is only one AUX event per mmap?
>
> On my test platform, there have two Arm SPE events, the first one's cpumask is
> 2-5 and the second SPE's cpumask is 6-7. The AUX events do not intersect CPU
> maps. Therefore, a mmapped AUX buffer only binds to a AUX event.
>
> I think we can add a checking in the function record__auxtrace_init(). After
> it calls auxtrace_record__init(), we can report failure if the AUX events have
> the overlapped cpumask.
>
> > They would have to be on different CPUs for that to be true?
>
> Yes.
>
> > And wouldn't --per-thread have all AUX events in every mmap?
>
> Before I roughly tested '--per-thread' mode and did not see issue. But this
> time I tested '--per-thread' mode and found the failure by the kernel checking
> [1] - it does not allow the different AUX events to bind to the same FD.
>
> Here I need to dig a bit for two options, either we need to fix the perf
> tool to open multiple AUX events for '--per-thread' mode, or remove the
> kernel's checking to allow different AUX events to bind to same FD.
>
> Thanks a lot for review!
Ok, the two patches from Adrian are in, I'll now wait for you to refresh
this series,
Thanks,
- Arnaldo
On Fri, Jul 26, 2024 at 07:00:55PM +0300, Adrian Hunter wrote:
> On 26/07/24 17:41, Arnaldo Carvalho de Melo wrote:
> > On Thu, Jul 18, 2024 at 06:06:16PM +0300, Adrian Hunter wrote:
> >> On 18/07/24 15:58, Peter Zijlstra wrote:
> >>> On Thu, Jul 18, 2024 at 01:51:26PM +0200, Peter Zijlstra wrote:
> >>>
> >>>> OK, let me do that and make a few more edits and see if I can stare at
> >>>> that next patch some.
> >>>
> >>> I pushed out a stack of patches into queue.git perf/core
> >>> Could you please double check I didn't wreck anything?
> >>
> >> Looks fine, and seems to work OK in a brief test.
> >>
> >> Thank you! :-)
> >
> > So should I go ahead and pick the tooling patches since the kernel bits
> > are merged?
>
> Not exactly merged. Probably should wait until it is in tip at least.
Ok, so I get just these, as you asked on another message:
acme@x1:~/git/perf-tools-next$ git log --oneline -5 perf-tools-next/tmp.perf-tools-next
9140fec01b2de8d3 (HEAD -> perf-tools-next, perf-tools-next/tmp.perf-tools-next, perf-tools-next.korg/tmp.perf-tools-next, number/perf-tools-next, acme.korg/tmp.perf-tools-next) perf tools: Enable evsel__is_aux_event() to work for S390_CPUMSF
c3b7dba6ea81a5b1 perf tools: Enable evsel__is_aux_event() to work for ARM/ARM64
866400c0b08ef9d9 perf scripts python cs-etm: Restore first sample log in verbose mode
08ee74eb03e37191 perf cs-etm: Output 0 instead of 0xdeadbeef when exception packets are flushed
c22dd7ec2b2808b2 perf inject: Convert comma to semicolon
acme@x1:~/git/perf-tools-next$
It'll go to perf-tools-next once 6.11-rc1 is out.
- Arnaldo
On Tue, Jul 23, 2024 at 12:09:46PM +0300, Adrian Hunter wrote:
> On 23/07/24 11:26, Leo Yan wrote:
> > On 7/15/2024 5:07 PM, Adrian Hunter wrote:
> >>
> >> evsel__is_aux_event() identifies AUX area tracing selected events.
> >>
> >> S390_CPUMSF uses a raw event type (PERF_TYPE_RAW - refer
> >> s390_cpumsf_evsel_is_auxtrace()) not a PMU type value that could be checked
> >> in evsel__is_aux_event(). However it sets needs_auxtrace_mmap (refer
> >> auxtrace_record__init()), so check that first.
> >>
> >> Currently, the features that use evsel__is_aux_event() are used only by
> >> Intel PT, but that may change in the future.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter(a)intel.com>
> >> Acked-by: Ian Rogers <irogers(a)google.com>
> >> Reviewed-by: Andi Kleen <ak(a)linux.intel.com>
> >
> > Reviewed-by: Leo Yan <leo.yan(a)arm.com>
>
> Namhyung, could we get patches 5 and 6 from this series
> applied? They are independent and Leo's new patch set is
> dependent on patch 5.
I'm getting those two into tmp.perf-tools-next, probably should pick the
rest as peterz has the kernel bits already in his queue.git/perf/core,
right?
- Arnaldo
> >
> >> ---
> >> tools/perf/util/pmu.c | 6 +++++-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >> index 986166bc7c78..0b38c51bd6eb 100644
> >> --- a/tools/perf/util/pmu.c
> >> +++ b/tools/perf/util/pmu.c
> >> @@ -1199,8 +1199,12 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
> >>
> >> bool evsel__is_aux_event(const struct evsel *evsel)
> >> {
> >> - struct perf_pmu *pmu = evsel__find_pmu(evsel);
> >> + struct perf_pmu *pmu;
> >>
> >> + if (evsel->needs_auxtrace_mmap)
> >> + return true;
> >> +
> >> + pmu = evsel__find_pmu(evsel);
> >> return pmu && pmu->auxtrace;
> >> }
> >>
> >> --
> >> 2.34.1
> >>
> >>
On Thu, Jul 18, 2024 at 06:06:16PM +0300, Adrian Hunter wrote:
> On 18/07/24 15:58, Peter Zijlstra wrote:
> > On Thu, Jul 18, 2024 at 01:51:26PM +0200, Peter Zijlstra wrote:
> >
> >> OK, let me do that and make a few more edits and see if I can stare at
> >> that next patch some.
> >
> > I pushed out a stack of patches into queue.git perf/core
> > Could you please double check I didn't wreck anything?
>
> Looks fine, and seems to work OK in a brief test.
>
> Thank you! :-)
So should I go ahead and pick the tooling patches since the kernel bits
are merged?
- Arnaldo
On Tue, Jul 23, 2024 at 05:05:09PM +0100, Leo Yan wrote:
> On 7/23/2024 2:28 PM, James Clark wrote:
> >
> > The linked commit moved the early return on the first sample to before
> > the verbose log, so move the log earlier too. Now the first sample is
> > also logged and not skipped.
> >
> > Fixes: 2d98dbb4c9c5 ("perf scripts python arm-cs-trace-disasm.py: Do not ignore disam first sample")
> > Signed-off-by: James Clark <james.clark(a)linaro.org>
>
> Reviewed-by: Leo Yan <leo.yan(a)arm.com>
Thanks, applied to tmp.perf-tools-next,
- Arnaldo
> > ---
> > tools/perf/scripts/python/arm-cs-trace-disasm.py | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/perf/scripts/python/arm-cs-trace-disasm.py
> > index d973c2baed1c..7aff02d84ffb 100755
> > --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
> > +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
> > @@ -192,17 +192,16 @@ def process_event(param_dict):
> > ip = sample["ip"]
> > addr = sample["addr"]
> >
> > + if (options.verbose == True):
> > + print("Event type: %s" % name)
> > + print_sample(sample)
> > +
> > # Initialize CPU data if it's empty, and directly return back
> > # if this is the first tracing event for this CPU.
> > if (cpu_data.get(str(cpu) + 'addr') == None):
> > cpu_data[str(cpu) + 'addr'] = addr
> > return
> >
> > -
> > - if (options.verbose == True):
> > - print("Event type: %s" % name)
> > - print_sample(sample)
> > -
> > # If cannot find dso so cannot dump assembler, bail out
> > if (dso == '[unknown]'):
> > return
> > --
> > 2.34.1
> >
I noticed this when looking into Ganapat's fix to arm-cs-trace-disasm.py.
I assumed that seeing this in the output was a bug and went to
investigate why it happened in some cases and not others.
It turned out to not actually be a bug, but I think it doesn't look right.
In the end this change doesn't really accomplish anything and I'm not
sure if it's worth putting it in or not?
Maybe it will save someone doing the same thing as me, or maybe it will
actually break something if someones script is looking for 0xdeadbeef?
James Clark (1):
perf cs-etm: Output 0 instead of 0xdeadbeef when exception packets are
flushed
tools/perf/util/cs-etm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
--
2.34.1
On 24/07/2024 4:00 pm, Leo Yan wrote:
> On 7/24/2024 3:33 PM, James Clark wrote:
>>
>> The example shows the vmlinux path being given to the script, but this
>> only works when running on the target. If the script is run off the
>> target, then confusingly the vmlinux argument also needs to be given to
>> Perf as well.
>>
>> Without going into too much detail in the example about when it is or
>> isn't required, just include it. It doesn't do any harm even when
>> running on the target. Now the example command works in both places.
>>
>> Signed-off-by: James Clark <james.clark(a)linaro.org>
>
> The change makes senses to me. Just check a bit, does it need to add the same
> option for the command "only source line and symbols"?
>
> Thanks,
> Leo
>
I assumed that because that one didn't have vmlinux at all then it's
just for userspace tracing. I think it's good to have an example without
vmlinux to show that it's not a strict requirement.
>> ---
>> tools/perf/scripts/python/arm-cs-trace-disasm.py | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> index 7aff02d84ffb..4aeb9b497f7a 100755
>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> @@ -19,10 +19,10 @@ from perf_trace_context import perf_set_itrace_options, \
>> # Below are some example commands for using this script.
>> #
>> # Output disassembly with objdump:
>> -# perf script -s scripts/python/arm-cs-trace-disasm.py \
>> +# perf script -k path/to/vmlinux -s scripts/python/arm-cs-trace-disasm.py \
>> # -- -d objdump -k path/to/vmlinux
>> # Output disassembly with llvm-objdump:
>> -# perf script -s scripts/python/arm-cs-trace-disasm.py \
>> +# perf script -k path/to/vmlinux -s scripts/python/arm-cs-trace-disasm.py \
>> # -- -d llvm-objdump-11 -k path/to/vmlinux
>> # Output only source line and symbols:
>> # perf script -s scripts/python/arm-cs-trace-disasm.py
>> --
>> 2.34.1
>>
The example shows the vmlinux path being given to the script, but this
only works when running on the target. If the script is run off the
target, then confusingly the vmlinux argument also needs to be given to
Perf as well.
Without going into too much detail in the example about when it is or
isn't required, just include it. It doesn't do any harm even when
running on the target. Now the example command works in both places.
Signed-off-by: James Clark <james.clark(a)linaro.org>
---
tools/perf/scripts/python/arm-cs-trace-disasm.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/perf/scripts/python/arm-cs-trace-disasm.py
index 7aff02d84ffb..4aeb9b497f7a 100755
--- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
+++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
@@ -19,10 +19,10 @@ from perf_trace_context import perf_set_itrace_options, \
# Below are some example commands for using this script.
#
# Output disassembly with objdump:
-# perf script -s scripts/python/arm-cs-trace-disasm.py \
+# perf script -k path/to/vmlinux -s scripts/python/arm-cs-trace-disasm.py \
# -- -d objdump -k path/to/vmlinux
# Output disassembly with llvm-objdump:
-# perf script -s scripts/python/arm-cs-trace-disasm.py \
+# perf script -k path/to/vmlinux -s scripts/python/arm-cs-trace-disasm.py \
# -- -d llvm-objdump-11 -k path/to/vmlinux
# Output only source line and symbols:
# perf script -s scripts/python/arm-cs-trace-disasm.py
--
2.34.1
The linked commit moved the early return on the first sample to before
the verbose log, so move the log earlier too. Now the first sample is
also logged and not skipped.
Fixes: 2d98dbb4c9c5 ("perf scripts python arm-cs-trace-disasm.py: Do not ignore disam first sample")
Signed-off-by: James Clark <james.clark(a)linaro.org>
---
tools/perf/scripts/python/arm-cs-trace-disasm.py | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/perf/scripts/python/arm-cs-trace-disasm.py
index d973c2baed1c..7aff02d84ffb 100755
--- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
+++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
@@ -192,17 +192,16 @@ def process_event(param_dict):
ip = sample["ip"]
addr = sample["addr"]
+ if (options.verbose == True):
+ print("Event type: %s" % name)
+ print_sample(sample)
+
# Initialize CPU data if it's empty, and directly return back
# if this is the first tracing event for this CPU.
if (cpu_data.get(str(cpu) + 'addr') == None):
cpu_data[str(cpu) + 'addr'] = addr
return
-
- if (options.verbose == True):
- print("Event type: %s" % name)
- print_sample(sample)
-
# If cannot find dso so cannot dump assembler, bail out
if (dso == '[unknown]'):
return
--
2.34.1