Hi Finn,
On 09/09/21 21:40, Finn Thain wrote:
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.
Yes, but that WARN_ON depends on irqs_disabled() returning true if and
only if interrupts are fully disabled, i.e. the IPL is 7. Our
implementation of irqs_disabled() returns true if the IPL is > 0 (in
general) or < 2 (Atari).
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.
It uses spin_trylock() while interrupts are disabled, so someone must
have worried about deadlocks there.
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.
That assertion fails here (IMO because we allow interrupts in code that
assumes they have been disabled, and tries unsuccessfully to assert
that), but that's OK in this case, as the code protected by the lock
isn't called if locking fails.
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;
You add a redundant interrupt disable/enable for SMP or PREEMPT arches.
That might be justified if it fixes a kernel crash, not merely a
CONFIG_DEBUG_SPINLOCK warning.
If we can add preempt count checking or actual spinlocks (without adding
too much overhead everywhere else), that might fix this issue without
need for a patch to random.c?
(But then, so would fixing irqs_disables(), or running interrupt
handlers with interrupts disabled...)
Cheers,
Michael
