Message-ID: <45de7e62-d951-1019-990e-6df285a64cf6@amd.com>
Date:   Mon, 13 Mar 2023 17:59:46 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v2 2/3] perf/ibs: Fix interface via core pmu events
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     namhyung@kernel.org, eranian@google.com, acme@kernel.org,
        mark.rutland@arm.com, jolsa@kernel.org, irogers@google.com,
        bp@alien8.de, x86@kernel.org, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, sandipan.das@amd.com,
        ananth.narayan@amd.com, santosh.shukla@amd.com,
        Ravi Bangoria <ravi.bangoria@amd.com>
References: <20230309101111.444-1-ravi.bangoria@amd.com>
 <20230309101111.444-3-ravi.bangoria@amd.com>
 <20230312145426.GA1757905@hirez.programming.kicks-ass.net>
From:   Ravi Bangoria <ravi.bangoria@amd.com>
In-Reply-To: <20230312145426.GA1757905@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZzJ2Uy9kdGVPSTBvYUN5d0Y0UjYxVUVCeFpUSVlsZHVqNmZZeVBlOHNVWjNK?=
 =?utf-8?B?dVBDYVovODFJdGVwQlpVYkx0Zmk1bjJCaUFYVGNuWTVJZHdJdDVVa29BbVp0?=
 =?utf-8?B?UkYwTUg4NDA2MVBCS0Z5WXAySFVSdUphaXJJTENtZ1VLQng5STYwdWVPZDM0?=
 =?utf-8?B?NjFJVE5RK1hDbVpmQ1hKSjZZRkp4NTlRRnpkRnhOeDRBR0E0bVFDVHFWUHFN?=
 =?utf-8?B?QkpnVFQxUDAvTmJsalZDVmhMRkJsUWU0SW9pdWxxUnJHeWp2VVd4QW95eVhO?=
 =?utf-8?B?RDhUQzVsNC9ONWRaNmxVb3R2cUJEc05VMjdKRUZVUHJIQlJMdk1yMUVVZGFw?=
 =?utf-8?B?QnVGNE04d2JIWTZBRGFNcFNYNE1LR0ZRZUkwZ2RtaHA4cjFYdlhTT1FMQ1Aw?=
 =?utf-8?B?Y1k2amJiRnB1azRNQnNpbXVWaFA2QU5mQjVLWjArMk51bFROei9XUjdPWUNv?=
 =?utf-8?B?QVduSW90bHJqYmpDeHBvZE5SdnNaVGlDUkRaTlRMT1YxbUc3NUZvVDR0cUpq?=
 =?utf-8?B?dUFBNDJhdWp5OEo3eDNPMUdmTHdxTlFtMUxFOWl1UUN6dXdabC9zMzVKemVx?=
 =?utf-8?B?U1h0WWdmckd6LzBSMXpURVpYRnh3RTBScWpMWmJra2ZNSm04dVV2WFpYV0tD?=
 =?utf-8?B?eGp3OEtBTU9yR3RYRC9kRUR4SUU4Tys4dEYzV1lzNlJrM0kzN2RlbXUzV1Zm?=
 =?utf-8?B?aDFKWmJsb00rcDRRV1JFdjh4QjE1ZUd2bnFLM1oxTFh6M2JKVlJCdlJHM2Z0?=
 =?utf-8?B?dHpNMnZxaVVtWTZxdlRVUHAyQ29XMWh4TStySWZ4a2hCbGllR0h3SWNQczNp?=
 =?utf-8?B?Vnk2Znk4OHhmb3RxVWpyV3F2Sm5nSnRLUUdzQzd0ZGtieCtkTnRsUVBwREhJ?=
 =?utf-8?B?UG9yUkZzejZRY3AvV3BReWFkWW9BRVcxZWVMRmdMMURJcTRjN1dnRXl6a0VZ?=
 =?utf-8?B?dGZMMGVXbkRBWlZ4MkpCeXd4OHd1TzcrenNlVEVoQTVzQmx4SUdxV1RqVXBk?=
 =?utf-8?B?SzRTdTBpWVpQRXdGeUp1UTNiVzh4QWIvUnFRZmE1WUNaRkRXNThMTjZpd2c4?=
 =?utf-8?B?aGwvMUxPVEtlTXJSSDJ0QWc1czdtQlpKUFNKR2luN1FlNHhSWDZzeEFaN3No?=
 =?utf-8?B?dmZKdG1sMFBBYXg5TlFBalFkRExScVJMdlNSbEpvMGFkU01aTnNtYmlvcGk5?=
 =?utf-8?B?Vk1WRkdFV0FHaEE1cSt5dXAvSTRtdGVaQVNWSTFJYmE5VGVTRzkzRVd0aVdS?=
 =?utf-8?B?WUpXUHU4ZlZLWTQzaWQzQkRTT1Ric0cxdWpLd2RaVk1kSXJ1M3FocmVDOUh0?=
 =?utf-8?B?MnBZc0FqZGpxc3dKdFRldjBZYnJsYWxLYXp3Ry9UUGFvcXNtMWUxTEcweFdH?=
 =?utf-8?B?bWdjcUZjTXZ1Snp5MTFOUFdvaVNucmZZWmJaSHd5b3NxUldMY29tVjIxV3BO?=
 =?utf-8?B?R3dqbW5lMDk0M3c0TGZEU0d4TkdES2RPMk9MckZ0SnJ1bzhPaXhQaUl3RUdx?=
 =?utf-8?B?aGVaWjRTYlFwL0xLZEh4cmsrSTU4TlNGbVQwOHFueFdvRU9Bc0tOd2JRTFVx?=
 =?utf-8?B?VDR1WXhiS1JlYmgwSjQ0TEFXcHR6LzFLdVNCZDdrMjVlbElvT2dWZ3hOSFVZ?=
 =?utf-8?B?ZHJoOHlxeUxMSytxRmlMMVpyL292azhlQ21ROVhxcytUaDZFOWZKdDY1T05D?=
 =?utf-8?B?L0Fhd1VSMVNxYU1GQUk5MFlFc1BzbllFUlBBekZiWFZMd1MwNzVnM2RCeTU2?=
 =?utf-8?B?eGJ6ZzEvS3JVQ0t2UXZmVXE2eFNoOEZDMEhSYzQySFZaMWRTdi9mRzJBdDVs?=
 =?utf-8?B?S25YVmx4OGdLWHN2SG9mZVhYdFd3Tm1RSWZXNzlKSGxUL3Q4M2hlWjM5WFoy?=
 =?utf-8?B?dW5kaWVOTThRd1V0dVFjVitvSG9IaGs0MmZpS2VXa2JIbWJuMkc3Q2FmRjc5?=
 =?utf-8?B?Qm56eHlnL3Q2UmpyaStCZGlvelZmQmFEZTdYdXVsT1h3VVVtcnJpejJSSnZZ?=
 =?utf-8?B?UCtPNlZzblJCKzV6Tm9vR0VLRkRkMEJzT0lBMUFQd0ZTck8zZXA0MG12bTBp?=
 =?utf-8?B?S0Y5dDc0SDh4NlBDdXByMVdlb0JJN1FFOVY1Tk9pZS9vc3M1MjlldUxhd09G?=
 =?utf-8?Q?AokQK3JnxNyKiSYTcyVMpzQJS?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 685eadf2-0c7d-4744-c206-08db23bea986
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2023 12:30:00.2072
 (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: V9C9UIo2mjugIfl1biaB1Q8kjQ0fcgpDt1ZoaW2jVQ7lTGxRnc/jwbUyCLLw38s+xN2/k81f0MKVRTjzWN9h3w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4091
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Peter,

On 12-Mar-23 8:24 PM, Peter Zijlstra wrote:
> On Thu, Mar 09, 2023 at 03:41:10PM +0530, Ravi Bangoria wrote:
>> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
>> index 8c45b198b62f..81d67b899371 100644
>> --- a/arch/x86/events/amd/core.c
>> +++ b/arch/x86/events/amd/core.c
>> @@ -371,10 +371,15 @@ static inline int amd_has_nb(struct cpu_hw_events *cpuc)
>>  static int amd_pmu_hw_config(struct perf_event *event)
>>  {
>>  	int ret;
>> +	u64 dummy;
>>  
>> -	/* pass precise event sampling to ibs: */
>> -	if (event->attr.precise_ip && get_ibs_caps())
>> -		return -ENOENT;
>> +	if (event->attr.precise_ip) {
>> +		/* pass precise event sampling to ibs by returning -ESRCH */
>> +		if (get_ibs_caps() && !ibs_core_pmu_event(event, &dummy))
>> +			return -ESRCH;
>> +		else
>> +			return -ENOENT;
>> +	}
>>  
>>  	if (has_branch_stack(event) && !x86_pmu.lbr_nr)
>>  		return -EOPNOTSUPP;
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index f79fd8b87f75..e990c71ba34a 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -11639,18 +11639,26 @@ static struct pmu *perf_init_event(struct perf_event *event)
>>  			goto again;
>>  		}
>>  
>> +		/*
>> +		 * pmu->event_init() should return -ESRCH only when it
>> +		 * wants to forward the event to other pmu.
>> +		 */
>> +		if (ret == -ESRCH)
>> +			goto try_all;
>> +
>>  		if (ret)
>>  			pmu = ERR_PTR(ret);
>>  
>>  		goto unlock;
>>  	}
>>  
>> +try_all:
>>  	list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
>>  		ret = perf_try_init_event(pmu, event);
>>  		if (!ret)
>>  			goto unlock;
>>  
>> -		if (ret != -ENOENT) {
>> +		if (ret != -ENOENT && ret != -ESRCH) {
>>  			pmu = ERR_PTR(ret);
>>  			goto unlock;
>>  		}
> 
> Urgh.. So amd_pmu_hw_config() knows what PMU it should be but has no
> real way to communicate this, so you make it try all of them again.
> 
> Now, we already have a gruesome hack in there, and I'm thikning you
> should use that instead of adding yet another one. Note:
> 
> 		if (ret == -ENOENT && event->attr.type != type && !extended_type) {
> 			type = event->attr.type;
> 			goto again;
> 
> So if you have amd_pmu_hw_config() do:
> 
> 	event->attr.type = ibs_pmu.type;
> 	return -ENOENT;
> 
> it should all just work no?

IBS driver needs to convert RAW pmu config to IBS config, which it does
based on original event->attr.type. See perf_ibs_precise_event(). This
logic will fail with event->attr.type overwrite.

> 
> And now thinking about this, I'm thinking we can clean up the whole
> swevent mess too, a little something like the below perhaps... Then it
> might just be possible to remove that list_for_each_entry_rcu()
> entirely.
> 
> Hmm?

I'll check this and revert back.

> 
> 
> ---
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f79fd8b87f75..26130d1ca40b 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9951,6 +9951,9 @@ static void sw_perf_event_destroy(struct perf_event *event)
>  	swevent_hlist_put();
>  }
>  
> +static struct pmu perf_cpu_clock; /* fwd declaration */
> +static struct pmu perf_task_clock;
> +
>  static int perf_swevent_init(struct perf_event *event)
>  {
>  	u64 event_id = event->attr.config;
> @@ -9966,7 +9969,11 @@ static int perf_swevent_init(struct perf_event *event)
>  
>  	switch (event_id) {
>  	case PERF_COUNT_SW_CPU_CLOCK:
> +		event->attr.type = perf_cpu_clock.type;
> +		return -ENOENT;
> +
>  	case PERF_COUNT_SW_TASK_CLOCK:
> +		event->attr.type = perf_task_clock.type;
>  		return -ENOENT;
>  
>  	default:

Thanks,
Ravi