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

[Kim Phillips]

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