[PATCH v2 0/5] padata flushing and CPU hotplug fixes

[PATCH v2 0/5] padata flushing and CPU hotplug fixes

[Daniel Jordan]

Hi,
Here are some miscellaneous padata fixes, mostly to do with CPU hotplug.
This time around there's a new hotplug state to make the CPU remove path
cleaner, and the CC list grew a bit.

Daniel

v2:
 - patches 1-3 are new; 4-5 have changed since v1[*]
 - attempted to fix padata flushing as requested (Herbert)
 - changed hotplug state in which __padata_remove_cpu() is
   called (Herbert)
 - purged cpumask_clear_cpu() calls from same function (Herbert)
 - dropped v1's patch 3/2 (Herbert)
 - after more thought, left the Fixes: tag on the last patch the same

testing:
 - testcase was tcrypt mode=215 sec=1 throughout
 - ran all cpumask combos among parallel cpumasks, serial cpumasks, and CPU
   hotplug in a 3-CPU VM
 - lockdep to check patch 4
 - tested at each patch in this set with and without
   CONFIG_CRYPTO_PCRYPT

Series based on recent mainline plus all padata patches in cryptodev:
    git://oss.oracle.com/git/linux-dmjordan.git padata-cpuhp-v2

[*] https://lore.kernel.org/linux-crypto/20190809192857.26585-1-daniel.m.jordan@oracle.com/

Daniel Jordan (5):
  padata: make flushing work with async users
  padata: remove reorder_objects
  padata: get rid of padata_remove_cpu() for real
  padata: always acquire cpu_hotplug_lock before pinst->lock
  padata: validate cpumask without removed CPU during offline

 Documentation/padata.txt   | 18 ++------
 include/linux/cpuhotplug.h |  1 +
 include/linux/padata.h     |  5 +-
 kernel/padata.c            | 95 +++++++++++---------------------------
 4 files changed, 36 insertions(+), 83 deletions(-)


base-commit: d1abaeb3be7b5fa6d7a1fbbd2e14e3310005c4c1
prerequisite-patch-id: a5bfed8ea60d5a784b8b3e21ccb5657ced2aa1e3
prerequisite-patch-id: 96d53aecccb5af242ba5ee342d75810ecd9bfb84
prerequisite-patch-id: 965d8a63c1461f00219aec2d817f2ca85d49cfb3
prerequisite-patch-id: 8e6c2988331b46c9467ac568157c6c575cbe6578
-- 
2.23.0

[PATCH v2 1/5] padata: make flushing work with async users

[Daniel Jordan]

padata_flush_queues() is broken for an async ->parallel() function
because flush_work() can't wait on it:

  # modprobe tcrypt alg="pcrypt(cryptd(rfc4106(gcm_base(ctr(aes-generic),ghash-generic))))" type=3
  # modprobe tcrypt mode=215 sec=1 &
  # sleep 5; echo 7 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask

  kernel BUG at kernel/padata.c:473!
  invalid opcode: 0000 [#1] SMP PTI
  CPU: 0 PID: 282 Comm: sh Not tainted 5.3.0-rc5-padata-base+ #3
  RIP: 0010:padata_flush_queues+0xa7/0xb0
  Call Trace:
   padata_replace+0xa1/0x110
   padata_set_cpumask+0xae/0x130
   store_cpumask+0x75/0xa0
   padata_sysfs_store+0x20/0x30
   ...

Wait instead for the parallel_data (pd) object's refcount to drop to
zero.

Simplify by taking an initial reference on a pd when allocating an
instance.  That ref is dropped during flushing, which allows calling
wait_for_completion() unconditionally.

If the initial ref weren't used, the only other alternative I could
think of is that complete() would be conditional on !PADATA_INIT or
PADATA_REPLACE (in addition to zero pd->refcnt), and
wait_for_completion() on nonzero pd->refcnt.  But then complete() could
be called without a matching wait:

completer                     waiter
---------                     ------
DEC pd->refcnt    // 0
                              pinst->flags |= PADATA_REPLACE
LOAD pinst->flags // REPLACE
                              LOAD pd->refcnt // 0
                              /* return without wait_for_completion() */
complete()

No more flushing per-CPU work items, so no more CPU hotplug lock in
__padata_stop.

Fixes: 2b73b07ab8a4 ("padata: Flush the padata queues actively")
Reported-by: Herbert Xu <herbert@gondor.apana.org.au>
Suggested-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/padata.h |  3 +++
 kernel/padata.c        | 32 ++++++++++++--------------------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/include/linux/padata.h b/include/linux/padata.h
index 8da673861d99..1c73f9cc7b72 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -14,6 +14,7 @@
 #include <linux/list.h>
 #include <linux/notifier.h>
 #include <linux/kobject.h>
+#include <linux/completion.h>
 
 #define PADATA_CPU_SERIAL   0x01
 #define PADATA_CPU_PARALLEL 0x02
@@ -104,6 +105,7 @@ struct padata_cpumask {
  * @squeue: percpu padata queues used for serialuzation.
  * @reorder_objects: Number of objects waiting in the reorder queues.
  * @refcnt: Number of objects holding a reference on this parallel_data.
+ * @flushing_done: Wait for all objects to be sent out.
  * @max_seq_nr:  Maximal used sequence number.
  * @cpu: Next CPU to be processed.
  * @cpumask: The cpumasks in use for parallel and serial workers.
@@ -116,6 +118,7 @@ struct parallel_data {
 	struct padata_serial_queue	__percpu *squeue;
 	atomic_t			reorder_objects;
 	atomic_t			refcnt;
+	struct completion		flushing_done;
 	atomic_t			seq_nr;
 	int				cpu;
 	struct padata_cpumask		cpumask;
diff --git a/kernel/padata.c b/kernel/padata.c
index b60cc3dcee58..958166e23123 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -301,7 +301,8 @@ static void padata_serial_worker(struct work_struct *serial_work)
 		list_del_init(&padata->list);
 
 		padata->serial(padata);
-		atomic_dec(&pd->refcnt);
+		if (atomic_dec_return(&pd->refcnt) == 0)
+			complete(&pd->flushing_done);
 	}
 	local_bh_enable();
 }
@@ -423,7 +424,9 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
 	padata_init_squeues(pd);
 	atomic_set(&pd->seq_nr, -1);
 	atomic_set(&pd->reorder_objects, 0);
-	atomic_set(&pd->refcnt, 0);
+	/* Initial ref dropped in padata_flush_queues. */
+	atomic_set(&pd->refcnt, 1);
+	init_completion(&pd->flushing_done);
 	pd->pinst = pinst;
 	spin_lock_init(&pd->lock);
 	pd->cpu = cpumask_first(pd->cpumask.pcpu);
@@ -453,24 +456,15 @@ static void padata_free_pd(struct parallel_data *pd)
 /* Flush all objects out of the padata queues. */
 static void padata_flush_queues(struct parallel_data *pd)
 {
-	int cpu;
-	struct padata_parallel_queue *pqueue;
-	struct padata_serial_queue *squeue;
-
-	for_each_cpu(cpu, pd->cpumask.pcpu) {
-		pqueue = per_cpu_ptr(pd->pqueue, cpu);
-		flush_work(&pqueue->work);
-	}
-
-	if (atomic_read(&pd->reorder_objects))
-		padata_reorder(pd);
+	if (!(pd->pinst->flags & PADATA_INIT))
+		return;
 
-	for_each_cpu(cpu, pd->cpumask.cbcpu) {
-		squeue = per_cpu_ptr(pd->squeue, cpu);
-		flush_work(&squeue->work);
-	}
+	if (atomic_dec_return(&pd->refcnt) == 0)
+		complete(&pd->flushing_done);
 
-	BUG_ON(atomic_read(&pd->refcnt) != 0);
+	wait_for_completion(&pd->flushing_done);
+	reinit_completion(&pd->flushing_done);
+	atomic_set(&pd->refcnt, 1);
 }
 
 static void __padata_start(struct padata_instance *pinst)
@@ -487,9 +481,7 @@ static void __padata_stop(struct padata_instance *pinst)
 
 	synchronize_rcu();
 
-	get_online_cpus();
 	padata_flush_queues(pinst->pd);
-	put_online_cpus();
 }
 
 /* Replace the internal control structure with a new one. */
-- 
2.23.0

[PATCH v2 2/5] padata: remove reorder_objects

[Daniel Jordan]

reorder_objects is unused since the rework of padata's flushing, so
remove it.

Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/padata.h | 2 --
 kernel/padata.c        | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/include/linux/padata.h b/include/linux/padata.h
index 1c73f9cc7b72..720e970cda0a 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -103,7 +103,6 @@ struct padata_cpumask {
  * @pinst: padata instance.
  * @pqueue: percpu padata queues used for parallelization.
  * @squeue: percpu padata queues used for serialuzation.
- * @reorder_objects: Number of objects waiting in the reorder queues.
  * @refcnt: Number of objects holding a reference on this parallel_data.
  * @flushing_done: Wait for all objects to be sent out.
  * @max_seq_nr:  Maximal used sequence number.
@@ -116,7 +115,6 @@ struct parallel_data {
 	struct padata_instance		*pinst;
 	struct padata_parallel_queue	__percpu *pqueue;
 	struct padata_serial_queue	__percpu *squeue;
-	atomic_t			reorder_objects;
 	atomic_t			refcnt;
 	struct completion		flushing_done;
 	atomic_t			seq_nr;
diff --git a/kernel/padata.c b/kernel/padata.c
index 958166e23123..2bfce01c5b85 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -179,7 +179,6 @@ static struct padata_priv *padata_get_next(struct parallel_data *pd)
 				    struct padata_priv, list);
 
 		list_del_init(&padata->list);
-		atomic_dec(&pd->reorder_objects);
 
 		pd->cpu = cpumask_next_wrap(cpu, pd->cpumask.pcpu, -1,
 					    false);
@@ -323,7 +322,6 @@ void padata_do_serial(struct padata_priv *padata)
 
 	spin_lock(&pqueue->reorder.lock);
 	list_add_tail(&padata->list, &pqueue->reorder.list);
-	atomic_inc(&pd->reorder_objects);
 	spin_unlock(&pqueue->reorder.lock);
 
 	/*
@@ -423,7 +421,6 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
 	padata_init_pqueues(pd);
 	padata_init_squeues(pd);
 	atomic_set(&pd->seq_nr, -1);
-	atomic_set(&pd->reorder_objects, 0);
 	/* Initial ref dropped in padata_flush_queues. */
 	atomic_set(&pd->refcnt, 1);
 	init_completion(&pd->flushing_done);
-- 
2.23.0

[PATCH v2 3/5] padata: get rid of padata_remove_cpu() for real

[Daniel Jordan]

A later patch is going to address a lock ordering issue involving
pinst->mutex and the CPU hotplug lock.  padata_remove_cpu() needs fixing
but it has no callers, so just delete it rather than maintaining unused
code.  The Fixes commit forgot to do it anyway.

While at it remove Documentation references to other unused functions.

Fixes: 815613da6a67 ("kernel/padata.c: removed unused code")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/padata.txt | 18 ++++--------------
 kernel/padata.c          | 35 -----------------------------------
 2 files changed, 4 insertions(+), 49 deletions(-)

diff --git a/Documentation/padata.txt b/Documentation/padata.txt
index b103d0c82000..43ca928da713 100644
--- a/Documentation/padata.txt
+++ b/Documentation/padata.txt
@@ -51,27 +51,17 @@ padata cpumask contains no active CPU (flag not set).
 padata_stop clears the flag and blocks until the padata instance
 is unused.
 
-The list of CPUs to be used can be adjusted with these functions::
+The list of CPUs to be used can be adjusted with this function::
 
-    int padata_set_cpumasks(struct padata_instance *pinst,
-			    cpumask_var_t pcpumask,
-			    cpumask_var_t cbcpumask);
     int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 			   cpumask_var_t cpumask);
-    int padata_add_cpu(struct padata_instance *pinst, int cpu, int mask);
-    int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask);
 
 Changing the CPU masks are expensive operations, though, so it should not be
 done with great frequency.
 
-It's possible to change both cpumasks of a padata instance with
-padata_set_cpumasks by specifying the cpumasks for parallel execution (pcpumask)
-and for the serial callback function (cbcpumask). padata_set_cpumask is used to
-change just one of the cpumasks. Here cpumask_type is one of PADATA_CPU_SERIAL,
-PADATA_CPU_PARALLEL and cpumask specifies the new cpumask to use.
-To simply add or remove one CPU from a certain cpumask the functions
-padata_add_cpu/padata_remove_cpu are used. cpu specifies the CPU to add or
-remove and mask is one of PADATA_CPU_SERIAL, PADATA_CPU_PARALLEL.
+padata_set_cpumask is used to change just one of the cpumasks. Here cpumask_type
+is one of PADATA_CPU_SERIAL or PADATA_CPU_PARALLEL, and cpumask specifies the
+new cpumask to use.
 
 If a user is interested in padata cpumask changes, he can register to
 the padata cpumask change notifier::
diff --git a/kernel/padata.c b/kernel/padata.c
index 2bfce01c5b85..6adce3b203fe 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -710,41 +710,6 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
 	return 0;
 }
 
- /**
- * padata_remove_cpu - remove a cpu from the one or both(serial and parallel)
- *                     padata cpumasks.
- *
- * @pinst: padata instance
- * @cpu: cpu to remove
- * @mask: bitmask specifying from which cpumask @cpu should be removed
- *        The @mask may be any combination of the following flags:
- *          PADATA_CPU_SERIAL   - serial cpumask
- *          PADATA_CPU_PARALLEL - parallel cpumask
- */
-int padata_remove_cpu(struct padata_instance *pinst, int cpu, int mask)
-{
-	int err;
-
-	if (!(mask & (PADATA_CPU_SERIAL | PADATA_CPU_PARALLEL)))
-		return -EINVAL;
-
-	mutex_lock(&pinst->lock);
-
-	get_online_cpus();
-	if (mask & PADATA_CPU_SERIAL)
-		cpumask_clear_cpu(cpu, pinst->cpumask.cbcpu);
-	if (mask & PADATA_CPU_PARALLEL)
-		cpumask_clear_cpu(cpu, pinst->cpumask.pcpu);
-
-	err = __padata_remove_cpu(pinst, cpu);
-	put_online_cpus();
-
-	mutex_unlock(&pinst->lock);
-
-	return err;
-}
-EXPORT_SYMBOL(padata_remove_cpu);
-
 static inline int pinst_has_cpu(struct padata_instance *pinst, int cpu)
 {
 	return cpumask_test_cpu(cpu, pinst->cpumask.pcpu) ||
-- 
2.23.0

[PATCH v2 4/5] padata: always acquire cpu_hotplug_lock before pinst->lock

[Daniel Jordan]

lockdep complains when...

  # echo 0 > /sys/devices/system/cpu/cpu1/online
  # echo ff > /sys/kernel/pcrypt/pencrypt/parallel_cpumask

  ======================================================
  WARNING: possible circular locking dependency detected
  5.3.0-rc5-padata-base+ #6 Not tainted
  ------------------------------------------------------
  bash/258 is trying to acquire lock:
  00000000c43f7f29 (cpu_hotplug_lock.rw_sem){++++}, at: padata_set_cpumask+0x30/0x130

  but task is already holding lock:
  00000000676aa31d (&pinst->lock){+.+.}, at: padata_set_cpumask+0x2b/0x130

  which lock already depends on the new lock.

padata doesn't take cpu_hotplug_lock and pinst->lock in a consistent
order.  Which should be first?  CPU hotplug calls into padata with
cpu_hotplug_lock already held, so it should have priority.

Fixes: 6751fb3c0e0c ("padata: Use get_online_cpus/put_online_cpus")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 kernel/padata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index 6adce3b203fe..75e668fedd8d 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -603,8 +603,8 @@ int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 	struct cpumask *serial_mask, *parallel_mask;
 	int err = -EINVAL;
 
-	mutex_lock(&pinst->lock);
 	get_online_cpus();
+	mutex_lock(&pinst->lock);
 
 	switch (cpumask_type) {
 	case PADATA_CPU_PARALLEL:
@@ -622,8 +622,8 @@ int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 	err =  __padata_set_cpumasks(pinst, parallel_mask, serial_mask);
 
 out:
-	put_online_cpus();
 	mutex_unlock(&pinst->lock);
+	put_online_cpus();
 
 	return err;
 }
-- 
2.23.0

[PATCH v2 5/5] padata: validate cpumask without removed CPU during offline

[Daniel Jordan]

Configuring an instance's parallel mask without any online CPUs...

  echo 2 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
  echo 0 > /sys/devices/system/cpu/cpu1/online

...makes tcrypt mode=215 crash like this:

  divide error: 0000 [#1] SMP PTI
  CPU: 4 PID: 287 Comm: modprobe Not tainted 5.3.0-rc5-padata-base+ #7
  RIP: 0010:padata_do_parallel+0xfd/0x290
  Call Trace:
   pcrypt_do_parallel+0xed/0x1e0 [pcrypt]
   pcrypt_aead_encrypt+0xbf/0xd0 [pcrypt]
   crypto_aead_encrypt+0x1f/0x30
   do_mult_aead_op+0x4e/0xdf [tcrypt]
   test_mb_aead_speed.constprop.0.cold+0x226/0x564 [tcrypt]
   do_test+0x2280/0x4c16 [tcrypt]
   tcrypt_mod_init+0x55/0x1000 [tcrypt]
   ...

cpumask_weight() in padata_cpu_hash() returns 0 because the mask has no
CPUs.  The problem is __padata_remove_cpu() checks for valid masks too
early and so doesn't mark the instance PADATA_INVALID as expected, which
would have made padata_do_parallel() return error before doing the
division.

Fix by introducing a second padata CPU hotplug state before
CPUHP_BRINGUP_CPU so that __padata_remove_cpu() sees the online mask
without @cpu.  No need for the cpumask_clear_cpu()s since @cpu is
already missing from those masks.

Fixes: 33e54450683c ("padata: Handle empty padata cpumasks")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/cpuhotplug.h |  1 +
 kernel/padata.c            | 21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 068793a619ca..2d55cee638fc 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -59,6 +59,7 @@ enum cpuhp_state {
 	CPUHP_IOMMU_INTEL_DEAD,
 	CPUHP_LUSTRE_CFS_DEAD,
 	CPUHP_AP_ARM_CACHE_B15_RAC_DEAD,
+	CPUHP_PADATA_DEAD,
 	CPUHP_WORKQUEUE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
diff --git a/kernel/padata.c b/kernel/padata.c
index 75e668fedd8d..5325e5978ee4 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -690,7 +690,7 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
 {
 	struct parallel_data *pd = NULL;
 
-	if (cpumask_test_cpu(cpu, cpu_online_mask)) {
+	if (!cpumask_test_cpu(cpu, cpu_online_mask)) {
 
 		if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
 		    !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
@@ -702,9 +702,6 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
 			return -ENOMEM;
 
 		padata_replace(pinst, pd);
-
-		cpumask_clear_cpu(cpu, pd->cpumask.cbcpu);
-		cpumask_clear_cpu(cpu, pd->cpumask.pcpu);
 	}
 
 	return 0;
@@ -731,7 +728,7 @@ static int padata_cpu_online(unsigned int cpu, struct hlist_node *node)
 	return ret;
 }
 
-static int padata_cpu_prep_down(unsigned int cpu, struct hlist_node *node)
+static int padata_cpu_dead(unsigned int cpu, struct hlist_node *node)
 {
 	struct padata_instance *pinst;
 	int ret;
@@ -752,6 +749,7 @@ static enum cpuhp_state hp_online;
 static void __padata_free(struct padata_instance *pinst)
 {
 #ifdef CONFIG_HOTPLUG_CPU
+	cpuhp_state_remove_instance_nocalls(CPUHP_PADATA_DEAD, &pinst->node);
 	cpuhp_state_remove_instance_nocalls(hp_online, &pinst->node);
 #endif
 
@@ -938,6 +936,8 @@ static struct padata_instance *padata_alloc(struct workqueue_struct *wq,
 
 #ifdef CONFIG_HOTPLUG_CPU
 	cpuhp_state_add_instance_nocalls_cpuslocked(hp_online, &pinst->node);
+	cpuhp_state_add_instance_nocalls_cpuslocked(CPUHP_PADATA_DEAD,
+						    &pinst->node);
 #endif
 	return pinst;
 
@@ -984,17 +984,24 @@ static __init int padata_driver_init(void)
 	int ret;
 
 	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, "padata:online",
-				      padata_cpu_online,
-				      padata_cpu_prep_down);
+				      padata_cpu_online, NULL);
 	if (ret < 0)
 		return ret;
 	hp_online = ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_PADATA_DEAD, "padata:dead",
+				      NULL, padata_cpu_dead);
+	if (ret < 0) {
+		cpuhp_remove_multi_state(hp_online);
+		return ret;
+	}
 	return 0;
 }
 module_init(padata_driver_init);
 
 static __exit void padata_driver_exit(void)
 {
+	cpuhp_remove_multi_state(CPUHP_PADATA_DEAD);
 	cpuhp_remove_multi_state(hp_online);
 }
 module_exit(padata_driver_exit);
-- 
2.23.0

Re: [PATCH v2 1/5] padata: make flushing work with async users

[Herbert Xu]

On Wed, Aug 28, 2019 at 06:14:21PM -0400, Daniel Jordan wrote:
>
> @@ -453,24 +456,15 @@ static void padata_free_pd(struct parallel_data *pd)
>  /* Flush all objects out of the padata queues. */
>  static void padata_flush_queues(struct parallel_data *pd)
>  {
> -	int cpu;
> -	struct padata_parallel_queue *pqueue;
> -	struct padata_serial_queue *squeue;
> -
> -	for_each_cpu(cpu, pd->cpumask.pcpu) {
> -		pqueue = per_cpu_ptr(pd->pqueue, cpu);
> -		flush_work(&pqueue->work);
> -	}
> -
> -	if (atomic_read(&pd->reorder_objects))
> -		padata_reorder(pd);
> +	if (!(pd->pinst->flags & PADATA_INIT))
> +		return;
>  
> -	for_each_cpu(cpu, pd->cpumask.cbcpu) {
> -		squeue = per_cpu_ptr(pd->squeue, cpu);
> -		flush_work(&squeue->work);
> -	}
> +	if (atomic_dec_return(&pd->refcnt) == 0)
> +		complete(&pd->flushing_done);
>  
> -	BUG_ON(atomic_read(&pd->refcnt) != 0);
> +	wait_for_completion(&pd->flushing_done);
> +	reinit_completion(&pd->flushing_done);
> +	atomic_set(&pd->refcnt, 1);
>  }

I don't think waiting is an option.  In a pathological case the
hardware may not return at all.  We cannot and should not hold off
CPU hotplug for an arbitrary amount of time when the event we are
waiting for isn't even occuring on that CPU.

I don't think flushing is needed at all.  All we need to do is
maintain consistency before and after the CPU hotplug event.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/5] padata: make flushing work with async users

[Daniel Jordan]

On Thu, Sep 05, 2019 at 02:17:35PM +1000, Herbert Xu wrote:
> On Wed, Aug 28, 2019 at 06:14:21PM -0400, Daniel Jordan wrote:
> >
> > @@ -453,24 +456,15 @@ static void padata_free_pd(struct parallel_data *pd)
> >  /* Flush all objects out of the padata queues. */
> >  static void padata_flush_queues(struct parallel_data *pd)
> >  {
> > -	int cpu;
> > -	struct padata_parallel_queue *pqueue;
> > -	struct padata_serial_queue *squeue;
> > -
> > -	for_each_cpu(cpu, pd->cpumask.pcpu) {
> > -		pqueue = per_cpu_ptr(pd->pqueue, cpu);
> > -		flush_work(&pqueue->work);
> > -	}
> > -
> > -	if (atomic_read(&pd->reorder_objects))
> > -		padata_reorder(pd);
> > +	if (!(pd->pinst->flags & PADATA_INIT))
> > +		return;
> >  
> > -	for_each_cpu(cpu, pd->cpumask.cbcpu) {
> > -		squeue = per_cpu_ptr(pd->squeue, cpu);
> > -		flush_work(&squeue->work);
> > -	}
> > +	if (atomic_dec_return(&pd->refcnt) == 0)
> > +		complete(&pd->flushing_done);
> >  
> > -	BUG_ON(atomic_read(&pd->refcnt) != 0);
> > +	wait_for_completion(&pd->flushing_done);
> > +	reinit_completion(&pd->flushing_done);
> > +	atomic_set(&pd->refcnt, 1);
> >  }
> 
> I don't think waiting is an option.  In a pathological case the
> hardware may not return at all.  We cannot and should not hold off
> CPU hotplug for an arbitrary amount of time when the event we are
> waiting for isn't even occuring on that CPU.

Ok, I hadn't considered hardware not returning.

> I don't think flushing is needed at all.  All we need to do is
> maintain consistency before and after the CPU hotplug event.

I could imagine not flushing would work for replacing a pd.  The old pd could
be freed by whatever drops the last reference and the new pd could be
installed, all without flushing.

In the case of freeing an instance, though, padata needs to wait for all the
jobs to complete so they don't use the instance's data after it's been freed.
Holding the CPU hotplug lock isn't necessary for this, though, so I think we're
ok to wait here.

Re: [PATCH v2 1/5] padata: make flushing work with async users

[Daniel Jordan]

On Thu, Sep 05, 2019 at 06:37:56PM -0400, Daniel Jordan wrote:
> On Thu, Sep 05, 2019 at 02:17:35PM +1000, Herbert Xu wrote:
> > I don't think waiting is an option.  In a pathological case the
> > hardware may not return at all.  We cannot and should not hold off
> > CPU hotplug for an arbitrary amount of time when the event we are
> > waiting for isn't even occuring on that CPU.
> 
> Ok, I hadn't considered hardware not returning.
> 
> > I don't think flushing is needed at all.  All we need to do is
> > maintain consistency before and after the CPU hotplug event.
> 
> I could imagine not flushing would work for replacing a pd.  The old pd could
> be freed by whatever drops the last reference and the new pd could be
> installed, all without flushing.
> 
> In the case of freeing an instance, though, padata needs to wait for all the
> jobs to complete so they don't use the instance's data after it's been freed.
> Holding the CPU hotplug lock isn't necessary for this, though, so I think we're
> ok to wait here.

[FYI, I'm currently on leave until mid-October and will return to this series
then.]