Re: [PATCH]powerpc/mobility: Serialize PRRN and LPM in device tree update

[Juliet Kim <julietk@linux.vnet.ibm.com>]

Hi Nathan,

On 5/6/19 12:14 PM, Nathan Lynch wrote:
> Hi Juliet,
>
> Juliet Kim<julietk@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