Re: [U-Boot] [PATCH v3 01/10] arm: add atomic functions with return support

[Antoine Tenart]

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 
> 
> 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):
> 
>   CPU0CPU1
> 
>   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

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 01/10] arm: add atomic functions with return support

[Mark Rutland]

Hi,

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 

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.

> +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):

CPU0CPU1

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,
Mark.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v3 01/10] arm: add atomic functions with return support

[Antoine Tenart]

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 
---
 arch/arm/include/asm/atomic.h | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index 171f4d979281..0d9a6e3901e2 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -46,6 +46,18 @@ static inline void atomic_add(int i, volatile atomic_t *v)
local_irq_restore(flags);
 }
 
+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;
+}
+
 static inline void atomic_sub(int i, volatile atomic_t *v)
 {
unsigned long flags = 0;
@@ -55,6 +67,18 @@ static inline void atomic_sub(int i, volatile atomic_t *v)
local_irq_restore(flags);
 }
 
+static inline int atomic_sub_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;
+}
+
 static inline void atomic_inc(volatile atomic_t *v)
 {
unsigned long flags = 0;
@@ -64,6 +88,11 @@ static inline void atomic_inc(volatile atomic_t *v)
local_irq_restore(flags);
 }
 
+static inline int atomic_inc_return(volatile atomic_t *v)
+{
+   return atomic_add_return(1, v);
+}
+
 static inline void atomic_dec(volatile atomic_t *v)
 {
unsigned long flags = 0;
@@ -73,6 +102,11 @@ static inline void atomic_dec(volatile atomic_t *v)
local_irq_restore(flags);
 }
 
+static inline int atomic_dec_return(volatile atomic_t *v)
+{
+   return atomic_sub_return(1, v);
+}
+
 static inline int atomic_dec_and_test(volatile atomic_t *v)
 {
unsigned long flags = 0;
-- 
2.10.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot