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/[email protected]/
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
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 <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Steffen Klassert <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Steffen Klassert <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
reorder_objects is unused since the rework of padata's flushing, so
remove it.
Signed-off-by: Daniel Jordan <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Steffen Klassert <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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