From: Veronika Molnarova <vmolnaro@redhat.com>
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] perf test: Be more tolerant of metricgroup failures
Date: Mon, 29 Apr 2024 16:49:06 +0200 [thread overview]
Message-ID: <ce24cf21-422f-45fe-838e-1e36858b93be@redhat.com> (raw)
In-Reply-To: <CAP-5=fU_EvN-bSjwbWMP8R+WyG-jeAQ1p4ziyejy2FCH0kgYig@mail.gmail.com>
On 4/22/24 17:42, Ian Rogers wrote:
> On Mon, Apr 22, 2024 at 4:51 AM Veronika Molnarova <vmolnaro@redhat.com> wrote:
>>
>> Hi Ian,
>>
>> On Wed, Apr 3, 2024 at 6:48 PM Ian Rogers <irogers@google.com> wrote:
>>>
>>> Previously "set -e" meant any non-zero exit code from perf stat would
>>> cause a test failure. As a non-zero exit happens when there aren't
>>> sufficient permissions, check for this case and make the exit code
>>> 2/skip for it.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>> .../perf/tests/shell/stat_all_metricgroups.sh | 28 +++++++++++++++----
>>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/perf/tests/shell/stat_all_metricgroups.sh b/tools/perf/tests/shell/stat_all_metricgroups.sh
>>> index 55ef9c9ded2d..d6db192b9f18 100755
>>> --- a/tools/perf/tests/shell/stat_all_metricgroups.sh
>>> +++ b/tools/perf/tests/shell/stat_all_metricgroups.sh
>>> @@ -1,9 +1,7 @@
>>> -#!/bin/sh
>>> +#!/bin/bash
>>> # perf all metricgroups test
>>> # SPDX-License-Identifier: GPL-2.0
>>>
>>> -set -e
>>> -
>>> ParanoidAndNotRoot()
>>> {
>>> [ "$(id -u)" != 0 ] && [ "$(cat /proc/sys/kernel/perf_event_paranoid)" -gt $1 ]
>>> @@ -14,11 +12,29 @@ if ParanoidAndNotRoot 0
>>> then
>>> system_wide_flag=""
>>> fi
>>> -
>>> +err=0
>>> for m in $(perf list --raw-dump metricgroups)
>>> do
>>> echo "Testing $m"
>>> - perf stat -M "$m" $system_wide_flag sleep 0.01
>>> + result=$(perf stat -M "$m" $system_wide_flag sleep 0.01 2>&1)
>>> + result_err=$?
>>> + if [[ $result_err -gt 0 ]]
>>> + then
>>> + if [[ "$result" =~ \
>>> + "Access to performance monitoring and observability operations is limited" ]]
>>> + then
>>> + echo "Permission failure"
>>> + echo $result
>>> + if [[ $err -eq 0 ]]
>>> + then
>>> + err=2 # Skip
>>> + fi
>>> + else
>>> + echo "Metric group $m failed"
>>> + echo $result
>>> + err=1 # Fail
>>> + fi
>>> + fi
>>> done
>>>
>>> -exit 0
>>> +exit $err
>>> --
>>> 2.44.0.478.gd926399ef9-goog
>>>
>>>
>>
>> The patch looks good and thanks for taking care of it.
>>
>> Just wanted to check what is the desired outcome for metric groups
>> with events that are invalid in per-thread mode causing the test to fail.
>>
>> ```
>> $ ./stat_all_metricgroups.sh
>> Testing smi
>> Metric group smi failed
>> Error: Invalid event (msr/smi/u) in per-thread mode, enable system wide with '-a'.
>> ```
>>
>> Wouldn't it be better if in these cases the test would result in skip instead of fail?
>
> Hi Veronika,
>
> I agree that fail isn't best here. I'm wondering:
>
> - why doesn't msr/smi/ support per-thread mode? Can't we save/restore
> the count on a context switch? The implementation is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/arch/x86/events/msr.c?h=perf-tools-next#n234
> There's clearly something going on as pperf appears to have other restrictions:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/arch/x86/events/msr.c?h=perf-tools-next#n115
> I'm wondering if aggregation is working right if these counters are
> more than per hyperthread (I'm guessing why the restrictions exist).
>
Hi Ian,
I am not really sure why the msr/smi/ does not support per-thread mode but
I encountered multiple instances of events that aren't supported in per-thread
mode during testing. Generally, we cannot be sure that there won't be such event,
whether the msr/smi event should be able to handle per-thread mode is another
question.
Thanks for checking it out and would be great if you could add it to v2.
Thanks,
Veronika
> - the tool error message is doing pretty good. In the test I guess we
> can spot the per-thread error and turn the fail to a skip. It's a
> shame to bucket things as skip, but it seems easier than listing
> metrics in the test or spotting particular events.
>
> I can do a v2 to add this.
>
> Thanks,
> Ian
>
>> Thanks,
>> Veronika
>>
>
prev parent reply other threads:[~2024-04-29 14:49 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-03 16:48 [PATCH v1] perf test: Be more tolerant of metricgroup failures Ian Rogers
2024-04-04 18:08 ` Namhyung Kim
[not found] ` <CAGa8GX5MhP3FUhafN5QivHaE3Fg+p5MgvTq3SW6MQy4NeZ1sYQ@mail.gmail.com>
2024-04-22 15:42 ` Ian Rogers
2024-04-25 18:58 ` Arnaldo Carvalho de Melo
2024-04-29 14:49 ` Veronika Molnarova [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ce24cf21-422f-45fe-838e-1e36858b93be@redhat.com \
--to=vmolnaro@redhat.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).