[Adeos-main] non-atomic bitop in __adeos_stall_root()

[Adeos-main] non-atomic bitop in __adeos_stall_root()

[Michael Neuhauser]

Hello!

During my work on Adeos for ARM I've noticed the following: In
__adeos_stall_root() (both 2.4 & 2.6) the stall flag is set like this:

	adeos_get_cpu(flags);
	__set_bit(IPIPE_STALL_FLAG,&adp_root->cpudata[cpuid].status);
	adeos_put_cpu(flags);

I don't understand why the non-atomic __set_bit() is used instead of the
atomic set_bit(). Maybe someone can shed some light on this.

Suppose we have a non-SMP machine. In this case, adeos_get_cpu() is a
nop. Hence, hw-irqs may be enabled when __set_bit() is executed. On ARM,
__set_bit() is implemented as

	read from memory; modify register; write to memory

Assume an interrupt happens after the read and before the write and that
some other bit of "status" is modified during the interrupt
=> this modification is lost as the interrupted __set_bit() uses the
value of "status" before the interrupt!

This may not happen on i386 where __set_bit() is implemented as a single
asm instruction (which I assume is interrupt safe). But as
__adeos_stall_root() is contained in include/linux/adeos.h (i.e. generic
code) I think the atomic bitop has to be used.

__adeos_test_and_stall_root() and adeos_unstall_pipeline_from() also use
some non-atomic bitop without ensuring that hw-interrupts are disabled.

Mike
-- 
Dr. Michael Neuhauser                phone: +43 1 789 08 49 - 30
Firmix Software GmbH                   fax: +43 1 789 08 49 - 55
Vienna/Austria/Europe                      email: mike@domain.hid
Embedded Linux Development and Services    http://www.firmix.at/

Re: [Adeos-main] non-atomic bitop in __adeos_stall_root()

[Philippe Gerum]

On Thu, 2005-01-27 at 21:26, Michael Neuhauser wrote:
> Hello!
> 
> During my work on Adeos for ARM I've noticed the following: In
> __adeos_stall_root() (both 2.4 & 2.6) the stall flag is set like this:
> 
> 	adeos_get_cpu(flags);
> 	__set_bit(IPIPE_STALL_FLAG,&adp_root->cpudata[cpuid].status);
> 	adeos_put_cpu(flags);
> 
> I don't understand why the non-atomic __set_bit() is used instead of the
> atomic set_bit(). Maybe someone can shed some light on this.
> 
> Suppose we have a non-SMP machine. In this case, adeos_get_cpu() is a
> nop. Hence, hw-irqs may be enabled when __set_bit() is executed. On ARM,
> __set_bit() is implemented as
> 
> 	read from memory; modify register; write to memory
> 
> Assume an interrupt happens after the read and before the write and that
> some other bit of "status" is modified during the interrupt
> => this modification is lost as the interrupted __set_bit() uses the
> value of "status" before the interrupt!
> 
> This may not happen on i386 where __set_bit() is implemented as a single
> asm instruction (which I assume is interrupt safe). But as
> __adeos_stall_root() is contained in include/linux/adeos.h (i.e. generic
> code) I think the atomic bitop has to be used.
> 
> __adeos_test_and_stall_root() and adeos_unstall_pipeline_from() also use
> some non-atomic bitop without ensuring that hw-interrupts are disabled.
> 

Ack, it's a bug non-x86 wise; likely a collision between two past change
sets, namely using adeos_get_cpu() when adeos_lock_cpu() would have been
overkill on UP, and set/clear_bits() changed to __set/__clear_bits() for
the same reason given that the interrupts were guaranteed to be off at
that time. The problem is that #1 came after #2, and introduced this bug
recently.

> Mike
-- 

Philippe.

Re: [Adeos-main] non-atomic bitop in __adeos_stall_root()

[Philippe Gerum]

On Thu, 2005-01-27 at 21:26, Michael Neuhauser wrote:
> Hello!
> 
> During my work on Adeos for ARM I've noticed the following: In
> __adeos_stall_root() (both 2.4 & 2.6) the stall flag is set like this:
> 
> 	adeos_get_cpu(flags);
> 	__set_bit(IPIPE_STALL_FLAG,&adp_root->cpudata[cpuid].status);
> 	adeos_put_cpu(flags);
> 
> I don't understand why the non-atomic __set_bit() is used instead of the
> atomic set_bit(). Maybe someone can shed some light on this.
> 

In the case of __adeos_stall_root() and friends which all deal with the
current cpuid, we need to keep the adeos_lock_cpu/adeos_unlock_cpu
construct first to load the cpuid, then to guarantee that no CPU
migration will occur while fiddling with the array dereference.

> Suppose we have a non-SMP machine. In this case, adeos_get_cpu() is a
> nop. Hence, hw-irqs may be enabled when __set_bit() is executed. On ARM,
> __set_bit() is implemented as
> 
> 	read from memory; modify register; write to memory
> 
> Assume an interrupt happens after the read and before the write and that
> some other bit of "status" is modified during the interrupt
> => this modification is lost as the interrupted __set_bit() uses the
> value of "status" before the interrupt!
> 
> This may not happen on i386 where __set_bit() is implemented as a single
> asm instruction (which I assume is interrupt safe). But as
> __adeos_stall_root() is contained in include/linux/adeos.h (i.e. generic
> code) I think the atomic bitop has to be used.
> 
> __adeos_test_and_stall_root() and adeos_unstall_pipeline_from() also use
> some non-atomic bitop without ensuring that hw-interrupts are disabled.
> 
> Mike
-- 

Philippe.

Re: [Adeos-main] non-atomic bitop in __adeos_stall_root()

[Philippe Gerum]

On Thu, 2005-01-27 at 21:54, Philippe Gerum wrote:
> On Thu, 2005-01-27 at 21:26, Michael Neuhauser wrote:
> > Hello!
> > 
> > During my work on Adeos for ARM I've noticed the following: In
> > __adeos_stall_root() (both 2.4 & 2.6) the stall flag is set like this:
> > 
> > 	adeos_get_cpu(flags);
> > 	__set_bit(IPIPE_STALL_FLAG,&adp_root->cpudata[cpuid].status);
> > 	adeos_put_cpu(flags);
> > 
> > I don't understand why the non-atomic __set_bit() is used instead of the
> > atomic set_bit(). Maybe someone can shed some light on this.
> > 
> > Suppose we have a non-SMP machine. In this case, adeos_get_cpu() is a
> > nop. Hence, hw-irqs may be enabled when __set_bit() is executed. On ARM,
> > __set_bit() is implemented as
> > 
> > 	read from memory; modify register; write to memory
> > 
> > Assume an interrupt happens after the read and before the write and that
> > some other bit of "status" is modified during the interrupt
> > => this modification is lost as the interrupted __set_bit() uses the
> > value of "status" before the interrupt!
> > 
> > This may not happen on i386 where __set_bit() is implemented as a single
> > asm instruction (which I assume is interrupt safe). But as
> > __adeos_stall_root() is contained in include/linux/adeos.h (i.e. generic
> > code) I think the atomic bitop has to be used.
> > 
> > __adeos_test_and_stall_root() and adeos_unstall_pipeline_from() also use
> > some non-atomic bitop without ensuring that hw-interrupts are disabled.
> > 
> 
> Ack, it's a bug non-x86 wise; likely a collision between two past change
> sets, namely using adeos_get_cpu() when adeos_lock_cpu() would have been
> overkill on UP, and set/clear_bits() changed to __set/__clear_bits() for
> the same reason given that the interrupts were guaranteed to be off at
> that time. The problem is that #1 came after #2, and introduced this bug
> recently

Should be fixed in the CVS by now. I tried to refrain from using the
adeos_get_cpu/adeos_put_cpu construct but rather differenciate the
UP/SMP cases explicitely, because the former is confusing and leads to
less efficient code in the UP case. At some point in time, I guess that
we will need to get rid of them completely.

> .
> 
> > Mike
-- 

Philippe.