Re: [PATCH 3/3] random: use trylock in irq handler rather than spinning

[Dominik Brodowski]

Am Sun, Feb 13, 2022 at 12:10:22AM +0100 schrieb Jason A. Donenfeld:
> crng_pre_init_inject() (and prior crng_fast_load()) uses a trylock when
> in fast mode, so that it never contends. We should be doing the same
> when grabbing a spinlock for mixing into the entropy pool. So switch to
> doing that before calling the underscored _mix_pool_bytes().
> 
> Cc: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx>
> Cc: Theodore Ts'o <tytso@xxxxxxx>
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> ---
>  drivers/char/random.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 9a8e1bb9845d..ca224c3f2561 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1286,13 +1286,10 @@ void add_interrupt_randomness(int irq)
>  			atomic_set(&fast_pool->count, 0);
>  			fast_pool->last = now;
>  
> -			/*
> -			 * Technically this call means that we're using a spinlock_t
> -			 * in the IRQ handler, which isn't terrific for PREEMPT_RT.
> -			 * However, this only happens during boot, and then never
> -			 * again, so we live with it.
> -			 */
> -			mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
> +			if (spin_trylock(&input_pool.lock)) {
> +				_mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool));
> +				spin_unlock(&input_pool.lock);
> +			}

You're still using a spinlock_t here, so I don't see a need to remove the
comment. Also, I'm not super happy that the count is re-set to 0 but the
input remains unused. Maybe the better approach here is, as discussed in the
other thread, to always use the workqueue mechanism, which would allow us to
streamline the code further.

Thanks,
	Dominik