Message-ID: <4281dce8-0e2a-cfe8-3b05-1b9a455d09a9@amd.com>
Date:   Mon, 29 Nov 2021 16:09:47 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH 2/2] perf tools: Improve IBS error handling
Content-Language: en-US
To:     kajoljain <kjain@linux.ibm.com>, Jiri Olsa <jolsa@redhat.com>
Cc:     Arnaldo Carvalho de Melo <acme@redhat.com>,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Ian Rogers <irogers@google.com>,
        Ingo Molnar <mingo@redhat.com>,
        Joao Martins <joao.m.martins@oracle.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Michael Petlan <mpetlan@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Robert Richter <robert.richter@amd.com>,
        Stephane Eranian <eranian@google.com>
References: <20211004214114.188477-1-kim.phillips@amd.com>
 <20211004214114.188477-2-kim.phillips@amd.com> <YV8uQVnMnnMd1Led@krava>
 <8a8583dc-5a5d-f107-8ef0-6be96e2f9095@amd.com>
 <fdcfec83-01c6-5e25-5b99-dac05287fdae@linux.ibm.com>
 <74e17a71-98ff-e0b1-61d4-d37992b1ae15@amd.com>
 <f095bebf-77d5-94c7-5787-13a6f38867cc@linux.ibm.com>
From:   Kim Phillips <kim.phillips@amd.com>
In-Reply-To: <f095bebf-77d5-94c7-5787-13a6f38867cc@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d2diVTM4VWJxbmoyNEdvUCtTa1gzZFJXQk9xK0Yvb2xzdGFmUjRVMGdacUl1?=
 =?utf-8?B?S1krc1l5a2hkbWZmNU1FQmNKdkgwVzg0bzkyNVplUHR0RkZ4YitIVDNUNFVO?=
 =?utf-8?B?YmJONjZET3VuT3ZMM1hlUDZ1K3Z5aHRzcFpGbm9CblZwR1BWV3M1Y3g2REwy?=
 =?utf-8?B?K0loU293cFZnQ3BrdnhLc21XRmN1T05Kb0V5WUFiWEZxbHVoUGtSeEFJR1BT?=
 =?utf-8?B?VGRZNlBsMkVWYnF0RGR1MVFOaWU0RUcwYkp2a093QllzNkI1bnBJb2hzYk5v?=
 =?utf-8?B?MUdXeithbWhvVHQxVHM3UTJ1dC85MVpoeVpZT0tWMGl1MUVDK2VOcXFSenpZ?=
 =?utf-8?B?TndjWHZwY1M2enpMZ0NpNCt3dUQ3Z1RCS3E1UXRlVTRndlFkZEUvaHRpWTFZ?=
 =?utf-8?B?RjFZMkZZNXhwY3J2MWYwUEk5K1RERkphUFNlSGYweS82eER4ZVVUNWQvUWho?=
 =?utf-8?B?NGpUc3hBSFhua0pPbksvK0ZiYTl2WXRRaTh4YXdmRVEvNnNaTTJ3anFpNHpq?=
 =?utf-8?B?dUcrRXRKdGN2U3g0bjFGSmdMREVsQVNzc0d1aWZQdGFNQVZ3QzZTM01iVzVl?=
 =?utf-8?B?R1BKRUd6a2pQSUJFSnZ1eCtoRmNKUm0ydWt0OTlBWGlJeGxVNElIb3dEOGpu?=
 =?utf-8?B?SS9hSkhVdDJySUhrTGhEUUNoUkEybFRhclBSc0Y1V3p0K0VqMGFoV1NVREIx?=
 =?utf-8?B?anFjbUw2S0taSE1QNTl1dTE1SFZ4T1RpYVFQSEVLcFVRMXRhVWFCY0FzR05C?=
 =?utf-8?B?WDkydGxiMzNKRm1FTEoydG1sWHd5azkyZWNsVXJvczJIK2ZkMUVub0lHQ2RK?=
 =?utf-8?B?ZDc4L1huY2hkeTgxNE1WSHZVaGI5RTYxVHB0clZiRytHcTVuNHRuekJzRE5J?=
 =?utf-8?B?WWN3NzkzNzFtVHpjOGxXL0tmNmh3cWlnYXE0K011L3NiaUhHdmhMM3dPZUNO?=
 =?utf-8?B?bVhIa2tBQzA5aXg3MG1ZdGlmS3hGNlNiQy9qQXlzZ0hvVUJjK3ZKRVFWdS9s?=
 =?utf-8?B?UFd5VVR3ZXdHdUloK1FMeGxZSG82MkVWSHowaWN3TnRES1RiR2xIL0JTVDZl?=
 =?utf-8?B?REo1ZkwrU0tjRlFVSUFGMU1GbEFaMVdjS0hINjJHMUdhYW4yV01VRzlJNFFQ?=
 =?utf-8?B?ZW1aRU1URHp2UmdFWFp6MU85U0FRdWw5bFB0RU9iYTc2ejQyRGJEdEpWcnVU?=
 =?utf-8?B?NlRaQlRSSzNnUzB6cjB5WkhNUS9RMDRjUndXKzAvNW43RTZ5SVBUcWE1ZElI?=
 =?utf-8?B?QkNqTURyd2t4aEowaHdETHFrR3hDaWFoT1g5UTIvZVYwK3o5dlY2S1R2K0o0?=
 =?utf-8?B?alVtS0lsNktheHg4QzFWNlkvTzNvZ1gvaGFuZVkzajdFQXkrZGh1VjBxcFRY?=
 =?utf-8?B?Tkp4WDQ5V0d2Z1BienJaazJLL25FZHR0L1lKRHhlUHZJVjVkZUVZSDVhaUJu?=
 =?utf-8?B?MlBFaHAzbStmVHh4TkZsS1Q0L3JyMTEyNktGS2l2dlc2anVBY2toaXVMeVZt?=
 =?utf-8?B?VWtBV0RMcHloeFhmMWM4WUx1Ukx1NXZXbk5qZjEzSGNtMWw5cnlBcHRqVDBh?=
 =?utf-8?B?V1hGMmZqeWxhdjRSdzljNkQrOTBtN1VOT2dweW1oZHlEOFRPcWZoZ3MzRkwy?=
 =?utf-8?B?L3ZCb0RhTSt5a0hRWCtQMUJLL1V0N3Y1c0ZKdjRnLytYcU1maU1JUjRRcmpD?=
 =?utf-8?B?UUZDbVZaQlNqZ1Rzd09LU0pkaDBMRUNHWTc2M05DVUVxNTVrUy9XUCs3WXU1?=
 =?utf-8?B?a2NCRkZjYnYxQjZtTU12b0ZYUGdKNUR2WUtJaHJTWkF1NHhVRXhPY0xpYTNM?=
 =?utf-8?B?aW5MRnlZY1RJb3JJOWxkN3pkTVFGdEh6RFdpd3NZNlphMWhzOXJRUWhMenQw?=
 =?utf-8?B?TGhJZ3R1V1d2eDNrRFRybVlFUEZuU0xVcUNwSkhmeVVzSHU1ZnFXNW95UDdo?=
 =?utf-8?B?cEJBYjJpTW1KQ2Vodk5wTG8wV0IrWEEzVklXVUUwWVZrS2s5Yy9lTkQxbnl0?=
 =?utf-8?B?Kzk0ZlA0QlpleFlleWRvVWdpbnRRbDVjR3ZvZU9RQjlvQUZ2SmdlZk5Oa1FN?=
 =?utf-8?B?bURvTC9JcllBTDg1aGtPL0xraGh5UGx3UkprTC9xOURtUXlVNklwaGRzOGpW?=
 =?utf-8?B?Q2NPc3Q3ejNUbGpuMjhLWU1mQVQ1TzAyYmNmUFg0VStIOVRiMUxIek5NbTNI?=
 =?utf-8?Q?ILtKGtwHNUkFIxuKDFbsIIY=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ce11b4ee-2387-48d6-a92f-08d9b384f70b
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3505.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2021 22:09:51.3766
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: b81WKstaVFttrsnPkxzEmGjMEW5BmSBeNccAVMgVxLo/Vhf3NIoMcNV+Zw+YZJqszBc7g3Wl+mnpjh6SJw56cA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR12MB3187
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 11/24/21 2:00 AM, kajoljain wrote:
> On 11/23/21 8:55 PM, Kim Phillips wrote:
>> 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.
> 
> Hi Kim,
>     Sorry for the confusion, what I meant by adding new function is just
> to handle these corner error cases and not duplicating whole
> evsel__open_strerror code.
> 
> Maybe something like below code, Its just prototype of code to show you
> the flow, you can refine it and check for any build or indentation
> issues using checkpatch.pl script.
> 
> So basically, in common function we can just have 2 calls, first to
> check if we are in amd system and second to return corresponding error
> message, rather then adding whole chunk of if's which are specific to amd.
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ac0127be0459..adefb162ae08 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2852,9 +2852,40 @@ 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 int error_amd_ibs_brs(struct evsel *evsel, char *msg, size_t size)
> +{
> +       if (evsel->core.attr.precise_ip || !strncmp(evsel->pmu_name,
> "ibs", 3)) {
> +               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");
> +       }
> +
> +       if (((evsel->core.attr.config & 0xff) == 0xc4) &&
> (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)) {
> +               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.");
> +               }
> +       }
> +}
> +
>   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;
> 
> @@ -2950,6 +2981,8 @@ 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))
> +                       return error_amd_ibs_brs(evsel, msg, size);
>                  break;
>          case ENODATA:
>                  return scnprintf(msg, size, "Cannot collect data source
> with the load latency event alone. "

That change will makes AMD machines fail to fall back to the default
"The sys_perf_event_open() syscall returned with..." error string
in case it's not those AMD IBS and BRS sub-conditions.

Is having the AMD error code checking in the main evsel__open_strerror()
so bad?  Other arches and their PMU implementations may find error
conditions that they have in common with AMD's, therefore
opening up the code for opposite types of refactoring and
reuse than what is being requested here.  E.g., I've seen
other hardware configurations - not specific to one architecture -
that could also use this message:

{"AMD IBS"->"%s",pmu_name} may only be available in system-wide/per-cpu mode.  Try using -a, or -C and workload affinity");

Thanks,

Kim