[PATCH] random: set fast pool count to zero in cpuhp teardown

[PATCH] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

Rather than having to use expensive atomics, which were visibly the most
expensive thing in the entire irq handler, simply take care of the
extreme edge case of resetting count to 0 in the cpuhp teardown handler.
This simplifies the code a bit and lets us use vanilla variables rather
than atomics, and performance should be improved.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Sebastian - this was *way* simpler than I had anticipated, and I wish it
were done this way originally. If this looks good to you and you ack it,
I'll wind up merging this commit into the previous one in my branch,
since the intermediate atomic_t stage isn't really that interesting
any more, now that I've seen the light. Please take a look and let me
know what you think. -Jason

 drivers/char/random.c      | 33 ++++++++++++++++++---------------
 include/linux/cpuhotplug.h |  1 +
 include/linux/random.h     |  2 ++
 kernel/cpu.c               |  6 ++++++
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 805a924b1f5f..e177d806db1d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1180,7 +1180,7 @@ struct fast_pool {
 	unsigned long pool[16 / sizeof(long)];
 	struct work_struct mix;
 	unsigned long last;
-	atomic_t count;
+	unsigned int count;
 	u16 reg_idx;
 };
 
@@ -1216,6 +1216,19 @@ static void fast_mix(u32 pool[4])
 
 static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
 
+int random_offline_cpu(unsigned int cpu)
+{
+	/*
+	 * Set the count to zero when offlining the CPU for two
+	 * reasons: 1) so that all new accumulated irqs are fresh
+	 * when it comes back online, and 2) so that its worker is
+	 * permitted to schedule again when it comes back online,
+	 * since the MIX_INFLIGHT flag will be cleared.
+	 */
+	per_cpu_ptr(&irq_randomness, cpu)->count = 0;
+	return 0;
+}
+
 static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
 {
 	u32 *ptr = (u32 *)regs;
@@ -1240,15 +1253,6 @@ static void mix_interrupt_randomness(struct work_struct *work)
 	local_irq_disable();
 	if (fast_pool != this_cpu_ptr(&irq_randomness)) {
 		local_irq_enable();
-		/*
-		 * If we are unlucky enough to have been moved to another CPU,
-		 * during CPU hotplug while the CPU was shutdown then we set
-		 * our count to zero atomically so that when the CPU comes
-		 * back online, it can enqueue work again. The _release here
-		 * pairs with the atomic_inc_return_acquire in
-		 * add_interrupt_randomness().
-		 */
-		atomic_set_release(&fast_pool->count, 0);
 		return;
 	}
 
@@ -1257,7 +1261,7 @@ static void mix_interrupt_randomness(struct work_struct *work)
 	 * consistent view, before we reenable irqs again.
 	 */
 	memcpy(pool, fast_pool->pool, sizeof(pool));
-	atomic_set(&fast_pool->count, 0);
+	fast_pool->count = 0;
 	fast_pool->last = jiffies;
 	local_irq_enable();
 
@@ -1289,14 +1293,13 @@ void add_interrupt_randomness(int irq)
 	}
 
 	fast_mix((u32 *)fast_pool->pool);
-	/* The _acquire here pairs with the atomic_set_release in mix_interrupt_randomness(). */
-	new_count = (unsigned int)atomic_inc_return_acquire(&fast_pool->count);
+	new_count = ++fast_pool->count;
 
 	if (unlikely(crng_init == 0)) {
 		if (new_count >= 64 &&
 		    crng_pre_init_inject(fast_pool->pool, sizeof(fast_pool->pool),
 					 true, true) > 0) {
-			atomic_set(&fast_pool->count, 0);
+			fast_pool->count = 0;
 			fast_pool->last = now;
 			if (spin_trylock(&input_pool.lock)) {
 				_mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
@@ -1314,7 +1317,7 @@ void add_interrupt_randomness(int irq)
 
 	if (unlikely(!fast_pool->mix.func))
 		INIT_WORK(&fast_pool->mix, mix_interrupt_randomness);
-	atomic_or(MIX_INFLIGHT, &fast_pool->count);
+	fast_pool->count |= MIX_INFLIGHT;
 	queue_work_on(raw_smp_processor_id(), system_highpri_wq, &fast_pool->mix);
 }
 EXPORT_SYMBOL_GPL(add_interrupt_randomness);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 411a428ace4d..199dedacd32e 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -239,6 +239,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_POWERPC_HV_GPCI_ONLINE,
 	CPUHP_AP_PERF_CSKY_ONLINE,
 	CPUHP_AP_WATCHDOG_ONLINE,
+	CPUHP_AP_RANDOM_ONLINE,
 	CPUHP_AP_WORKQUEUE_ONLINE,
 	CPUHP_AP_RCUTREE_ONLINE,
 	CPUHP_AP_BASE_CACHEINFO_ONLINE,
diff --git a/include/linux/random.h b/include/linux/random.h
index d7354de9351e..8dd5721e3512 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -35,6 +35,8 @@ extern void add_interrupt_randomness(int irq) __latent_entropy;
 extern void add_hwgenerator_randomness(const void *buffer, size_t count,
 				       size_t entropy);
 
+extern int random_offline_cpu(unsigned int cpu);
+
 extern void get_random_bytes(void *buf, size_t nbytes);
 extern int wait_for_random_bytes(void);
 extern int __init rand_initialize(void);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 407a2568f35e..e03f7880c163 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -34,6 +34,7 @@
 #include <linux/scs.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/cpuset.h>
+#include <linux/random.h>
 
 #include <trace/events/power.h>
 #define CREATE_TRACE_POINTS
@@ -1777,6 +1778,11 @@ static struct cpuhp_step cpuhp_hp_states[] = {
 		.startup.single		= lockup_detector_online_cpu,
 		.teardown.single	= lockup_detector_offline_cpu,
 	},
+	[CPUHP_AP_RANDOM_ONLINE] = {
+		.name			= "random:online",
+		.startup.single		= NULL,
+		.teardown.single	= random_offline_cpu,
+	},
 	[CPUHP_AP_WORKQUEUE_ONLINE] = {
 		.name			= "workqueue:online",
 		.startup.single		= workqueue_online_cpu,
-- 
2.35.0

Re: [PATCH] random: set fast pool count to zero in cpuhp teardown

[Sebastian Andrzej Siewior]

On 2022-02-13 22:53:43 [+0100], Jason A. Donenfeld wrote:
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 805a924b1f5f..e177d806db1d 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1216,6 +1216,19 @@ static void fast_mix(u32 pool[4])
>  
>  static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
>  
> +int random_offline_cpu(unsigned int cpu)
> +{
> +	/*
> +	 * Set the count to zero when offlining the CPU for two
> +	 * reasons: 1) so that all new accumulated irqs are fresh
> +	 * when it comes back online, and 2) so that its worker is
> +	 * permitted to schedule again when it comes back online,
> +	 * since the MIX_INFLIGHT flag will be cleared.
> +	 */
> +	per_cpu_ptr(&irq_randomness, cpu)->count = 0;

You registered that handler between
   CPUHP_AP_ONLINE_IDLE … CPUHP_AP_ACTIVE

That means:
- interrupts are still active. So you could schedule a worker after the
  handler run (need 64 interrupts).
- That handler will be invoked on the CPU that goes offline.
- it will be invoked before the worker is unbound from the offline-going
  CPU. Once unbound at CPUHP_AP_WORKQUEUE_ONLINE the worker may remain
  on the CPU before the scheduler pushes it away.

You could:
- keeping it at this position but
  - this_cpu_ptr() could be used.
  - move it to the online phase so it is reset for the CPU coming up
    (from this point, you could have >= 64 interrupts before the CPU is
    no longer serving interrupts).

- move it to CPUHP_OFFLINE … CPUHP_BRINGUP_CPU. This is invoked on
  another CPU once the is dead / before it comes up.
  - in that case the function can remain as-is. But we have "rollback".

One slight complication:
After the CPUHP_AP_WORKQUEUE_ONLINE state (going down), the worker is
tainted. It may be moved to another CPU but may remain on the (correct)
CPU and run. Moving HP-handler to an earlier point has the advantage
that you know. 

We also have this "rollback". That means the CPU goes down and then one
step fails and it rolls back online. That means if you have your handler
at an earlier point, you don't notice it and the worker may have skipped
its turn. (Also I didn't check but I *assume* that after
CPUHP_AP_WORKQUEUE_ONLINE there is no pool bound to the CPU and any new
worker will be moved to an unbound pool).

The more I think I about it, moving the state left and right, I think
that having a CPU up handler after CPUHP_AP_WORKQUEUE_ONLINE (like you
have now (but up, not down)) doing:

	fast_pool = this_cpu_ptr(&irq_randomness);
	local_irq_disable();
	if (fast_pool->count & FAST_POOL_MIX_INFLIGHT) {
		fast_pool->count = 0;
		fast_pool->last = jiffies
	}
	local_irq_enable();

should work just fine. This covers:
- should a worker be scheduled while CPU is going down and do its job,
  we good.

- should a worker be scheduled while CPU is going down, moved to another
  CPU and skip its work then we have FAST_POOL_MIX_INFLIGHT set and
  nothing pending. This gets reset once the CPU is going online.
  This also covers the possible rollback scenarios.

- should a CPU already contribute entropy then the HP-handler is not
  going to reset it.

- should a CPU already contribute entropy and schedule a worker then we
  reset the FAST_POOL_MIX_INFLIGHT bit. However ->last is also reset so
  no new worker is scheduled due the time and because count == 0.
  So either the worker runs (and does the same reset) _or_ we wait
  another 64 interrupts + jiffy (in case the worker was scheduled but
  did nothing because it was run on another CPU).

> +	return 0;
> +}
> +

Sebastian

Re: [PATCH] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

Hey Sebastian,

Thanks for your pointers.

I actually think that rollback is less of an issue than:

> - should a CPU already contribute entropy and schedule a worker then we
>   reset the FAST_POOL_MIX_INFLIGHT bit.

If a CPU is going down, and we zero out count, and then it rolls back,
I'm fine with it having to acquire 64 more interrupts. In contrast, if
a CPU comes back online, I'd rather not potentially throw out the
fresh entropy. So I think I will go with your suggestion of:

> - move it to CPUHP_OFFLINE … CPUHP_BRINGUP_CPU. This is invoked on
>   another CPU once the is dead / before it comes up.
>   - in that case the function can remain as-is. But we have "rollback".

This seems like the best compromise. Also, executing in that phase
will let us do other things with that handler later (setting the crng
generation counter to ULONG_MAX), where it also would make sense.

I'll give this a shot and send you a v+1.

Jason

[PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

Rather than having to use expensive atomics, which were visibly the most
expensive thing in the entire irq handler, simply take care of the
extreme edge case of resetting count to 0 in the cpuhp teardown handler,
after no more interrupts will arrive on that CPU. This simplifies the
code a bit and lets us use vanilla variables rather than atomics, and
performance should be improved.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Sebastian -

v2 moves the teardown to CPUHP_OFFLINE…CPUHP_BRINGUP_CPU, per our
discussion.

This was *way* simpler than I had anticipated, and I wish it
were done this way originally. If this looks good to you and you ack it,
I'll wind up merging this commit into the previous one in my branch,
since the intermediate atomic_t stage isn't really that interesting
any more, now that I've seen the light. Please take a look and let me
know what you think.

-Jason

 drivers/char/random.c      | 33 ++++++++++++++++++---------------
 include/linux/cpuhotplug.h |  1 +
 include/linux/random.h     |  2 ++
 kernel/cpu.c               |  6 ++++++
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index bf6e8627b74e..df5aef93da34 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1179,7 +1179,7 @@ struct fast_pool {
 	unsigned long pool[16 / sizeof(long)];
 	struct work_struct mix;
 	unsigned long last;
-	atomic_t count;
+	unsigned int count;
 	u16 reg_idx;
 };
 
@@ -1215,6 +1215,19 @@ static void fast_mix(u32 pool[4])
 
 static DEFINE_PER_CPU(struct fast_pool, irq_randomness);
 
+int random_dead_cpu(unsigned int cpu)
+{
+	/*
+	 * Set the count to zero after offlining the CPU for two
+	 * reasons: 1) so that all new accumulated irqs are fresh
+	 * when it comes back online, and 2) so that its worker is
+	 * permitted to schedule again when it comes back online,
+	 * since the MIX_INFLIGHT flag will be cleared.
+	 */
+	per_cpu_ptr(&irq_randomness, cpu)->count = 0;
+	return 0;
+}
+
 static u32 get_reg(struct fast_pool *f, struct pt_regs *regs)
 {
 	u32 *ptr = (u32 *)regs;
@@ -1239,15 +1252,6 @@ static void mix_interrupt_randomness(struct work_struct *work)
 	local_irq_disable();
 	if (fast_pool != this_cpu_ptr(&irq_randomness)) {
 		local_irq_enable();
-		/*
-		 * If we are unlucky enough to have been moved to another CPU,
-		 * during CPU hotplug while the CPU was shutdown then we set
-		 * our count to zero atomically so that when the CPU comes
-		 * back online, it can enqueue work again. The _release here
-		 * pairs with the atomic_inc_return_acquire in
-		 * add_interrupt_randomness().
-		 */
-		atomic_set_release(&fast_pool->count, 0);
 		return;
 	}
 
@@ -1256,7 +1260,7 @@ static void mix_interrupt_randomness(struct work_struct *work)
 	 * consistent view, before we reenable irqs again.
 	 */
 	memcpy(pool, fast_pool->pool, sizeof(pool));
-	atomic_set(&fast_pool->count, 0);
+	fast_pool->count = 0;
 	fast_pool->last = jiffies;
 	local_irq_enable();
 
@@ -1288,14 +1292,13 @@ void add_interrupt_randomness(int irq)
 	}
 
 	fast_mix((u32 *)fast_pool->pool);
-	/* The _acquire here pairs with the atomic_set_release in mix_interrupt_randomness(). */
-	new_count = (unsigned int)atomic_inc_return_acquire(&fast_pool->count);
+	new_count = ++fast_pool->count;
 
 	if (unlikely(crng_init == 0)) {
 		if (new_count >= 64 &&
 		    crng_pre_init_inject(fast_pool->pool, sizeof(fast_pool->pool),
 					 true, true) > 0) {
-			atomic_set(&fast_pool->count, 0);
+			fast_pool->count = 0;
 			fast_pool->last = now;
 			if (spin_trylock(&input_pool.lock)) {
 				_mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
@@ -1313,7 +1316,7 @@ void add_interrupt_randomness(int irq)
 
 	if (unlikely(!fast_pool->mix.func))
 		INIT_WORK(&fast_pool->mix, mix_interrupt_randomness);
-	atomic_or(MIX_INFLIGHT, &fast_pool->count);
+	fast_pool->count |= MIX_INFLIGHT;
 	queue_work_on(raw_smp_processor_id(), system_highpri_wq, &fast_pool->mix);
 }
 EXPORT_SYMBOL_GPL(add_interrupt_randomness);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 411a428ace4d..38294af566e4 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -127,6 +127,7 @@ enum cpuhp_state {
 	CPUHP_MM_ZSWP_POOL_PREPARE,
 	CPUHP_KVM_PPC_BOOK3S_PREPARE,
 	CPUHP_ZCOMP_PREPARE,
+	CPUHP_RANDOM_PREPARE,
 	CPUHP_TIMERS_PREPARE,
 	CPUHP_MIPS_SOC_PREPARE,
 	CPUHP_BP_PREPARE_DYN,
diff --git a/include/linux/random.h b/include/linux/random.h
index d7354de9351e..fd8c354bae8e 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -35,6 +35,8 @@ extern void add_interrupt_randomness(int irq) __latent_entropy;
 extern void add_hwgenerator_randomness(const void *buffer, size_t count,
 				       size_t entropy);
 
+extern int random_dead_cpu(unsigned int cpu);
+
 extern void get_random_bytes(void *buf, size_t nbytes);
 extern int wait_for_random_bytes(void);
 extern int __init rand_initialize(void);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 407a2568f35e..f83ae4ae7275 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -34,6 +34,7 @@
 #include <linux/scs.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/cpuset.h>
+#include <linux/random.h>
 
 #include <trace/events/power.h>
 #define CREATE_TRACE_POINTS
@@ -1689,6 +1690,11 @@ static struct cpuhp_step cpuhp_hp_states[] = {
 		.startup.single		= rcutree_prepare_cpu,
 		.teardown.single	= rcutree_dead_cpu,
 	},
+	[CPUHP_RANDOM_PREPARE] = {
+		.name			= "random:prepare",
+		.startup.single		= NULL,
+		.teardown.single	= random_dead_cpu,
+	},
 	/*
 	 * On the tear-down path, timers_dead_cpu() must be invoked
 	 * before blk_mq_queue_reinit_notify() from notify_dead(),
-- 
2.35.0

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Sebastian Andrzej Siewior]

On 2022-02-14 14:37:35 [+0100], Jason A. Donenfeld wrote:
> Rather than having to use expensive atomics, which were visibly the most
> expensive thing in the entire irq handler, simply take care of the
> extreme edge case of resetting count to 0 in the cpuhp teardown handler,
> after no more interrupts will arrive on that CPU. This simplifies the
> code a bit and lets us use vanilla variables rather than atomics, and
> performance should be improved.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Sultan Alsawaf <sultan@kerneltoast.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Sebastian -
> 
> v2 moves the teardown to CPUHP_OFFLINE…CPUHP_BRINGUP_CPU, per our
> discussion.

My suggestion was to move it to the startup handler with the code
snippet I had.
As I tried to explain, this may have two problems:
- worker scheduled during CPU-UP before CPUHP_AP_WORKQUEUE_ONLINE are
  probably unbound.

- worker scheduled during CPU-DOWN after CPUHP_AP_WORKQUEUE_ONLINE are
  probably unbound.

The unbound worker may run on any CPU and thus do nothing.
In the CPU-DOWN case before: should we rollback before
CPUHP_RANDOM_PREPARE but after CPUHP_AP_WORKQUEUE_ONLINE then the needed
reset (in case the worker did nothing because it was on the wrong CPU)
will not happen.
Therefore I think, moving it to startup, online, (as suggested in
https://lore.kernel.org/all/Ygo3%2FpuhZFpuX91x@linutronix.de/).

will not have any of this downsides/ corner cases.

Sebastian

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

Hi Sebastian,

If we move this to startup, is there a phase during which no interrupt
will arrive? That is, can this happen very very early in startup, so
that zeroing out count happens *before* ++count?

Jason

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Sebastian Andrzej Siewior]

On 2022-02-14 15:42:50 [+0100], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,

> If we move this to startup, is there a phase during which no interrupt
> will arrive? That is, can this happen very very early in startup, so
> that zeroing out count happens *before* ++count?

Interrupts will arrive starting with CPUHP_AP_ONLINE_IDLE from the CPU
HP point of view. My suggestion had a check for upper most bit and only
clear count if that bit was seen. So we wouldn't clear the counter if we
wouldn't suspect one of the rare corner cases.

> Jason

Sebastian

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

On Mon, Feb 14, 2022 at 3:49 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2022-02-14 15:42:50 [+0100], Jason A. Donenfeld wrote:
> > Hi Sebastian,
> Hi Jason,
>
> > If we move this to startup, is there a phase during which no interrupt
> > will arrive? That is, can this happen very very early in startup, so
> > that zeroing out count happens *before* ++count?
>
> Interrupts will arrive starting with CPUHP_AP_ONLINE_IDLE from the CPU
> HP point of view. My suggestion had a check for upper most bit and only
> clear count if that bit was seen. So we wouldn't clear the counter if we
> wouldn't suspect one of the rare corner cases.

That doesn't work for the other use cases I have for this (see the other patch).

So I think it seems better to keep it before CPUHP_TIMERS_PREPARE, but
do it on startup rather than teardown. Seem reasonable? Would that
mean we zero out before IRQs are enabled?

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Sebastian Andrzej Siewior]

On 2022-02-14 15:52:34 [+0100], Jason A. Donenfeld wrote:
> On Mon, Feb 14, 2022 at 3:49 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > On 2022-02-14 15:42:50 [+0100], Jason A. Donenfeld wrote:
> > > Hi Sebastian,
> > Hi Jason,
> >
> > > If we move this to startup, is there a phase during which no interrupt
> > > will arrive? That is, can this happen very very early in startup, so
> > > that zeroing out count happens *before* ++count?
> >
> > Interrupts will arrive starting with CPUHP_AP_ONLINE_IDLE from the CPU
> > HP point of view. My suggestion had a check for upper most bit and only
> > clear count if that bit was seen. So we wouldn't clear the counter if we
> > wouldn't suspect one of the rare corner cases.
> 
> That doesn't work for the other use cases I have for this (see the other patch).

But you acked my question regarding boot-up? So the teardown callback
won't happen during boot-up.

> So I think it seems better to keep it before CPUHP_TIMERS_PREPARE, but
> do it on startup rather than teardown. Seem reasonable? Would that
> mean we zero out before IRQs are enabled?
I would only zero it if the upper-most bit is there.
If you need (want) to reset the get_random_uXX() pools and such, there
is nothing wrong with having an early notifier at CPU up time before the
CPU gets active (say CPUHP_RANDOM_PREPARE) where you make sure that the
pools will re-fill during first usage.
And then have another one after CPUHP_AP_WORKQUEUE_ONLINE to ensure that
a possible scheduled worker between CPUHP_AP_ONLINE_IDLE and
CPUHP_AP_WORKQUEUE_ONLINE runs on the correct CPU. And this covers also
the rollback problem.

Sebastian

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

Hi Sebastian,

On Mon, Feb 14, 2022 at 4:06 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> But you acked my question regarding boot-up? So the teardown callback
> won't happen during boot-up.

I'd like to do only one method here, so we can set those fields in
startup, provided it happens early enough.
> > So I think it seems better to keep it before CPUHP_TIMERS_PREPARE, but
> > do it on startup rather than teardown. Seem reasonable? Would that
> > mean we zero out before IRQs are enabled?
> I would only zero it if the upper-most bit is there.

I still don't quite understand: why can't we just unconditionally
zero, always, before CPUHP_TIMERS_PREPARE?

> And then have another one after

Two of them seems a little bit out of hand in complexity here... Let's
just find one phase where we can simply set variables without too much
fiddly logic. I'll send a v+1 of what I have in mind for the startup
path.

Jason

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Sebastian Andrzej Siewior]

On 2022-02-14 16:10:36 [+0100], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,

> On Mon, Feb 14, 2022 at 4:06 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > But you acked my question regarding boot-up? So the teardown callback
> > won't happen during boot-up.
> 
> I'd like to do only one method here, so we can set those fields in
> startup, provided it happens early enough.
> > > So I think it seems better to keep it before CPUHP_TIMERS_PREPARE, but
> > > do it on startup rather than teardown. Seem reasonable? Would that
> > > mean we zero out before IRQs are enabled?
> > I would only zero it if the upper-most bit is there.
> 
> I still don't quite understand: why can't we just unconditionally
> zero, always, before CPUHP_TIMERS_PREPARE?

If you have a rollback before CPUHP_TIMERS_PREPARE you don't notice it
and your worker may have skipped this work because it run on the wrong
CPU. Also, I *think* that if you happen to have 64 interrupts between
   CPUHP_AP_ONLINE_IDLE … CPUHP_AP_WORKQUEUE_ONLINE

then the scheduled worker is unbound and may run on the "wrong" CPU.

> > And then have another one after
> 
> Two of them seems a little bit out of hand in complexity here... Let's
> just find one phase where we can simply set variables without too much
> fiddly logic. I'll send a v+1 of what I have in mind for the startup
> path.

Oki.

> Jason

Sebastian

Re: [PATCH v2] random: set fast pool count to zero in cpuhp teardown

[Jason A. Donenfeld]

Hi Sebastian,

On Mon, Feb 14, 2022 at 4:17 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> > I'd like to do only one method here, so we can set those fields in
> > startup, provided it happens early enough.
> > > > So I think it seems better to keep it before CPUHP_TIMERS_PREPARE, but
> > > > do it on startup rather than teardown. Seem reasonable? Would that
> > > > mean we zero out before IRQs are enabled?
> > > I would only zero it if the upper-most bit is there.
> >
> > I still don't quite understand: why can't we just unconditionally
> > zero, always, before CPUHP_TIMERS_PREPARE?
>
> If you have a rollback before CPUHP_TIMERS_PREPARE you don't notice it
> and your worker may have skipped this work because it run on the wrong
> CPU. Also, I *think* that if you happen to have 64 interrupts between
>    CPUHP_AP_ONLINE_IDLE … CPUHP_AP_WORKQUEUE_ONLINE
>
> then the scheduled worker is unbound and may run on the "wrong" CPU.

I'm talking about in the context of unconditionally zeroing during
*startup*, just before CPUHP_TIMERS_PREPARE. That's what I sent in
this patch series:
https://lore.kernel.org/lkml/20220214151415.1108141-1-Jason@zx2c4.com/t/#u

Jason