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: <ea39ef1d-44a3-478d-f6fb-2120be67e745@amd.com>
Date:   Fri, 18 Mar 2022 13:32:04 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
From:   Sandipan Das <sandipan.das@amd.com>
Subject: Re: [PATCH 5/7] perf/x86/amd/core: Add PerfMonV2 counter control
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        x86@kernel.org, bp@alien8.de, dave.hansen@linux.intel.com,
        acme@kernel.org, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, namhyung@kernel.org,
        jolsa@kernel.org, tglx@linutronix.de, mingo@redhat.com,
        pbonzini@redhat.com, jmattson@google.com, like.xu.linux@gmail.com,
        eranian@google.com, ananth.narayan@amd.com, ravi.bangoria@amd.com,
        santosh.shukla@amd.com
References: <cover.1647498015.git.sandipan.das@amd.com>
 <7958e729c6be0f682379bec81f115b8cd7cca1ad.1647498015.git.sandipan.das@amd.com>
 <YjMfe7KyKp+a3Jkp@hirez.programming.kicks-ass.net>
Content-Language: en-US
In-Reply-To: <YjMfe7KyKp+a3Jkp@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?UnhmS2JjTi9CTVArTVBZNzNrSndIdFZRUUlNaVQ2bFNkRjBKMTB1NWlaZjc2?=
 =?utf-8?B?aEovN29wMFVkU1dnT1h2RTVnZTdKSjhUSXMrdDB2Q0cva2UvdHByOHRaV1Uv?=
 =?utf-8?B?ZkExdHJlbUZaZEVVaUR3ZUw3bUFVY0Z3a3JNZ282ZENIamdUN1B6Ynd2ODZH?=
 =?utf-8?B?WVdhUFNoREt5MnFxL2xzblcyM0R1YktDdlljeFhqTjhWSGdOR00wYUloUU43?=
 =?utf-8?B?NTNPZUJrQ3JaWlNLbWtjamZPMHFzRklFdEFUREtTalhrNmRrRy8wYUJFMDRz?=
 =?utf-8?B?YWl0N21WSURCVjBWWVdPdVBob2pDS1k3clRCQjd4UmgwVkExcXRkNUoxaE5j?=
 =?utf-8?B?YkhTWjAvdjhkQUJvSXBPQlIyNWJ6d2hFajQxMWZORHp5bWxObzRET2lTMlRV?=
 =?utf-8?B?UmlqdDk3Z1Erc3RnY2c4K3E3VmJkeDd1eWJYWFNpMmErWlEwOGdMUzdQRXl1?=
 =?utf-8?B?MHZWMEVPWlBJMVdhVTRBdmc5c1NRYk5Kay9QUmJCZjg5SXdTYktick83aXAz?=
 =?utf-8?B?SzNINnlCTGZIMWpPdFVTZFZlSlZkM0F0Zy9QTnllQmJnRVJTVVJOaXprODll?=
 =?utf-8?B?MFU2Szg0QkJYVlUwTG1RcWxlZExkTmtUb0F5b0tZWjdaVzRyUzk2ZW4xNlFF?=
 =?utf-8?B?U0J2NlhOWlVBci94RWJqeFNUSENBcEl4d0VQODV4NTQxd085c2hDRWVrN1Vn?=
 =?utf-8?B?dnQ4L2ZrUzVmSG03d2U5VkVUcGgrVFpiVlVCKzBmVVdld0tlZEMrZDJERnp6?=
 =?utf-8?B?RmRHQzFVVTJxL3poa0FOKzhNTVVUaFlLOWhjMUJuK3MvUUdvNG45S2dlUDk4?=
 =?utf-8?B?WlY0SWU3emU4MVh0V0d4RXRtclEvNHB3WEdjSkZkSTFIWVRqWkNjWnBxeU95?=
 =?utf-8?B?Q1d5ZWF6dmltUVFlK0lxU3ZHYktiK0xMN1NzR3RDY3lUczZCWGx0MTNrb1Zi?=
 =?utf-8?B?YjcrbHVmM05yWkcyMkg1SEhrNUZHZ2JzQlF0dkN2SkhJeFJYYUpHWE5Ed0VO?=
 =?utf-8?B?SEF5UHVHZE56UHBxNXVBMEFmaDlWSndxVEhxb25DZmp1L243SkxwRmp6VDRi?=
 =?utf-8?B?djNjbzhpU2VLUGZ5TytyeFFtSklsODlVOU9CaS9IYWNHT1IvZnNpb3RPaWxi?=
 =?utf-8?B?a00wR2Q4S0lYQ2VLWmhnUkFYT2tKZ1NOYURhelA4czJabmNidHJYR0JqNFBq?=
 =?utf-8?B?R05FYlJoTUhCTG9QM3BVVlN3OElNNXh4YkhEOFpZYUowdE9ISUxzNU9vL3RV?=
 =?utf-8?B?bjdNUUtlL25sZzBQV3ArWFVMZzlPT2cwcnRRY3ErS2NMakx1ZXR2VlQrNHBh?=
 =?utf-8?B?MXJCanF0TEZ1RmZpcGpJMVQrK24vb3Z3WlBGUFFoamViRzBGNG9zbWxBanZy?=
 =?utf-8?B?Qk53MC91OWxhREN4M3dlb1Y1dVBHUEdHU3RQdkJLaG4weWNFWkp3aDIxZlFQ?=
 =?utf-8?B?OFJRc09CZkJsQkVlNWkrREIyK2NPb0kxOXdVaXA2anFnVVlrU3lxNkRBcDRr?=
 =?utf-8?B?Y2ZVVlFLc2RHV0pJMCtnei9NUnpYQlhnaUkvMTVQMWtIb0V5RHdGU0lLeExn?=
 =?utf-8?B?K3Bkc2tyeHdpNkc1ZXRYcWR4VzEwaURVMllSandjckw3MFNtbVpDK29xdlFw?=
 =?utf-8?B?NWFnTGRQbTNyVXNhL3F5Mmo1Rm9aZjI5c1l5UVVpbEV4WGZtcWRBeVF4eVUr?=
 =?utf-8?B?STIrUExwb0gzdGI0ZGJlek5YZHVMcmFlSTk0K2ZxbjV0UGVZOFhLelRjbVFV?=
 =?utf-8?B?UkxZL3V1b01UYllIMlpPSXl5aDMvNzlnTHAvaGZENzhwQ0xzQjBUT2dKS1VQ?=
 =?utf-8?B?Y3puaUtWa3ZiU0Q2dXRnR2ZXV05uY3owNllJc3ZNWXo0aTBGS3NHQnlJOXdZ?=
 =?utf-8?B?dW5Wc1RORWtQSngrVGZ2UlV1b1NXNlV0V3cxRzZCR21DQXRQeWJKa0VsRlB4?=
 =?utf-8?Q?M3n8qGtVmZjCm9W3TwzKFucPK4EQyild?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f957b08a-4f71-4e1e-2454-08da08b5a3d6
X-MS-Exchange-CrossTenant-AuthSource: BN6PR1201MB0178.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Mar 2022 08:02:26.0294
 (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: 5zZ4ZWkkLU642SrQjMU2zFhNdvg7YQJYzzrAjq/5DoxPMNlPZmg4B1tjo4YIWsQUkpbd6zdSw3I0eROmLywn6A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB2634
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/17/2022 5:16 PM, Peter Zijlstra wrote:
> On Thu, Mar 17, 2022 at 11:58:34AM +0530, Sandipan Das wrote:
>> @@ -625,12 +630,32 @@ static void amd_pmu_wait_on_overflow(int idx)
>>  	}
>>  }
>>  
>> +static void amd_pmu_global_enable_all(int added)
>> +{
>> +	amd_pmu_set_global_ctl(amd_pmu_global_cntr_mask);
>> +}
>> +
>> +DEFINE_STATIC_CALL(amd_pmu_enable_all, x86_pmu_enable_all);
>> +
>> +static void amd_pmu_enable_all(int added)
>> +{
>> +	static_call(amd_pmu_enable_all)(added);
>> +}
>> +
>> +static void amd_pmu_global_disable_all(void)
>> +{
>> +	/* Disable all PMCs */
>> +	amd_pmu_set_global_ctl(0);
>> +}
>> +
>> +DEFINE_STATIC_CALL(amd_pmu_disable_all, x86_pmu_disable_all);
>> +
>>  static void amd_pmu_disable_all(void)
>>  {
>>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>  	int idx;
>>  
>> -	x86_pmu_disable_all();
>> +	static_call(amd_pmu_disable_all)();
>>  
>>  	/*
>>  	 * This shouldn't be called from NMI context, but add a safeguard here
>> @@ -671,6 +696,28 @@ static void amd_pmu_disable_event(struct perf_event *event)
>>  	amd_pmu_wait_on_overflow(event->hw.idx);
>>  }
>>  
>> +static void amd_pmu_global_enable_event(struct perf_event *event)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +
>> +	/*
>> +	 * Testing cpu_hw_events.enabled should be skipped in this case unlike
>> +	 * in x86_pmu_enable_event().
>> +	 *
>> +	 * Since cpu_hw_events.enabled is set only after returning from
>> +	 * x86_pmu_start(), the PMCs must be programmed and kept ready.
>> +	 * Counting starts only after x86_pmu_enable_all() is called.
>> +	 */
>> +	__x86_pmu_enable_event(hwc, ARCH_PERFMON_EVENTSEL_ENABLE);
>> +}
>> +
>> +DEFINE_STATIC_CALL(amd_pmu_enable_event, x86_pmu_enable_event);
>> +
>> +static void amd_pmu_enable_event(struct perf_event *event)
>> +{
>> +	static_call(amd_pmu_enable_event)(event);
>> +}
>> +
>>  /*
>>   * Because of NMI latency, if multiple PMC counters are active or other sources
>>   * of NMIs are received, the perf NMI handler can handle one or more overflowed
>> @@ -929,8 +976,8 @@ static __initconst const struct x86_pmu amd_pmu = {
>>  	.name			= "AMD",
>>  	.handle_irq		= amd_pmu_handle_irq,
>>  	.disable_all		= amd_pmu_disable_all,
>> -	.enable_all		= x86_pmu_enable_all,
>> -	.enable			= x86_pmu_enable_event,
>> +	.enable_all		= amd_pmu_enable_all,
>> +	.enable			= amd_pmu_enable_event,
>>  	.disable		= amd_pmu_disable_event,
>>  	.hw_config		= amd_pmu_hw_config,
>>  	.schedule_events	= x86_schedule_events,
>> @@ -989,6 +1036,11 @@ static int __init amd_core_pmu_init(void)
>>  		x86_pmu.num_counters = EXT_PERFMON_DEBUG_NUM_CORE_PMC(ebx);
>>  
>>  		amd_pmu_global_cntr_mask = (1ULL << x86_pmu.num_counters) - 1;
>> +
>> +		/* Update PMC handling functions */
>> +		static_call_update(amd_pmu_enable_all, amd_pmu_global_enable_all);
>> +		static_call_update(amd_pmu_disable_all, amd_pmu_global_disable_all);
>> +		static_call_update(amd_pmu_enable_event, amd_pmu_global_enable_event);
>>  	}
> 
> 
> This makes no sense to me...
> 
> First and foremost, *please* tell me your shiny new hardware fixed the
> terrible behaviour that requires the wait_on_overflow hacks in
> amd_pmu_disable_all().

Unfortunately, that workaround is still required.

> 
> Second, all these x86_pmu methods are already static_calls per
> arch/x86/events/core.c. So what you want to do is something like:
> 
> 	x86_pmu = amd_pmu;
> 	if (amd_v2) {
> 		x86_pmu.disable_all = amd_v2_disable_all;
> 		x86_pmu.enable_all  = amd_v2_enable_all;
> 	}
> 
> And leave it at that.
> 

Sure. I'll clean this up.

- Sandipan