RFC: Proper locking tips for IRQ handlers?

RFC: Proper locking tips for IRQ handlers?

[Joshua Kinard]

Hi all,

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?

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

Re: RFC: Proper locking tips for IRQ handlers?

[James Hogan]

[-- 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 --]

Re: RFC: Proper locking tips for IRQ handlers?

[Joshua Kinard]

On 03/06/2017 06:40, James Hogan wrote:
> 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.

Okay, that's a really good data point to be aware of.  I haven't tried RT
kernels on any SGI systems, so I wasn't aware of that.


> 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 have always found that cheat sheet table difficult to decipher.  Probably
from a lack of understanding of all the intricate ways locks can be utilized.


> (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

I dumped a lot of the older locking code after learning about per_cpu and
following some of the header comments on how to properly do per_cpu reads and
writes to avoid preemption issues....not that I've tested preemption out on
these systems either.

Access to HEART is done using simple aliases to ____raw_readq (as heart_read)
and ____raw_writeq (as heart_write).  As far as I can tell, these don't do any
IRQ enabling/disabling since they're used in the places where I *think* the
local IRQs are already disabled (is there a way to check this?), however,
documentation on these versions of readq/writeq is somewhat scant, and the only
real hit on them is from 2004:

https://www.linux-mips.org/archives/linux-mips/2004-01/msg00288.html

I got rid of a lot of the direct pointer access methods like *foo |= bar; when
writing stuff back to HEART, replacing with the aliased heart_read/heart_write
methods, so that should avoid RMW issues.

That said, I'm not even 100% sure locking is the cause of this recent issue
(assuming for a moment it's not tied to my known BRIDGE/PCI issues).
CONFIG_DEBUG_LOCK_ALLOC can't be used on these platforms because the resultant
kernel is apparently too large for ARCS to stick into a "FreeMemory" area.
However, I've stripped kernels down to ~9MB with CONFIG_DEBUG_LOCK_ALLOC set
and ARCS still refuses to load it, so I suspect it's a specific section of the
ELF binary that ARCS can't load properly with that option enabled.  Even
arcload (ARCS bootloader written by Stan) can't get around it.  That's been a
major reason why I can't fully verify that I'm doing locking correctly.

--J


> 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!