Disabling interrupts to protect data

On Thu, 29 Oct 2009 21:39:03 +0000, ChrisQ <meru@devnull.com> wrote:

>A related problem is where a single shared variable at driver level that 
>needs to be accessed from different functions, all of which disable the 
>interrupt within the critical section. Disabling interrupts is fine, but 
>it is necessary to know at the time of reenabling any instance, what the 
>current interrupt state was for the device, prior to the call. The 
>solution was to write a couple of functions to stack the current device 
>interrupt state before disabling, then pop on exit, rather than just a 
>hard disable / reenable.
>
>I suppose this will turn out to be a standard technique, but new to me 
>at least...

I would not recommend that any function (despite some deeply internal
OS routines) should "enable" interrupts if it had not "disabled" them.

Of course, if the function may run in either context (i.e. interrupts
disabled or not) it should save the current state and restore it.

So no need for some kind of global variable to stack the interrupt
status.

BTW, the same is true for higher level locking using semaphores and
mutex.


-- 
42Bastian
Do not email to bastian42@yahoo.com, it's a spam-only account :-)
Use <same-name>@monlynx.de instead !

On Tue, 27 Oct 2009 22:07:40 +0100, David Brown
<david.brown@hesbynett.removethisbit.no> wrote:

>Reservations are hardware intensive to implement, unless you are willing 
>to pay extra memory latency.  But they scale well for multi-processor 
>systems, making them a good solution for "standard" PPC processors.  The 
>PPC core in the MPC55xx is more like a microcontroller core than a "big" 
>processor core, and Freescale probably saved a fair amount of silicon 
>and complexity by dropping reservations so that you must use traditional 
>interrupt disabling.

If FSL only would have placed a big sticker on the manual. :-)
Seriously, the manual states reservation works, and some place later
in the EBI (external bus interface) description they write it does not
work for EBI.
=> I learned, I have to read every single word of a manual.

-- 
42Bastian
Do not email to bastian42@yahoo.com, it's a spam-only account :-)
Use <same-name>@monlynx.de instead !

ChrisQ wrote:
> Stefan Reuther wrote:
> 
>> By "easy to get wrong" I mean, for example, that you have to tell all
>> your interrupt routines about your atomic_swap routine, so that they can
>> see that they've interrupted it and can complete or restart it. This is
>> nasty asm trickery. It enlarges all your interrupts by a dozen
>> instructions (at least some compares and jumps). And all that just
>> for a sticker "never disables interrupts" in the glossy brochures?
>> Sorry, I'd rather take the additional three-cycle interrupt latency for
>> running atomic_link under CLI.
> 
> A related problem is where a single shared variable at driver level that 
> needs to be accessed from different functions, all of which disable the 
> interrupt within the critical section. Disabling interrupts is fine, but 
> it is necessary to know at the time of reenabling any instance, what the 
> current interrupt state was for the device, prior to the call. The 
> solution was to write a couple of functions to stack the current device 
> interrupt state before disabling, then pop on exit, rather than just a 
> hard disable / reenable.
> 
> I suppose this will turn out to be a standard technique, but new to me 
> at least...
> 

Do you mean something like this :

void foo(void) {
	doSomethingUnlocked();
	memoryBarrier();
	oldSREG = SREG;		// Preserve old interrupt state
	cli();			// Ensure interrupts are disabled now
	doSomethingLocked();
	memoryBarrier();
	SREG = oldSREG;		// Restore old interrupt flag
	doMoreUnlocked();
}

The details vary from processor to processor, but the principle is the 
same.

Some compilers have extensions or extra headers making it easier.  For 
example, avr-gcc standard library has ATOMIC_BLOCK macros for this 
purpose, and msp430-gcc has an extra "critical" function __attribute__ 
that wraps a function in an interrupt disable/restore pair.

Stefan Reuther wrote:
> D Yuniskis wrote:
>> Stefan Reuther wrote:
>>> D Yuniskis wrote:
<snip>

Before I comment on your code below, let me first say I agree entirely 
that it is almost always better to have a small but clear and 
predictable interrupt latency using CLI/STI that you can easily see 
works correctly, than to have a complex scheme that whose correctness is 
difficult to evaluate.  If your system cannot tolerate the small extra 
interrupt latency caused by such interrupt disable blocks, make a faster 
system or move the critical parts to better hardware.

(Note that this is in the context of microcontroller programming - if 
you are working with large systems, especially SMP systems, there are 
many more factors to consider.)

> Exactly this "append a pointer" thing is where an atomic operation can
> come in very handy. You shouldn't append to the list while it's being
> processed, nor should you interrupt another instance in another
> interrupt (if you have nested interrupts).
> 
> All this is totally simple when you can use CLI/STI:
>    void atomic_swap(node** a, node** b, node* c) {
>       cli(); *a = *b; *b = c; sti();
>    }
> 
>    // prepend new node to a list (easier than appending...)
>    node* list;
>    node* newNode;
>    atomic_swap(&newNode->next, &list, newNode);
> 
>    // get list (for processing) and make it empty
>    node* oldList;
>    atomic_swap(&oldList, &list, 0);
> (some strategically-placed 'volatile' can be appropriate.)
> 

Here you have missed one important point that many people fail to 
understand about interrupt disabling (and the related topic of volatile 
accesses) - memory barriers.  Unless your cli() and sti() macros / 
assembly functions specifically include memory barriers, then your 
atomic_swap function is not safe.

The problem is that C does not have any requirements about ordering of 
operations except for volatile accesses - all it requires is that the 
program has the same effect as though the operations were done in order, 
running on a C abstract machine.  But the abstract machine has no 
concepts of interrupts, multi-threading, or external input and output 
and peripherals.  The compiler can freely make re-arrangements to 
improve the quality of the generated code, which are correct for the 
abstract machine but may not be correct for your actual code.

In the atomic_swap function above, for example, the compiler will 
probably see the cli() and sti() as volatile accesses (depending on the 
compiler and the definition of these macros).  But the code in the 
middle does not use volatile accesses - the compiler can therefore 
freely move these operations before or after the cli() and sti() 
operations.  To avoid this, you need a memory barrier before and after 
these operations, to tell the compiler that memory reads before the 
first barrier are now invalid, and that any pending memory writes must 
be completed before the second barrier.

A typical memory barrier on gcc is a memory clobber:
	"asm volatile ("" : : : "memory");

On some systems, you may need to issue processor instructions too (such 
as a "msync" or "mbar" on a PPC) to deal with pipelined write buffers, 
but that's not typically the case for small microcontrollers.

Some compilers will always consider inline assembly as a memory barrier 
- that makes it easier to write safe code in this case, but harder to 
use inline assembly optimally in other cases.  Either way, you have to 
know what your compiler is doing.

David Brown wrote:

>>
> 
> Do you mean something like this :
> 
> void foo(void) {
>     doSomethingUnlocked();
>     memoryBarrier();
>     oldSREG = SREG;        // Preserve old interrupt state
>     cli();            // Ensure interrupts are disabled now
>     doSomethingLocked();
>     memoryBarrier();
>     SREG = oldSREG;        // Restore old interrupt flag
>     doMoreUnlocked();
> }
> 
> The details vary from processor to processor, but the principle is the 
> same.
> 
> Some compilers have extensions or extra headers making it easier.  For 
> example, avr-gcc standard library has ATOMIC_BLOCK macros for this 
> purpose, and msp430-gcc has an extra "critical" function __attribute__ 
> that wraps a function in an interrupt disable/restore pair.

Yes, but using an array, rather than a single variable. The code looked 
like:


static U8 u8PwmInterruptStack [SIZE];
static U8 u8PwmStackPointer = 0;

/* Push Pwm Interrupt State and Mask Interrupts */
/* -------------------------------------------- */
void vPwmPushMaskInterruptState (void) {

if (u8PwmStackPointer < PWM_STACKDEPTH) {
    u8PwmInterruptStack [u8PwmStackPointer++] = u8PwmReadIntState ();
    vPwmMaskInterrupts ();
    }
}

/* Pop Pwm Interrupt State */
/* ----------------------- */
void vPwmPopInterruptState (void) {

if (u8PwmStackPointer > 0) {
    u8PwmStackPointer--;
    if ((u8PwmInterruptStack [u8PwmStackPointer] & PWM_INTMASK) != 0) {
       vPwmUnmaskInterrupts ();
       }
    }
}

The range check is just there for safety and in practice, the array is 
sized to suit the application. The first function is called before 
twiddling the shared data and second after.

Turned out to be a simple solution, but the first time i've had to use 
anything like it...

Regards,

Chris

ChrisQ wrote:
> David Brown wrote:
> 
>>>
>>
>> Do you mean something like this :
>>
>> void foo(void) {
>>     doSomethingUnlocked();
>>     memoryBarrier();
>>     oldSREG = SREG;        // Preserve old interrupt state
>>     cli();            // Ensure interrupts are disabled now
>>     doSomethingLocked();
>>     memoryBarrier();
>>     SREG = oldSREG;        // Restore old interrupt flag
>>     doMoreUnlocked();
>> }
>>
>> The details vary from processor to processor, but the principle is the 
>> same.
>>
>> Some compilers have extensions or extra headers making it easier.  For 
>> example, avr-gcc standard library has ATOMIC_BLOCK macros for this 
>> purpose, and msp430-gcc has an extra "critical" function __attribute__ 
>> that wraps a function in an interrupt disable/restore pair.
> 
> Yes, but using an array, rather than a single variable. The code looked 
> like:
> 
> 
> static U8 u8PwmInterruptStack [SIZE];
> static U8 u8PwmStackPointer = 0;
> 
> /* Push Pwm Interrupt State and Mask Interrupts */
> /* -------------------------------------------- */
> void vPwmPushMaskInterruptState (void) {
> 
> if (u8PwmStackPointer < PWM_STACKDEPTH) {
>    u8PwmInterruptStack [u8PwmStackPointer++] = u8PwmReadIntState ();
>    vPwmMaskInterrupts ();
>    }
> }
> 
> /* Pop Pwm Interrupt State */
> /* ----------------------- */
> void vPwmPopInterruptState (void) {
> 
> if (u8PwmStackPointer > 0) {
>    u8PwmStackPointer--;
>    if ((u8PwmInterruptStack [u8PwmStackPointer] & PWM_INTMASK) != 0) {
>       vPwmUnmaskInterrupts ();
>       }
>    }
> }
> 
> The range check is just there for safety and in practice, the array is 
> sized to suit the application. The first function is called before 
> twiddling the shared data and second after.
> 
> Turned out to be a simple solution, but the first time i've had to use 
> anything like it...
> 
> Regards,
> 
> Chris
> 

This may sound like a silly question, but why don't you just use a local 
variable to hold the old interrupt status?  It is rare that you would 
want to disable interrupts in one function, and restore the state in a 
different function - since you want to keep the disable time to a 
minimum, and be very clear about when they are disabled or not, your 
disable and enable are typically done at the same level.  Thus you have

{
   uint8_t oldState = u8PwmReadIntState ();
   vPwmMaskInterrupts ()
   doSomething();
   if ((oldState & PWM_INTMASK) != 0) {
      vPwmUnmaskInterrupts ();
   }
}

The code in doSomething() could quite happily have the same sort of 
sequence.  There is no need to have a special stack - normal locals suffice.


If you have a complex system with particular interrupts (rather than 
global ones) enabled and disabled from different threads in a program, 
then local variables like that won't be sufficient.  But I'd first 
suggest thinking hard about the structure of the program - there are 
probably better, clearer and therefore safer ways to get the effect you 
want.  However, if you want to do something like that, you don't need a 
stack - you just need a counter variable tracking the number of times 
the interrupt has been disabled.  Of course, you need to disable global 
interrupts around access to this counter...

On 30 Okt., 07:45, bastia...@yahoo.com (42Bastian Schick) wrote:
> On Tue, 27 Oct 2009 22:07:40 +0100, David Brown
>
> <david.br...@hesbynett.removethisbit.no> wrote:
> >Reservations are hardware intensive to implement, unless you are willing
> >to pay extra memory latency. =A0But they scale well for multi-processor
> >systems, making them a good solution for "standard" PPC processors. =A0T=
he
> >PPC core in the MPC55xx is more like a microcontroller core than a "big"
> >processor core, and Freescale probably saved a fair amount of silicon
> >and complexity by dropping reservations so that you must use traditional
> >interrupt disabling.
>
> If FSL only would have placed a big sticker on the manual. :-)
> Seriously, the manual states reservation works, and some place later
> in the EBI (external bus interface) description they write it does not
> work for EBI.
> =3D> I learned, I have to read every single word of a manual.
>
> --
> 42Bastian
> Do not email to bastia...@yahoo.com, it's a spam-only account :-)
> Use <same-name>@monlynx.de instead !

I remember reading somewhere, something like Motorola documentation
departments being the only US organizations running with CMMI level 5.
Their documents suck with a high standard definitively. :)

Mark

David Brown wrote:

> 
> This may sound like a silly question, but why don't you just use a local 
> variable to hold the old interrupt status?  It is rare that you would 
> want to disable interrupts in one function, and restore the state in a 
> different function - since you want to keep the disable time to a 
> minimum, and be very clear about when they are disabled or not, your 
> disable and enable are typically done at the same level.  Thus you have

The product is typically a 128 x 192 x 2 colour led pixel graphics 
display, driven via serial comms at 9.6k. The leds are driven from ti 
led drivers, which handle 8 leds each, and use a set of serial control 
lines to clock and latch data and switch between data transfer and self 
test modes. Intensity is done via pwm on an enable line, which itself is 
one of the control lines. The absolute requirement to avoid any flicker 
at all on the display means that most control functions and data 
transfers can only be done in interrupt context, synchronised to the pwm 
off period. From a programming point of view, it's not ideal but that's 
the hardware that we had to work with. There's no rtos, rather state 
driven, with more state machinery in the comms, data transfer and data 
self test areas.

The software was developed over 2 years and now runs on many variants. 
Problem was that there were many layers of code and an overall lock 
requirement that was difficult to resolve across the layers. The only 
way to keep track was to save and restore context at each point. Yes, 
you could do this with a local variable in each function, but it is more 
code and more bug prone than a single line function call for disable and 
enable. The functionality is encapsulated and I don't have to think 
about the internals, nor remember how many instances there are in the 
code if it needs to be changed. Run time speed is not an issue either, 
so an added function call or so is irrelevant.

Ideal world rarely maps onto real products in practice :-)...

Regards,

Chris

David Brown wrote:
> Stefan Reuther wrote:
>> All this is totally simple when you can use CLI/STI:
>>    void atomic_swap(node** a, node** b, node* c) {
>>       cli(); *a = *b; *b = c; sti();
>>    }
>>
>>    // prepend new node to a list (easier than appending...)
>>    node* list;
>>    node* newNode;
>>    atomic_swap(&newNode->next, &list, newNode);
>> (some strategically-placed 'volatile' can be appropriate.)
> 
> Here you have missed one important point that many people fail to
> understand about interrupt disabling (and the related topic of volatile
> accesses) - memory barriers.  Unless your cli() and sti() macros /
> assembly functions specifically include memory barriers, then your
> atomic_swap function is not safe.

Yep. The declaration should've been
   void atomic_swap(node*volatile* a, node*volatile* b, node* c);
I was just too lazy to figure out where to place the 'volatile's :-)
That, together with cli/sti macros acting as memory barriers (honestly,
cli/sti which are not such barriers are useless) fixes the routine for a
single processor system. Multiprocessor is a little harder.

> In the atomic_swap function above, for example, the compiler will
> probably see the cli() and sti() as volatile accesses (depending on the
> compiler and the definition of these macros).  But the code in the
> middle does not use volatile accesses - the compiler can therefore
> freely move these operations before or after the cli() and sti()
> operations.

Actually, our compiler moved the accesses even though I used 'volatile'.
Fortunately the vendor fixed it quickly, but in the meantime I've made
an asm version to fix it forever.


  Stefan

David Brown wrote:

> {
>   uint8_t oldState = u8PwmReadIntState ();
>   vPwmMaskInterrupts ()

That won't work, because it has the exact same problem that the concept 
of a critical section is supposed to work against.  Interrupt state can 
change between these two instructions, and that would leave you 
restoring an out-of-date oldState.

Ultimately, you need atomic query-and-change operations to resolve this.