[PATCH v3 0/2] powerpc/pseries: Improve serialization of PRRN events

[PATCH v3 0/2] powerpc/pseries: Improve serialization of PRRN events

[John Allen]

Stress testing has uncovered issues with handling continuously queued PRRN
events. Running PRRN events in this way can seriously load the system given
the sheer volume of dlpar being handled. This patchset ensures that PRRN
events are handled more synchronously, only allowing the PRRN handler to
queue a single dlpar event at any given time.  Additionally, it ensures
that rtas polling continues normally when multiple PRRN events are queued
simultaneously.

v3:
  -Scrap the PRRN mutex as it only replicates existing workqueue behavior.
v2:
  -Unlock prrn_lock when PRRN operations are complete, not after handler is
   scheduled.
  -Remove call to flush_work, the previous broken method of serializing
   PRRN events.

John Allen (2):
  powerpc/pseries: Avoid blocking rtas polling handling multiple PRRN
    events
  powerpc/pseries: Wait for completion of hotplug events during PRRN
    handling

 arch/powerpc/kernel/rtasd.c               | 10 +++++++---
 arch/powerpc/platforms/pseries/mobility.c |  5 ++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] powerpc/pseries: Avoid blocking rtas polling handling multiple PRRN events

[John Allen]

When a PRRN event is being handled and another PRRN event comes in, the
second event will block rtas polling waiting on the first to complete,
preventing any further rtas events from being handled. This can be
especially problematic in case that PRRN events are continuously being
queued in which case rtas polling gets indefinitely blocked completely.

This patch removes the blocking call to flush_work and allows the
default workqueue behavior to handle duplicate events.

Signed-off-by: John Allen <jallen@linux.ibm.com>
---
v3:
  -Scrap the mutex as it only replicates existing workqueue behavior.
v2:
  -Unlock prrn_lock when PRRN operations are complete, not after handler is
   scheduled.
  -Remove call to flush_work, the previous broken method of serializing
   PRRN events.
---
 arch/powerpc/kernel/rtasd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
index 44d66c33d59d..2017934e5985 100644
--- a/arch/powerpc/kernel/rtasd.c
+++ b/arch/powerpc/kernel/rtasd.c
@@ -290,7 +290,6 @@ static DECLARE_WORK(prrn_work, prrn_work_fn);
 
 static void prrn_schedule_update(u32 scope)
 {
-	flush_work(&prrn_work);
 	prrn_update_scope = scope;
 	schedule_work(&prrn_work);
 }
-- 
2.17.1

[PATCH v3 2/2] powerpc/pseries: Wait for completion of hotplug events during PRRN handling

[John Allen]

While handling PRRN events, the time to handle the actual hotplug events
dwarfs the time it takes to perform the device tree updates and queue the
hotplug events. In the case that PRRN events are being queued continuously,
hotplug events have been observed to be queued faster than the kernel can
actually handle them. This patch avoids the problem by waiting for a
hotplug request to complete before queueing more hotplug events.

Signed-off-by: John Allen <jallen@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 8a8033a249c7..49930848fa78 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -242,6 +242,7 @@ static int add_dt_node(__be32 parent_phandle, __be32 drc_index)
 static void prrn_update_node(__be32 phandle)
 {
 	struct pseries_hp_errorlog *hp_elog;
+	struct completion hotplug_done;
 	struct device_node *dn;
 
 	/*
@@ -263,7 +264,9 @@ static void prrn_update_node(__be32 phandle)
 	hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
 	hp_elog->_drc_u.drc_index = phandle;
 
-	queue_hotplug_event(hp_elog, NULL, NULL);
+	init_completion(&hotplug_done);
+	queue_hotplug_event(hp_elog, &hotplug_done, NULL);
+	wait_for_completion(&hotplug_done);
 
 	kfree(hp_elog);
 }
-- 
2.17.1

Re: [PATCH v3 2/2] powerpc/pseries: Wait for completion of hotplug events during PRRN handling

[Nathan Fontenot]

On 08/08/2018 10:29 AM, John Allen wrote:
> While handling PRRN events, the time to handle the actual hotplug events
> dwarfs the time it takes to perform the device tree updates and queue the
> hotplug events. In the case that PRRN events are being queued continuously,
> hotplug events have been observed to be queued faster than the kernel can
> actually handle them. This patch avoids the problem by waiting for a
> hotplug request to complete before queueing more hotplug events.
> 
> Signed-off-by: John Allen <jallen@linux.ibm.com>

In the V2 thread it was mentioned that we could just call the DLPAR operation
directly instead of going through the workqueue. I have written a patch to do
this that also cleans up some of the request handling.

requests that come through the hotplug interrupt still use the workqueue. The
other requests, PRRN and sysfs, just call the dlpar handler directly. This
eliminates the need for a wait conditional and return code handling in the
workqueue handler and should solve the issue that John solves with his patch.

This still needs testing but wanted to get people's thoughts.

-Nathan

---
 arch/powerpc/platforms/pseries/dlpar.c    |   37 +++++++----------------------
 arch/powerpc/platforms/pseries/mobility.c |   18 +++++---------
 arch/powerpc/platforms/pseries/pseries.h  |    5 ++--
 arch/powerpc/platforms/pseries/ras.c      |    2 +-
 4 files changed, 19 insertions(+), 43 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index a0b20c03f078..052c4f2ba0a0 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -32,8 +32,6 @@ static struct workqueue_struct *pseries_hp_wq;
 struct pseries_hp_work {
 	struct work_struct work;
 	struct pseries_hp_errorlog *errlog;
-	struct completion *hp_completion;
-	int *rc;
 };
 
 struct cc_workarea {
@@ -329,7 +327,7 @@ int dlpar_release_drc(u32 drc_index)
 	return 0;
 }
 
-static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
+int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 {
 	int rc;
 
@@ -371,20 +369,13 @@ static void pseries_hp_work_fn(struct work_struct *work)
 	struct pseries_hp_work *hp_work =
 			container_of(work, struct pseries_hp_work, work);
 
-	if (hp_work->rc)
-		*(hp_work->rc) = handle_dlpar_errorlog(hp_work->errlog);
-	else
-		handle_dlpar_errorlog(hp_work->errlog);
-
-	if (hp_work->hp_completion)
-		complete(hp_work->hp_completion);
+	handle_dlpar_errorlog(hp_work->errlog);
 
 	kfree(hp_work->errlog);
 	kfree((void *)work);
 }
 
-void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
-			 struct completion *hotplug_done, int *rc)
+void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog)
 {
 	struct pseries_hp_work *work;
 	struct pseries_hp_errorlog *hp_errlog_copy;
@@ -397,13 +388,9 @@ void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
 	if (work) {
 		INIT_WORK((struct work_struct *)work, pseries_hp_work_fn);
 		work->errlog = hp_errlog_copy;
-		work->hp_completion = hotplug_done;
-		work->rc = rc;
 		queue_work(pseries_hp_wq, (struct work_struct *)work);
 	} else {
-		*rc = -ENOMEM;
 		kfree(hp_errlog_copy);
-		complete(hotplug_done);
 	}
 }
 
@@ -521,18 +508,15 @@ static int dlpar_parse_id_type(char **cmd, struct pseries_hp_errorlog *hp_elog)
 static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
 			   const char *buf, size_t count)
 {
-	struct pseries_hp_errorlog *hp_elog;
-	struct completion hotplug_done;
+	struct pseries_hp_errorlog hp_elog;
 	char *argbuf;
 	char *args;
 	int rc;
 
 	args = argbuf = kstrdup(buf, GFP_KERNEL);
-	hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL);
-	if (!hp_elog || !argbuf) {
+	if (!argbuf) {
 		pr_info("Could not allocate resources for DLPAR operation\n");
 		kfree(argbuf);
-		kfree(hp_elog);
 		return -ENOMEM;
 	}
 
@@ -540,25 +524,22 @@ static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
 	 * Parse out the request from the user, this will be in the form:
 	 * <resource> <action> <id_type> <id>
 	 */
-	rc = dlpar_parse_resource(&args, hp_elog);
+	rc = dlpar_parse_resource(&args, &hp_elog);
 	if (rc)
 		goto dlpar_store_out;
 
-	rc = dlpar_parse_action(&args, hp_elog);
+	rc = dlpar_parse_action(&args, &hp_elog);
 	if (rc)
 		goto dlpar_store_out;
 
-	rc = dlpar_parse_id_type(&args, hp_elog);
+	rc = dlpar_parse_id_type(&args, &hp_elog);
 	if (rc)
 		goto dlpar_store_out;
 
-	init_completion(&hotplug_done);
-	queue_hotplug_event(hp_elog, &hotplug_done, &rc);
-	wait_for_completion(&hotplug_done);
+	rc = handle_dlpar_errorlog(&hp_elog);
 
 dlpar_store_out:
 	kfree(argbuf);
-	kfree(hp_elog);
 
 	if (rc)
 		pr_err("Could not handle DLPAR request \"%s\"\n", buf);
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index f0e30dc94988..6f27d00505cf 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -242,7 +242,7 @@ static int add_dt_node(__be32 parent_phandle, __be32 drc_index)
 
 static void prrn_update_node(__be32 phandle)
 {
-	struct pseries_hp_errorlog *hp_elog;
+	struct pseries_hp_errorlog hp_elog;
 	struct device_node *dn;
 
 	/*
@@ -255,18 +255,12 @@ static void prrn_update_node(__be32 phandle)
 		return;
 	}
 
-	hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL);
-	if(!hp_elog)
-		return;
-
-	hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_MEM;
-	hp_elog->action = PSERIES_HP_ELOG_ACTION_READD;
-	hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
-	hp_elog->_drc_u.drc_index = phandle;
-
-	queue_hotplug_event(hp_elog, NULL, NULL);
+	hp_elog.resource = PSERIES_HP_ELOG_RESOURCE_MEM;
+	hp_elog.action = PSERIES_HP_ELOG_ACTION_READD;
+	hp_elog.id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
+	hp_elog._drc_u.drc_index = phandle;
 
-	kfree(hp_elog);
+	handle_dlpar_errorlog(&hp_elog);
 }
 
 int pseries_devicetree_update(s32 scope)
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 60db2ee511fb..9310a20aef44 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -59,8 +59,9 @@ extern int dlpar_detach_node(struct device_node *);
 extern int dlpar_acquire_drc(u32 drc_index);
 extern int dlpar_release_drc(u32 drc_index);
 
-void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
-			 struct completion *hotplug_done, int *rc);
+void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog);
+int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_errlog);
+
 #ifdef CONFIG_MEMORY_HOTPLUG
 int dlpar_memory(struct pseries_hp_errorlog *hp_elog);
 #else
diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index 14a46b07ab2f..25b48994cd60 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -238,7 +238,7 @@ static irqreturn_t ras_hotplug_interrupt(int irq, void *dev_id)
 	 */
 	if (hp_elog->resource == PSERIES_HP_ELOG_RESOURCE_MEM ||
 	    hp_elog->resource == PSERIES_HP_ELOG_RESOURCE_CPU)
-		queue_hotplug_event(hp_elog, NULL, NULL);
+		queue_hotplug_event(hp_elog);
 	else
 		log_error(ras_log_buf, ERR_TYPE_RTAS_LOG, 0);
 

Re: [PATCH v3 2/2] powerpc/pseries: Wait for completion of hotplug events during PRRN handling

[Michael Ellerman]

Hi Nathan,

Nathan Fontenot <nfont@linux.vnet.ibm.com> writes:
> On 08/08/2018 10:29 AM, John Allen wrote:
>> While handling PRRN events, the time to handle the actual hotplug events
>> dwarfs the time it takes to perform the device tree updates and queue the
>> hotplug events. In the case that PRRN events are being queued continuously,
>> hotplug events have been observed to be queued faster than the kernel can
>> actually handle them. This patch avoids the problem by waiting for a
>> hotplug request to complete before queueing more hotplug events.
>> 
>> Signed-off-by: John Allen <jallen@linux.ibm.com>
>
> In the V2 thread it was mentioned that we could just call the DLPAR operation
> directly instead of going through the workqueue. I have written a patch to do
> this that also cleans up some of the request handling.
>
> requests that come through the hotplug interrupt still use the workqueue. The
> other requests, PRRN and sysfs, just call the dlpar handler directly. This
> eliminates the need for a wait conditional and return code handling in the
> workqueue handler and should solve the issue that John solves with his patch.
>
> This still needs testing but wanted to get people's thoughts.

It looks great to me.

I guess I thought we'd need to add a lock, but I think you're right that
the existing device hotplug lock will work.

So if this survives a bit of testing then I'd love to merge it.

cheers

Re: [PATCH v3 1/2] powerpc/pseries: Avoid blocking rtas polling handling multiple PRRN events

[Michael Ellerman]

John Allen <jallen@linux.ibm.com> writes:
> When a PRRN event is being handled and another PRRN event comes in, the
> second event will block rtas polling waiting on the first to complete,
> preventing any further rtas events from being handled. This can be
> especially problematic in case that PRRN events are continuously being
> queued in which case rtas polling gets indefinitely blocked completely.
>
> This patch removes the blocking call to flush_work and allows the
> default workqueue behavior to handle duplicate events.
>
> Signed-off-by: John Allen <jallen@linux.ibm.com>
> ---
> v3:
>   -Scrap the mutex as it only replicates existing workqueue behavior.
> v2:
>   -Unlock prrn_lock when PRRN operations are complete, not after handler is
>    scheduled.
>   -Remove call to flush_work, the previous broken method of serializing
>    PRRN events.
> ---
>  arch/powerpc/kernel/rtasd.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
> index 44d66c33d59d..2017934e5985 100644
> --- a/arch/powerpc/kernel/rtasd.c
> +++ b/arch/powerpc/kernel/rtasd.c
> @@ -290,7 +290,6 @@ static DECLARE_WORK(prrn_work, prrn_work_fn);
>  
>  static void prrn_schedule_update(u32 scope)
>  {
> -	flush_work(&prrn_work);
>  	prrn_update_scope = scope;
>  	schedule_work(&prrn_work);
>  }

I think we can still lose a "scope" value here.

ie.

prrn_schedule_update(1)
  prrn_update_scope = 1;
  schedule_work(&prrn_work);    // scheduled but doesn't run yet

...

prrn_schedule_update(2)
  prrn_update_scope = 2;
  schedule_work(&prrn_work);   // it's still scheduled

// finally

prrn_work_fn(..)
  pseries_devicetree_update(-prrn_update_scope); // aka 2


So we lost the "1" value.

We could fix that by allocating a work struct that holds the scope value.

But I'd really like to try just calling the prrn_work_fn() directly,
we're already running as a scheduled work function, I don't see any
benefit in scheduling more work.

cheers