Re: [PATCH V4 0/1] powerpc/perf: Clear pending PMI in ppmu callbacks

From: Nicholas Piggin <npiggin@gmail.com>
To: Athira Rajeev <atrajeev@linux.vnet.ibm.com>, mpe@ellerman.id.au
Cc: maddy@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
	rnsastry@linux.ibm.com
Subject: Re: [PATCH V4 0/1] powerpc/perf: Clear pending PMI in ppmu callbacks
Date: Tue, 23 Nov 2021 15:24:00 +1000	[thread overview]
Message-ID: <1637644730.h2erjs9iz0.astroid@bobo.none> (raw)
In-Reply-To: <40F23366-66BE-4063-8516-445E41E19EC6@linux.vnet.ibm.com>

Excerpts from Athira Rajeev's message of November 20, 2021 12:36 am:
> 
> 
>> On 21-Jul-2021, at 11:18 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> Running perf fuzzer testsuite popped up below messages
>> in the dmesg logs:
>> 
>> "Can't find PMC that caused IRQ"
>> 
>> This means a PMU exception happened, but none of the PMC's (Performance
>> Monitor Counter) were found to be overflown. Perf interrupt handler checks
>> the PMC's to see which PMC has overflown and if none of the PMCs are
>> overflown ( counter value not >= 0x80000000 ), it throws warning:
>> "Can't find PMC that caused IRQ".
>> 
>> Powerpc has capability to mask and replay a performance monitoring
>> interrupt (PMI). In case of replayed PMI, there are some corner cases
>> that clears the PMCs after masking. In such cases, the perf interrupt
>> handler will not find the active PMC values that had caused the overflow
>> and thus leading to this message. This patchset attempts to fix those
>> corner cases.
>> 
>> However there is one more case in PowerNV where these messages are
>> emitted during system wide profiling or when a specific CPU is monitored
>> for an event. That is, when a counter overflow just before entering idle
>> and a PMI gets triggered after wakeup from idle. Since PMCs
>> are not saved in the idle path, perf interrupt handler will not
>> find overflown counter value and emits the "Can't find PMC" messages.
>> This patch documents this race condition in powerpc core-book3s.
>> 
>> Patch fixes the ppmu callbacks to disable pending interrupt before clearing
>> the overflown PMC and documents the race condition in idle path.
>> 
>> Changelog:
>> changes from v3 -> v4
>>   Addressed review comments from Nicholas Piggin
>>   - Added comment explaining the need to clear MMCR0 PMXE bit in
>>     pmu disable callback.
>>   - Added a check to display warning if there is a PMI pending
>>     bit set in Paca without any overflown PMC.
>>   - Removed the condition check before clearing pending PMI
>>     in 'clear_pmi_irq_pending' function.
>>   - Added reviewed by from Nicholas Piggin.
>> 
>> Changes from v2 -> v3
>>   Addressed review comments from Nicholas Piggin
>>   - Moved the clearing of PMI bit to power_pmu_disable.
>>     In previous versions, this was done in power_pmu_del,
>>     power_pmu_stop/enable callbacks before clearing of PMC's.
>>   - power_pmu_disable is called before any event gets deleted
>>     or stopped. If more than one event is running in the PMU,
>>     we may clear the PMI bit for an event which is not going
>>     to be deleted/stopped. Hence introduced check in
>>     power_pmu_enable to set back PMI to avoid dropping of valid
>>     samples in such cases.
>>   - Disable MMCR0 PMXE bit in pmu disable callback which otherwise
>>     could trigger PMI when PMU is getting disabled.
>> Changes from v1 -> v2
>>   Addressed review comments from Nicholas Piggin
>>   - Moved the PMI pending check and clearing function
>>     to arch/powerpc/include/asm/hw_irq.h and renamed
>>     function to "get_clear_pmi_irq_pending"
>>   - Along with checking for pending PMI bit in Paca,
>>     look for PMAO bit in MMCR0 register to decide on
>>     pending PMI interrupt.
>> 
>> Athira Rajeev (1):
>>  powerpc/perf: Fix PMU callbacks to clear pending PMI before resetting
>>    an overflown PMC
> 
> Hi,
> 
> Please let me know if there are any review comments for this patch.
> 
> Thanks
> Athira

It seems good to me. It already has my R-B. Would be good if we can
get this one merged.

Thanks,
Nick