[PATCH 3/4] ARM: perf: check that we have an event in the PMU IRQ handlers

[Will Deacon]

On Mon, Feb 27, 2012 at 09:56:46AM +0000, Ming Lei wrote:
> Will Deacon <will.deacon at arm.com> wrote:
> > On Fri, Feb 24, 2012 at 01:34:32AM +0000, Ming Lei wrote:
> > > 
> > > I think we should check it via test_bit(idx, cpuc->used_mask) because
> > > 'hw_events->events[idx] = val' is not atomic operation and it is read here
> > > in irq context.
> > 
> > I dunno, that code is compiled to:
> > 
> > e5973000        ldr     r3, [r7]
> > e7834106        str     r4, [r3, r6, lsl #2]
> > 
> > so you should either see the new value or the old one - you can't see half a
> > pointer in there since it's a single 32-bit store.
> 
> Firstly the code above is only generated from one of many existing compile
> options, for example, maybe storing to 32bit variable involves two instructions
> in thumb mode, so are you sure it is always OK for all cases?

I'll check with the compiler guys if the tools ever do this. It would seem a
bit weird given that we may have things like memory type, alignment and
endianness coming into play that would make splitting up 32-bit stores a
really bad idea.

> Secondly, I am even not sure if ARM irq handler is always triggered in instruction
> boundary, maybe the irq handler is started during execution of the store instruction.
> In fact, I can find the sentence below in 'B1.6.16 IRQ exception' of ARMv7 manual:
> 
> 	This relaxation of the normal definition of a precise asynchronous exception
> 	permits interrupts to occur during the execution of instructions that change register
> 	or memory values, while only requiring the implementation to restore those register
> 	values that are needed to correctly re-execute the instruction after the preferred
> 	exception return. LDM and STM are examples of such instructions.

A single 32-bit store is not interruptible in the same way as load/store
multiple instructions are, so this isn't a problem.

> So suggest to take the correct way in theory, IMO.

I don't like checking the used_mask, because then we have to throw in some
barriers to ensure ordering between updates of the event arrays and the
mask. If we do have to change something, I think putting ACCESS_ONCE around
updates to the event array should do the trick.

Cheers,

Will