On Wed, 8 Sep 2021, Michael Schmitz wrote:
On 08/09/21 11:50, Finn Thain wrote:
On Tue, 7 Sep 2021, Michael Schmitz wrote:
Does anyone know what causes this?
Our practice to run interrupt handlers at the IPL of the current
interrupt under service, instead of disabling local interrupts for
the duration of the handler?
Lock contention will happen anyway.
If using spin_trylock() outside of "atomic" context was a bug
(really?) then it can't be used here.
add_interrupt_randomness() relies on interrupts being disabled in
__handle_irq_event_percpu(), so this check assumes atomic context.
But __handle_irq_event_percpu() also assumes that local interrupts have
been disabled. There is a WARN_ONCE to that effect.
Our definition of irqs_disabled() invalidates that assumption.
Perhaps add_interrupt_randomness() should use the lock in irq mode,
like the rest of drivers/char/random.c does.
That might deadlock on SMP when a task reading from the random pool
holding the lock executes on the same CPU as the interrupt.
That does not make sense to me. add_interrupt_randomness() effectively
does acquire the lock in irq mode on SMP ports yet it doesn't deadlock.
In the UP case, the spinlock is optimized away,
Yes and spin_trylock() always succeeds. Hence CONFIG_SPINLOCK_DEBUG
asserts that the lock is never contended.
One thing that bothered me when I raised this issue was the apparent false
positive from CONFIG_SPINLOCK_DEBUG. But after sleeping on it, it makes
more sense to me.
(Perhaps the difficulty here is !SMP && !PREEMPT_COUNT. If PREEMPT_COUNT
was available on m68k then spin_trylock() might be expected to test
preempt_count, and the problem would not arise.)
BTW, the problem went away when I changed to irq mode locking to avoid any
lock contention, like so:
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 605969ed0f96..4e73c87cbdb8 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1248,6 +1248,7 @@ void add_interrupt_randomness(int irq, int irq_flags)
struct fast_pool *fast_pool = this_cpu_ptr(&irq_randomness);
struct pt_regs *regs = get_irq_regs();
unsigned long now = jiffies;
+ unsigned long flags;
cycles_t cycles = random_get_entropy();
__u32 c_high, j_high;
__u64 ip;
@@ -1281,12 +1282,12 @@ void add_interrupt_randomness(int irq, int irq_flags)
return;
r = &input_pool;
- if (!spin_trylock(&r->lock))
+ if (!spin_trylock_irqsave(&r->lock, flags))
return;
fast_pool->last = now;
__mix_pool_bytes(r, &fast_pool->pool, sizeof(fast_pool->pool));
- spin_unlock(&r->lock);
+ spin_unlock_irqrestore(&r->lock, flags);
fast_pool->count = 0;
