HI Mark,
On Wed, Oct 26, 2016 at 04:14:31PM +0100, Mark Rutland wrote:
On Wed, Oct 26, 2016 at 02:10:24PM +0200, Antoine Tenart wrote:
Implement three atomic functions to allow making an atomic operation that returns the value. Adds: atomic_add_return(), atomic_sub_return(), atomic_inc_return() and atomic_dec_return().
Signed-off-by: Antoine Tenart antoine.tenart@free-electrons.com
In the cover letter, you mentioned that these are needed for SMP systems (for managing common suspend entry/exit management).
The below operations are *not* atomic in SMP systems, and can only work in UP. The same is true of all the existing code in the same file.
That's what I feared...
+static inline int atomic_add_return(int i, volatile atomic_t *v) +{
- unsigned long flags = 0;
- int ret;
- local_irq_save(flags);
- ret = (v->counter += i);
- local_irq_restore(flags);
- return ret;
+}
local_irq_{save,restore}() won't serialize two CPUs. Consider two CPUs executing this in parallel (assuming the compiler chooses a register rX as a temporary):
CPU0 CPU1
local_irq_save() local_irq_save() rX = v->counter rX = v->counter rX += i rX += i v->counter = rX v->counter = rX local_irq_restore() local_irq_restore()
At the end of this, CPU0's increment of v->counter is lost.
If you need atomics on SMP, you'll need to use the {load,store}-exclusive instructions, which come with a number of additional requirements (e.g. the memory being operated on must be mapped as write-back cacheable, the entire retry loop needs to be writtten in asm, etc).
Thanks a lot for the review and for the explanation.
So I need to do something like what's done in the kernel, at http://lxr.free-electrons.com/source/arch/arm/include/asm/atomic.h#L60 for ARM; by using ldrex and strex.
Thanks,
Antoine