[RFC PATCH 0/2] powerpc: CR based local atomic operation implementation

[David Laight]

From: Madhavan Srinivasan
> On Thursday 27 November 2014 07:35 PM, David Laight wrote:
> > From: Madhavan Srinivasan
> >> This patchset create the infrastructure to handle the CR based
> >> local_* atomic operations. Local atomic operations are fast
> >> and highly reentrant per CPU counters.  Used for percpu
> >> variable updates. Local atomic operations only guarantee
> >> variable modification atomicity wrt the CPU which owns the
> >> data and these needs to be executed in a preemption safe way.
> >
> > These are usually called 'restartable atomic sequences (RAS)'.
> >
> >> Here is the design of the first patch. Since local_* operations
> >> are only need to be atomic to interrupts (IIUC), patch uses
> >> one of the Condition Register (CR) fields as a flag variable. When
> >> entering the local_*, specific bit in the CR5 field is set
> >> and on exit, bit is cleared. CR bit checking is done in the
> >> interrupt return path. If CR5[EQ] bit set and if we return
> >> to kernel, we reset to start of local_* operation.
> >
> > I don't claim to be able to read ppc assembler.
> > But I can't see the code that clears CR5[EQ] for the duration
> > of the ISR.
> I use crclr instruction at the end of the code block to clear the bit.
> 
> > Without it a nested interrupt will go through unwanted paths.

That crclr looks to be in the ISR exit path, you need one in the
isr entry path.

> >
> > There are also a lot of 'magic' constants in that assembly code.
> >
> All these constants are define in asm/ppc-opcode.h

I was thinking of the lines like:
+	ori	r3,r3,16384
This one probably deserves a comment - or something
+"3:"	PPC405_ERR77(0,%2)

> > I also wonder if it is possible to inspect the interrupted
> > code to determine the start/end of the RAS block.
> > (Easiest if you assume that there is a single 'write' instruction
> > as the last entry in the block.)
> >
> So each local_* function also have code in the __ex_table section. IIUC,
> __ex_table contains two address. So if the return address found in the
> first column of the _ex_table, use the corresponding address in the
> second column to continue from.

That really doesn't scale.
I don't know how many 1000 address pairs you table will have (and the
ones in each loadable module), but the search isn't going to be cheap.

If these sequences are restartable then they can only have one write
to memory.

Given your:
> This patch re-write the current local_* functions to CR5 based one.
> Base flow for each function is 
> 
> {
> 	set cr5(eq)
> 	load
> 	..
> 	store
> 	clear cr5(eq)
> }

On ISR entry:
If an ISR detects cr5(eq) set then look at the returned to instruction.
If it is 'clear cr5(eq)' do nothing.
Otherwise read backwards through the code (for a max of (say) 16 instructions)
searching for the 'set cr5(eq)' and change the return address to be that
of the instruction following the 'set cr5(eq)'.
In all cases clear cr5(eq) for the ISR itself (leave the saved value unchanged).

The you don't need a table of fault locations.

	David