Hi Nathan,
On 5/6/19 12:14 PM, Nathan Lynch wrote:
Hi Juliet,
Juliet Kim<juli...@linux.vnet.ibm.com> writes:
Fix extending start/stop topology update scope during LPM
Commit 65b9fdadfc4d ("powerpc/pseries/mobility: Extend start/stop
topology update scope") made the change to the duration that
topology updates are suppressed during LPM to allow the complete
device tree update which leaves the property update notifier
unregistered until device tree update completes. This prevents
topology update during LPM.
Instead, use mutex_lock, which serializes LPM and PRRN operation
in pseries_devicetree_update.
I think this is conflating two issues:
1. Insufficient serialization/ordering of handling PRRNs and
LPM. E.g. we could migrate while processing a PRRN from the source
system and end up with incorrect contents in the device tree on the
destination if the LPM changes the same nodes. The OS is supposed to
drain any outstanding PRRNs before proceeding with migration, which
is a stronger requirement than simple serialization of device tree
updates. If we don't impose this ordering already we should fix that.
PRRN request can be received at any time including before/after LPM and
during LPM. Currently, we do not have a protocol with hypervisor
prohibiting
PRRN after LPM begins. This patch is to fix the regression(inconsistent
state
of device tree and skipping CPU affinity update) injected by a patch
Commit 65b9fdadfc4d (Extending start/stop topology update scope during
LPM ).
This patch uses mutex_lock to update device tree allowing device tree to be
consistent state in both cases : LPM begins while PRRN event is running and
vice versa. If we migrate while PRRN is running at source, PRRN holding
the lock
completes at target. Once PRRN release the lock, LPM take the lock and
update
device tree. PRRN completes device tree update before LPM begins.
To avoid PRRN and LPM from running at the same time, it needs serialization
at the higher layer which requires design change and may be future work.
2. The NUMA topology update processing. Generally speaking,
start/stop_topology_update() enable/disable dt_update_callback(),
which we use to update CPU-node assignments. Since we now know that
doing that is Bad, it's sort of a happy accident that
migration_store() was changed to re-register the notifier after
updating the device tree, which is too late. So I don't think we
should try to "fix" this. Instead we should remove the broken code
(dt_update_callback -> dlpar_cpu_readdd and so on).
When the regression (CPU affinity update has been accidentally disabled
at LPM)
and CPU readd causes some issues, I suggested that we revert the CPU
readd patch
already upstream and leave the regression without fixing. But then, we
decided to
disable CPU affinity update globally for LPM, PRRN, VPHN and fix the
regression
once the disablement CPU affinity update patch is accepted upstream as the
regression needs to be corrected in case of enabling CPU affinity update
and we
would learn up codes once the disablement is stabilized.
Do you agree?
Thanks,
Nathan
