Re: [PATCH V2] powerpc/perf: Enable PMU counters post partition migration if PMU is active

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 10238 bytes --]



> On 21-Oct-2021, at 4:24 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> Excerpts from Athira Rajeev's message of July 11, 2021 10:25 pm:
>> During Live Partition Migration (LPM), it is observed that perf
>> counter values reports zero post migration completion. However
>> 'perf stat' with workload continues to show counts post migration
>> since PMU gets disabled/enabled during sched switches. But incase
>> of system/cpu wide monitoring, zero counts were reported with 'perf
>> stat' after migration completion.
>> 
>> Example:
>> ./perf stat -e r1001e -I 1000
>>           time             counts unit events
>>     1.001010437         22,137,414      r1001e
>>     2.002495447         15,455,821      r1001e
>> <<>> As seen in next below logs, the counter values shows zero
>>        after migration is completed.
>> <<>>
>>    86.142535370    129,392,333,440      r1001e
>>    87.144714617                  0      r1001e
>>    88.146526636                  0      r1001e
>>    89.148085029                  0      r1001e
>> 
>> Here PMU is enabled during start of perf session and counter
>> values are read at intervals. Counters are only disabled at the
>> end of session. The powerpc mobility code presently does not handle
>> disabling and enabling back of PMU counters during partition
>> migration. Also since the PMU register values are not saved/restored
>> during migration, PMU registers like Monitor Mode Control Register 0
>> (MMCR0), Monitor Mode Control Register 1 (MMCR1) will not contain
>> the value it was programmed with. Hence PMU counters will not be
>> enabled correctly post migration.
>> 
>> Fix this in mobility code by handling disabling and enabling of
>> PMU in all cpu's before and after migration. Patch introduces two
>> functions 'mobility_pmu_disable' and 'mobility_pmu_enable'.
>> mobility_pmu_disable() is called before the processor threads goes
>> to suspend state so as to disable the PMU counters. And disable is
>> done only if there are any active events running on that cpu.
>> mobility_pmu_enable() is called after the processor threads are
>> back online to enable back the PMU counters.
>> 
>> Since the performance Monitor counters ( PMCs) are not
>> saved/restored during LPM, results in PMC value being zero and the
>> 'event->hw.prev_count' being non-zero value. This causes problem
> 
> Interesting. Are they defined to not be migrated, or may not be 
> migrated?
> 
> I wonder what QEMU migration does with PMU registers.
> 
>> during updation of event->count since we always accumulate
>> (event->hw.prev_count - PMC value) in event->count.  If
>> event->hw.prev_count is greater PMC value, event->count becomes
>> negative. Fix this by re-initialising 'prev_count' also for all
>> events while enabling back the events. A new variable 'migrate' is
>> introduced in 'struct cpu_hw_event' to achieve this for LPM cases
>> in power_pmu_enable. Use the 'migrate' value to clear the PMC
>> index (stored in event->hw.idx) for all events so that event
>> count settings will get re-initialised correctly.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> [ Fixed compilation error reported by kernel test robot ]
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>> Change from v1 -> v2:
>> - Moved the mobility_pmu_enable and mobility_pmu_disable
>>   declarations under CONFIG_PPC_PERF_CTRS in rtas.h.
>>   Also included 'asm/rtas.h' in core-book3s to fix the
>>   compilation warning reported by kernel test robot.
>> 
>> arch/powerpc/include/asm/rtas.h           |  8 ++++++
>> arch/powerpc/perf/core-book3s.c           | 44 ++++++++++++++++++++++++++++---
>> arch/powerpc/platforms/pseries/mobility.c |  4 +++
>> 3 files changed, 53 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
>> index 9dc97d2..cea72d7 100644
>> --- a/arch/powerpc/include/asm/rtas.h
>> +++ b/arch/powerpc/include/asm/rtas.h
>> @@ -380,5 +380,13 @@ static inline void rtas_initialize(void) { }
>> static inline void read_24x7_sys_info(void) { }
>> #endif
>> 
>> +#ifdef CONFIG_PPC_PERF_CTRS
>> +void mobility_pmu_disable(void);
>> +void mobility_pmu_enable(void);
>> +#else
>> +static inline void mobility_pmu_disable(void) { }
>> +static inline void mobility_pmu_enable(void) { }
>> +#endif
>> +
>> #endif /* __KERNEL__ */
>> #endif /* _POWERPC_RTAS_H */
> 
> It's not implemented in rtas, maybe consider putting this into a perf 
> header?

Hi Nick,
Thanks for the review comments.

Sure, I will move this to perf_event header file

> 
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index bb0ee71..90da7fa 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -18,6 +18,7 @@
>> #include <asm/ptrace.h>
>> #include <asm/code-patching.h>
>> #include <asm/interrupt.h>
>> +#include <asm/rtas.h>
>> 
>> #ifdef CONFIG_PPC64
>> #include "internal.h"
>> @@ -58,6 +59,7 @@ struct cpu_hw_events {
>> 
>> 	/* Store the PMC values */
>> 	unsigned long pmcs[MAX_HWEVENTS];
>> +	int migrate;
>> };
>> 
>> static DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
>> @@ -1335,6 +1337,22 @@ static void power_pmu_disable(struct pmu *pmu)
>> }
>> 
>> /*
>> + * Called from powerpc mobility code
>> + * before migration to disable counters
>> + * if the PMU is active.
>> + */
>> +void mobility_pmu_disable(void)
>> +{
>> +	struct cpu_hw_events *cpuhw;
>> +
>> +	cpuhw = this_cpu_ptr(&cpu_hw_events);
>> +	if (cpuhw->n_events != 0) {
>> +		power_pmu_disable(NULL);
>> +		cpuhw->migrate = 1;
>> +	}
> 
> Rather than add this migrate logic into power_pmu_enable(), would you be 
> able to do the work in the mobility callbacks instead? Something like
> this:
> 
> void mobility_pmu_disable(void)
> {
>        struct cpu_hw_events *cpuhw;
> 
>        cpuhw = this_cpu_ptr(&cpu_hw_events);
>        if (cpuhw->n_events != 0) {
>                int i;
> 
>                power_pmu_disable(NULL);
>                /*
>                 * Read off any pre-existing events because the register
>                 * values may not be migrated.
>                 */
>                for (i = 0; i < cpuhw->n_events; ++i) {
>                        event = cpuhw->event[i];
>                        if (event->hw.idx) {
>                                power_pmu_read(event);
>                                event->hw.idx = 0;
>                        }
>                }
>        }
> }
> 
> void mobility_pmu_enable(void)
> {
>        struct cpu_hw_events *cpuhw;
> 
>        cpuhw = this_cpu_ptr(&cpu_hw_events);
>        cpuhw->n_added = cpuhw->n_events;
>        power_pmu_enable(NULL);
> }
> 

Ok Nick, Will address these changes in next version.
With this approach, I won’t need the “migrate” field.
>> +}
>> +
>> +/*
>>  * Re-enable all events if disable == 0.
>>  * If we were previously disabled and events were added, then
>>  * put the new config on the PMU.
>> @@ -1379,8 +1397,10 @@ static void power_pmu_enable(struct pmu *pmu)
>> 	 * no need to recalculate MMCR* settings and reset the PMCs.
>> 	 * Just reenable the PMU with the current MMCR* settings
>> 	 * (possibly updated for removal of events).
>> +	 * While reenabling PMU during partition migration, continue
>> +	 * with normal flow.
>> 	 */
>> -	if (!cpuhw->n_added) {
>> +	if (!cpuhw->n_added && !cpuhw->migrate) {
>> 		mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
>> 		mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
>> 		if (ppmu->flags & PPMU_ARCH_31)
>> @@ -1434,11 +1454,15 @@ static void power_pmu_enable(struct pmu *pmu)
>> 	/*
>> 	 * Read off any pre-existing events that need to move
>> 	 * to another PMC.
>> +	 * While enabling PMU during partition migration,
>> +	 * skip power_pmu_read since all event count settings
>> +	 * needs to be re-initialised after migration.
>> 	 */
>> 	for (i = 0; i < cpuhw->n_events; ++i) {
>> 		event = cpuhw->event[i];
>> -		if (event->hw.idx && event->hw.idx != hwc_index[i] + 1) {
>> -			power_pmu_read(event);
>> +		if ((event->hw.idx && event->hw.idx != hwc_index[i] + 1) || (cpuhw->migrate)) {
>> +			if (!cpuhw->migrate)
>> +				power_pmu_read(event);
>> 			write_pmc(event->hw.idx, 0);
>> 			event->hw.idx = 0;
>> 		}
>> @@ -1506,6 +1530,20 @@ static void power_pmu_enable(struct pmu *pmu)
>> 	local_irq_restore(flags);
>> }
>> 
>> +/*
>> + * Called from powerpc mobility code
>> + * during migration completion to
>> + * enable back PMU counters.
>> + */
>> +void mobility_pmu_enable(void)
>> +{
>> +	struct cpu_hw_events *cpuhw;
>> +
>> +	cpuhw = this_cpu_ptr(&cpu_hw_events);
>> +	power_pmu_enable(NULL);
>> +	cpuhw->migrate = 0;
>> +}
>> +
>> static int collect_events(struct perf_event *group, int max_count,
>> 			  struct perf_event *ctrs[], u64 *events,
>> 			  unsigned int *flags)
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
>> index e83e089..ff7a77c 100644
>> --- a/arch/powerpc/platforms/pseries/mobility.c
>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>> @@ -476,6 +476,8 @@ static int do_join(void *arg)
>> retry:
>> 	/* Must ensure MSR.EE off for H_JOIN. */
>> 	hard_irq_disable();
>> +	/* Disable PMU before suspend */
>> +	mobility_pmu_disable();
>> 	hvrc = plpar_hcall_norets(H_JOIN);
>> 
>> 	switch (hvrc) {
> 
> Does the retry case cause mobility_pmu_disable to be called twice 
> without mobility_pmu_enable called? Would be neater if it was
> balanced.
> 

I will be moving these mobility callbacks to “pseries_migrate_partition” as Nathan suggested.
Will send a V3 with new changes. 

Thanks
Athira

> 
>> @@ -530,6 +532,8 @@ static int do_join(void *arg)
>> 	 * reset the watchdog.
>> 	 */
>> 	touch_nmi_watchdog();
>> +	/* Enable PMU after resuming */
>> +	mobility_pmu_enable();
>> 	return ret;
>> }
> 
> Not really a big deal but while you're updating it, you could leave 
> touch_nmi_watchdog() at the very end.
> 
> Thanks,
> Nick


[-- Attachment #2: Type: text/html, Size: 59543 bytes --]