Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2f0f83f3-91a9-a719-c36e-ca1ff5c9b6f8@linux.ibm.com>
Date:   Wed, 7 Dec 2022 11:01:35 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v3 2/2] perf test: Add event group test
To:     Ravi Bangoria <ravi.bangoria@amd.com>,
        Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Kan Liang <kan.liang@linux.intel.com>,
        Ian Rogers <irogers@google.com>, Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        carsten.haitzler@arm.com, Leo Yan <leo.yan@linaro.org>,
        Kajol Jain <kjain@linux.ibm.com>,
        Thomas Richter <tmricht@linux.ibm.com>,
        linux-perf-users@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>, sandipan.das@amd.com,
        ananth.narayan@amd.com, santosh.shukla@amd.com
References: <20221206043237.12159-1-ravi.bangoria@amd.com>
 <20221206043237.12159-3-ravi.bangoria@amd.com>
 <C7F7A9B0-421E-40AB-987C-6D250B9633D3@linux.vnet.ibm.com>
 <2e45a93d-56fd-b18e-de61-51ceccfee8a3@amd.com>
Content-Language: en-US
From:   Madhavan Srinivasan <maddy@linux.ibm.com>
In-Reply-To: <2e45a93d-56fd-b18e-de61-51ceccfee8a3@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 12/7/22 9:56 AM, Ravi Bangoria wrote:
> Hi Athira,
>
>>> +static int setup_uncore_event(void)
>>> +{
>>> +	struct perf_pmu *pmu;
>>> +	int i;
>>> +
>>> +	if (list_empty(&pmus))
>>> +		perf_pmu__scan(NULL);
>>> +
>>> +	perf_pmus__for_each_pmu(pmu) {
>>> +		for (i = 0; i < NR_UNCORE_PMUS; i++) {
>>> +			if (!strcmp(uncore_pmus[i].name, pmu->name)) {
>>> +				pr_debug("Using %s for uncore pmu event\n", pmu->name);
>>> +				types[2] = pmu->type;
>>> +				configs[2] = uncore_pmus[i].config;
>> Hi Ravi,
>>
>> Observed failure while running the test on powerpc. It is because the uncore PMU ie hv_24x7 needs
>> performance monitoring to be enabled in powerpc. So to handle such cases, can we add an “event_open" check before
>> proceeding with the test. Below is the change on top of “tmp.perf/core” .
>>
>>
>>  From 8b33fb900c26beafc28f75b6f64631f8fdd045c2 Mon Sep 17 00:00:00 2001
>> From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> Date: Tue, 6 Dec 2022 20:17:25 +0530
>> Subject: [PATCH] perf test: Update event group check for support of uncore
>>   event
>>
>> Event group test checks group creation for combinations of
>> hw, sw and uncore PMU events. Some of the uncore pmu event
>> requires performance enablement explicitly.
> You need to open an event to activate hv_24x7 pmu?
hv_24x7 pmu supports events which can also provide system-wide resource data
and partition should have permissions to access those, hence the check.

Maddy
>
>> Example, hv_24x7
>> event in powerpc. Hence add a check to see if event_open
>> succeeds before proceeding.
>>
>> Fixes: 5c88101b797d ("perf test: Add event group test for events in multiple PMUs")
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>>   tools/perf/tests/event_groups.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
>> index 612c0444aaa8..ad52e1da259a 100644
>> --- a/tools/perf/tests/event_groups.c
>> +++ b/tools/perf/tests/event_groups.c
>> @@ -51,7 +51,7 @@ static int event_open(int type, unsigned long config, int group_fd)
>>   static int setup_uncore_event(void)
>>   {
>>   	struct perf_pmu *pmu;
>> -	int i;
>> +	int i, fd;
>>   
>>   	if (list_empty(&pmus))
>>   		perf_pmu__scan(NULL);
>> @@ -62,6 +62,10 @@ static int setup_uncore_event(void)
>>   				pr_debug("Using %s for uncore pmu event\n", pmu->name);
>>   				types[2] = pmu->type;
>>   				configs[2] = uncore_pmus[i].config;
> Sure. Just add a comment here to explain why are we opening a
> standalone event here.
>
>> +				fd = event_open(types[2], configs[2], -1);
>> +				if (fd < 0)
>> +					return -1;
>> +				close(fd);
>>   				return 0;
>>   			}
>>   		}
> Thanks,
> Ravi