Re: [PATCH v2] perf evsel amd: Fix IBS error message

[Ravi Bangoria <ravi.bangoria@amd.com>]

On 27-Jun-23 4:34 AM, Namhyung Kim wrote:
> Hi Ravi,
> 
> On Mon, Jun 26, 2023 at 3:40 AM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>>
>> AMD IBS can do per-process profiling[1] and is no longer restricted to
>> per-cpu or systemwide only. Remove stale error message. Also, checking
>> just exclude_kernel is not sufficient since IBS does not support any
>> privilege filters. So include all exclude_* checks. And finally, move
>> these checks under tools/perf/arch/x86/ from generic code.
>>
>> Before:
>>   $ sudo ./perf record -e ibs_op//k -C 0
>>   Error:
>>   AMD IBS may only be available in system-wide/per-cpu mode.  Try
>>   using -a, or -C and workload affinity
>>
>> After:
>>   $ sudo ./perf record -e ibs_op//k -C 0
>>   Error:
>>   AMD IBS doesn't support privilege filtering. Try again with
>>   exclude_{kernel|user|hv|idle|host|guest}=0.
> 
> Can we have more user-friendly messages like below?
> 
>  "Try again without the privilege modifiers like 'k' at the end."
Sure.

> 
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
>> ---
>> v1: https://lore.kernel.org/r/20230621062359.201-1-ravi.bangoria@amd.com
>> v1->v2:
>>   - Check all exclude_* flags not just exclude_kernel
>>   - Move AMD specific checks under tools/perf/arch/x86/
>>
>>  tools/perf/arch/x86/util/evsel.c | 25 +++++++++++++++++++++++++
>>  tools/perf/util/evsel.c          | 30 +++++++++---------------------
>>  tools/perf/util/evsel.h          |  1 +
>>  3 files changed, 35 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>> index 512c2d885d24..9a7141c5a4ea 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -102,3 +102,28 @@ void arch__post_evsel_config(struct evsel *evsel, struct perf_event_attr *attr)
>>                 }
>>         }
>>  }
>> +
>> +int arch_evsel__open_strerror(struct evsel *evsel, char *msg, size_t size)
>> +{
>> +       if (!x86__is_amd_cpu())
>> +               return 0;
>> +
>> +       if (!evsel->core.attr.precise_ip &&
>> +           !(evsel->pmu_name && !strncmp(evsel->pmu_name, "ibs", 3)))
>> +               return 0;
>> +
>> +       /* More verbose IBS errors. */
>> +       if (evsel->core.attr.exclude_kernel || evsel->core.attr.exclude_user ||
>> +           evsel->core.attr.exclude_hv || evsel->core.attr.exclude_idle ||
>> +           evsel->core.attr.exclude_host || evsel->core.attr.exclude_guest) {
>> +               return scnprintf(msg, size, "AMD IBS doesn't support privilege filtering. Try "
>> +                                "again with exclude_{kernel|user|hv|idle|host|guest}=0.");
>> +       }
>> +
>> +       if (!evsel->core.attr.sample_period) {
>> +               return scnprintf(msg, size, "AMD IBS doesn't support counting mode. Try "
>> +                                "again with sample_{period|freq} with non-zero value.");
> 
> Is this for perf stat?   We don't allow zero period for perf record.
> 
>   $ perf record -F 0 true
>   frequency and count are zero, aborting
> 
> Then maybe it can say: "IBS doesn't support perf stat, Use perf record."

Ok. Let me skip this since generic code is already covering it.

Thanks,
Ravi