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.
