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

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

[Juilet Kim]

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.

Signed-off-by: Juliet Kim <julietk@linux.ibm.com>

 arch/powerpc/platforms/pseries/mobility.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..3a79ded056fd 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -42,6 +42,8 @@ struct update_props_workarea {
 #define MIGRATION_SCOPE	(1)
 #define PRRN_SCOPE -2
 
+static DEFINE_MUTEX(dt_affinity_mutex);
+
 static int mobility_rtas_call(int token, char *buf, s32 scope)
 {
 	int rc;
@@ -270,13 +272,19 @@ int pseries_devicetree_update(s32 scope)
 	int update_nodes_token;
 	int rc;
 
+	mutex_lock(&dt_affinity_mutex);
+
 	update_nodes_token = rtas_token("ibm,update-nodes");
-	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
+	if (update_nodes_token == RTAS_UNKNOWN_SERVICE) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -EINVAL;
+	}
 
 	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
+	if (!rtas_buf) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -ENOMEM;
+	}
 
 	do {
 		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
@@ -316,6 +324,7 @@ int pseries_devicetree_update(s32 scope)
 	} while (rc == 1);
 
 	kfree(rtas_buf);
+	mutex_unlock(&dt_affinity_mutex);
 	return rc;
 }
 
@@ -371,10 +380,10 @@ static ssize_t migration_store(struct class *class,
 	if (rc)
 		return rc;
 
-	post_mobility_fixup();
-
 	start_topology_update();
 
+	post_mobility_fixup();
+
 	return count;
 }
 
-- 
2.12.3

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

[Juilet Kim]

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.

Signed-off-by: Juliet Kim <julietk@linux.ibm.com>

 arch/powerpc/platforms/pseries/mobility.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..3a79ded056fd 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -42,6 +42,8 @@ struct update_props_workarea {
 #define MIGRATION_SCOPE	(1)
 #define PRRN_SCOPE -2
 
+static DEFINE_MUTEX(dt_affinity_mutex);
+
 static int mobility_rtas_call(int token, char *buf, s32 scope)
 {
 	int rc;
@@ -270,13 +272,19 @@ int pseries_devicetree_update(s32 scope)
 	int update_nodes_token;
 	int rc;
 
+	mutex_lock(&dt_affinity_mutex);
+
 	update_nodes_token = rtas_token("ibm,update-nodes");
-	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
+	if (update_nodes_token == RTAS_UNKNOWN_SERVICE) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -EINVAL;
+	}
 
 	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
+	if (!rtas_buf) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -ENOMEM;
+	}
 
 	do {
 		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
@@ -316,6 +324,7 @@ int pseries_devicetree_update(s32 scope)
 	} while (rc == 1);
 
 	kfree(rtas_buf);
+	mutex_unlock(&dt_affinity_mutex);
 	return rc;
 }
 
@@ -371,10 +380,10 @@ static ssize_t migration_store(struct class *class,
 	if (rc)
 		return rc;
 
-	post_mobility_fixup();
-
 	start_topology_update();
 
+	post_mobility_fixup();
+
 	return count;
 }
 
-- 
2.12.3

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

[Juliet Kim]

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.

Signed-off-by: Juliet Kim <julietk@linux.ibm.com>

 arch/powerpc/platforms/pseries/mobility.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..3a79ded056fd 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -42,6 +42,8 @@ struct update_props_workarea {
 #define MIGRATION_SCOPE	(1)
 #define PRRN_SCOPE -2
 
+static DEFINE_MUTEX(dt_affinity_mutex);
+
 static int mobility_rtas_call(int token, char *buf, s32 scope)
 {
 	int rc;
@@ -270,13 +272,19 @@ int pseries_devicetree_update(s32 scope)
 	int update_nodes_token;
 	int rc;
 
+	mutex_lock(&dt_affinity_mutex);
+
 	update_nodes_token = rtas_token("ibm,update-nodes");
-	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
+	if (update_nodes_token == RTAS_UNKNOWN_SERVICE) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -EINVAL;
+	}
 
 	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
+	if (!rtas_buf) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -ENOMEM;
+	}
 
 	do {
 		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
@@ -316,6 +324,7 @@ int pseries_devicetree_update(s32 scope)
 	} while (rc == 1);
 
 	kfree(rtas_buf);
+	mutex_unlock(&dt_affinity_mutex);
 	return rc;
 }
 
@@ -371,10 +380,10 @@ static ssize_t migration_store(struct class *class,
 	if (rc)
 		return rc;
 
-	post_mobility_fixup();
-
 	start_topology_update();
 
+	post_mobility_fixup();
+
 	return count;
 }
 
-- 
2.12.3

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

[Juliet Kim]

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.

Signed-off-by: Juliet Kim <julietk@linux.ibm.com>

 arch/powerpc/platforms/pseries/mobility.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..3a79ded056fd 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -42,6 +42,8 @@ struct update_props_workarea {
 #define MIGRATION_SCOPE	(1)
 #define PRRN_SCOPE -2
 
+static DEFINE_MUTEX(dt_affinity_mutex);
+
 static int mobility_rtas_call(int token, char *buf, s32 scope)
 {
 	int rc;
@@ -270,13 +272,19 @@ int pseries_devicetree_update(s32 scope)
 	int update_nodes_token;
 	int rc;
 
+	mutex_lock(&dt_affinity_mutex);
+
 	update_nodes_token = rtas_token("ibm,update-nodes");
-	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
+	if (update_nodes_token == RTAS_UNKNOWN_SERVICE) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -EINVAL;
+	}
 
 	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
+	if (!rtas_buf) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -ENOMEM;
+	}
 
 	do {
 		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
@@ -316,6 +324,7 @@ int pseries_devicetree_update(s32 scope)
 	} while (rc == 1);
 
 	kfree(rtas_buf);
+	mutex_unlock(&dt_affinity_mutex);
 	return rc;
 }
 
@@ -371,10 +380,10 @@ static ssize_t migration_store(struct class *class,
 	if (rc)
 		return rc;
 
-	post_mobility_fixup();
-
 	start_topology_update();
 
+	post_mobility_fixup();
+
 	return count;
 }
 
-- 
2.12.3

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

[Juliet Kim]

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.

Signed-off-by: Juliet Kim <julietk@linux.ibm.com>

 arch/powerpc/platforms/pseries/mobility.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..3a79ded056fd 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -42,6 +42,8 @@ struct update_props_workarea {
 #define MIGRATION_SCOPE	(1)
 #define PRRN_SCOPE -2
 
+static DEFINE_MUTEX(dt_affinity_mutex);
+
 static int mobility_rtas_call(int token, char *buf, s32 scope)
 {
 	int rc;
@@ -270,13 +272,19 @@ int pseries_devicetree_update(s32 scope)
 	int update_nodes_token;
 	int rc;
 
+	mutex_lock(&dt_affinity_mutex);
+
 	update_nodes_token = rtas_token("ibm,update-nodes");
-	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
+	if (update_nodes_token == RTAS_UNKNOWN_SERVICE) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -EINVAL;
+	}
 
 	rtas_buf = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-	if (!rtas_buf)
+	if (!rtas_buf) {
+		mutex_unlock(&dt_affinity_mutex);
 		return -ENOMEM;
+	}
 
 	do {
 		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
@@ -316,6 +324,7 @@ int pseries_devicetree_update(s32 scope)
 	} while (rc == 1);
 
 	kfree(rtas_buf);
+	mutex_unlock(&dt_affinity_mutex);
 	return rc;
 }
 
@@ -371,10 +380,10 @@ static ssize_t migration_store(struct class *class,
 	if (rc)
 		return rc;
 
-	post_mobility_fixup();
-
 	start_topology_update();
 
+	post_mobility_fixup();
+
 	return count;
 }
 
-- 
2.12.3

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

[Nathan Lynch]

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.

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).

Do you agree?


Thanks,
Nathan

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

[Juliet Kim]

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