Re: RFC: Proper locking tips for IRQ handlers?

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]

Hi Joshua,

On Sat, Mar 04, 2017 at 10:17:41AM -0500, Joshua Kinard wrote:
> I am looking for some feedback on how to improve IRQ handlers on IP30 (SGI
> Octane).  Since switching to the use of per_cpu data structures, I've
> apparently not needed to do any locking when handling IRQs.  However, it looks
> like around ~Linux-4.8.x, there's some noticeable contention going on with
> heavy disk I/O, and it's worse in 4.9 and 4.10.  Under 4.8, untar'ing a basic
> Linux kernel source tarball completed fine, but as of 4.9/4.10, there's a
> chance to trigger either an oops or oom killer.  While the untar operation
> happens, I can watch the graphics console and the blinking cursor will
> sometimes pause momentarily, which suggests contention somewhere.
> 
> So my question is in struct irq_chip accessors like irq_ack, irq_mask,
> irq_mask_ack, etc, should spinlocks be used prior to accessing the HEART ASIC?
> And if so, normal spinlocks, raw spinlocks, or the irq save/restore variants?

On RT kernels spinlock_t becomes a mutex and won't work correctly with
IRQs actually disabled for the IRQ callbacks, so raw spinlocks would be
needed instead.

To decide which variant, a good reference is:
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/index.html
in particular the cheat sheet for locking:
https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html

(I don't know OTOH which contexts these callbacks are called from,
definitely hard IRQ, but possibly others when registering IRQs etc).

If its only protecting access to per-CPU structures though you shouldn't
need the spinlock part of the locks (but does it not access hardware
registers too, or are those accesses atomic in hardware rather than
software doing read-modify-write?). In that case you could choose to use
the following instead of locks depending on the contexts its used in:

- spin_lock_irqsave/restore -> local_irq_save/restore +
				preempt_disable/enable
- spin_lock_irq -> local_irq_disable/enable +
				preempt_disable/enable
- spin_lock_bh -> local_bh_disable/enable
- spin_lock -> preempt_disable/enable

Hope that helps a bit

Cheers
James

> I've looked at the IRQ implementations for a few other MIPS boards, but because
> each system is so unique, there's no clear consensus on the right way to do it.
>  Octeon, uses irq_bus_lock and irq_bus_sync_unlock, along with a mutex, for
> locking, while netlogic uses locking only during irq_enable or irq_disable, and
> loongsoon-3 doesn't appear to employ any locking at all.  Other archs appear to
> be a mix as well, and I'm even seeing newer constructs like the use of
> irq_data_get_irq_chip_data() to fetch a common data structure where the
> spinlock variable is defined.  Other systems use a global spinlock.
> 
> Any advice would be greatly appreciated.  Thanks!
> 
> -- 
> Joshua Kinard
> Gentoo/MIPS
> kumba@gentoo.org
> 6144R/F5C6C943 2015-04-27
> 177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943
> 
> "The past tempts us, the present confuses us, the future frightens us.  And our
> lives slip away, moment by moment, lost in that vast, terrible in-between."
> 
> --Emperor Turhan, Centauri Republic
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]