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

Excerpts from Nathan Lynch's message of November 4, 2021 7:11 am:

Nicholas Piggin [off-list ref] writes:

quoted

Excerpts from Michael Ellerman's message of October 29, 2021 11:15 pm:

quoted

Nicholas Piggin [off-list ref] writes:

quoted

Excerpts from Athira Rajeev's message of October 29, 2021 1:05 pm:

quoted

@@ -631,12 +632,18 @@ static int pseries_migrate_partition(u64 handle)
 	if (ret)
 		return ret;
 
+	/* Disable PMU before suspend */
+	on_each_cpu(&mobility_pmu_disable, NULL, 0);

Why was this moved out of stop machine and to an IPI?

My concern would be, what are the other CPUs doing at this time? Is it 
possible they could take interrupts and schedule? Could that mess up the
perf state here?

pseries_migrate_partition() is called directly from migration_store(),
which is the sysfs store function, which can be called concurrently by
different CPUs.

It's also potentially called from rtas_syscall_dispatch_ibm_suspend_me(),
from sys_rtas(), again with no locking.

So we could have two CPUs calling into here at the same time, which
might not crash, but is unlikely to work well.

I think the lack of locking might have been OK in the past because only
one CPU will successfully get the other CPUs to call do_join() in
pseries_suspend(). But I could be wrong.

Anyway, now that we're mutating the PMU state before suspending we need
to be more careful. So I think we need a lock around the whole
sequence.

Regardless of the outcome here, generally agreed that some serialization
should be imposed in this path. The way the platform works (and some
extra measures by the drmgr utility) make it so that this code isn't
entered concurrently in usual operation, but it's possible to make it
happen if you are root.

A file-static mutex should be OK.

quoted

My concern is still that we wouldn't necessarily have the other CPUs 
under control at that point even if we serialize the migrate path.
They could take interrupts, possibly call into perf subsystem after
the mobility_pmu_disable (e.g., via syscall or context switch) which 
might mess things up.

I think the stop machine is a reasonable place for the code in this 
case. It's a low level disabling of hardware facility and saving off 
registers.

That makes sense, but I can't help feeling concerned still. For this to
be safe, power_pmu_enable() and power_pmu_disable() must never sleep or
re-enable interrupts or send IPIs. I don't see anything obviously unsafe
right now, but is that already part of their contract?

Yes that would have to be. That's much the same as an IPI handler. Maybe 
stop machine has a few other things to worry about but I'm not sure that
would get in the way. Just have a note in the implementation or name to
remind it is stop machine context.  

Is there much
risk they could change in the future to violate those constraints?

My guess is if it needed to do anything much more complicated then you
would want to shut down perf more completely in a way that other APIs
know about.

That aside, the proposed change seems like we would be hacking around a
more generic perf/pmu limitation in a powerpc-specific way. I see the
same behavior on x86 across suspend/resume.

Arguably it's fixing up a x86 suspend/resume bug that might be fixed
in the same way as this -- presumably it's not restoring PMU registers
at resume.

But if they did something generic in perf core, this code could be
changed over to use it quite easily I would hope.  But pinging the
other perf devs to check would be a good idea.

Thanks,
Nick