Re: [PATCH 2/2] perf tools: Improve IBS error handling

Kim Phillips
2021-11-23T15:26:08Z

On 11/23/21 2:40 AM, kajoljain wrote:
> On 10/8/21 12:47 AM, Kim Phillips wrote:
>> On 10/7/21 12:28 PM, Jiri Olsa wrote:
>>> On Mon, Oct 04, 2021 at 04:41:14PM -0500, Kim Phillips wrote:
>>>> ---
>>>>   tools/perf/util/evsel.c | 24 ++++++++++++++++++++++++
>>>>   1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>>> index b915840690d4..f8a9cbd99314 100644
>>>> --- a/tools/perf/util/evsel.c
>>>> +++ b/tools/perf/util/evsel.c
>>>> @@ -2743,9 +2743,22 @@ static bool find_process(const char *name)
>>>>       return ret ? false : true;
>>>>   }
>>>>   +static bool is_amd(const char *arch, const char *cpuid)
>>>> +{
>>>> +    return arch && !strcmp("x86", arch) && cpuid && strstarts(cpuid,
>>>> "AuthenticAMD");
>>>> +}
>>>> +
>>>> +static bool is_amd_ibs(struct evsel *evsel)
>>>> +{
>>>> +    return evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name,
>>>> "ibs", 3);
>>>> +}
>>>> +
>>>>   int evsel__open_strerror(struct evsel *evsel, struct target *target,
>>>>                int err, char *msg, size_t size)
>>>>   {
>>>> +    struct perf_env *env = evsel__env(evsel);
>>>> +    const char *arch = perf_env__arch(env);
>>>> +    const char *cpuid = perf_env__cpuid(env);
>>>>       char sbuf[STRERR_BUFSIZE];
>>>>       int printed = 0, enforced = 0;
>>>>   @@ -2841,6 +2854,17 @@ int evsel__open_strerror(struct evsel
>>>> *evsel, struct target *target,
>>>>               return scnprintf(msg, size, "wrong clockid (%d).",
>>>> clockid);
>>>>           if (perf_missing_features.aux_output)
>>>>               return scnprintf(msg, size, "The 'aux_output' feature
>>>> is not supported, update the kernel.");
>>>> +        if (is_amd(arch, cpuid)) {
>>>> +            if (is_amd_ibs(evsel)) {
>>>
>>> would single 'is_amd_ibs' call be better? checking on both amd and ibs
>>
>> Good suggestion. If you look at the later patch in the
>> BRS series, I have rewritten it to add the new
>> AMD PMU like so:
>>
>>  if (is_amd()) {
>>      if (is_amd_ibs()) {
>>          if (evsel->this)
>>              return
>>          if (evsel->that)
>>              return
>>      }
>> +    if (is_amd_brs()) {
>> +        if (evsel->this)
>> +            return
>> +        if (evsel->that)
>> +            return
>> +    }
>>  }
>
> Hi Kim,
> From my point of view, it won't be a good idea of adding so many
> checks in common function definition itself.
> Can you just create a check to see if its amd machine and then add a
> function call which will handle all four conditions together?
>
> which is basically for:
>
> + if (is_amd(arch, cpuid)) {
> + if (is_amd_ibs(evsel)) {
> + if (evsel->core.attr.exclude_kernel)
> + return scnprintf(msg, size,
> + "AMD IBS can't exclude kernel events. Try running at a higher
> privilege level.");
> + if (!evsel->core.system_wide)
> + return scnprintf(msg, size,
> + "AMD IBS may only be available in system-wide/per-cpu mode. Try using
> -a, or -C and workload affinity");
> + }
>
> and this:
>
> + if (is_amd_brs(evsel)) {
> + if (evsel->core.attr.freq)
> + return scnprintf(msg, size,
> + "AMD Branch Sampling does not support frequency mode sampling, must
> pass a fixed sampling period via -c option or
> cpu/branch-brs,period=xxxx/.");
> + /* another reason is that the period is too small */
> + return scnprintf(msg, size,
> + "AMD Branch Sampling does not support sampling period smaller than
> what is reported in /sys/devices/cpu/caps/branches.");
> + }

IIRC, I tried something like that but carrying the

struct target *target, int err, char *msg, size_t size

parameters made things worse.

> So, incase we are in amd machine, common function evsel__open_strerror
> will call function may be something like amd_evesel_open_strerror_check
> which will look for both ibs and brs conditions and return corresponding
> error statement.

The vast majority of decisions made by evsel__open_strerror are
going to be common across most arch/uarches. AMD has only these
two pesky exceptions to the rule and therefore IMO it's ok
to have them inline with the common function, since the decisions
are so deeply intertwined. A new amd_evsel_open_strerror_check
sounds like it'd duplicate too much of the common function code
in order to handle the common error cases.

Kim