Re: [PATCH RFC] locking: Add volatile to arch_spinlock_t structures

[Paul E. McKenney]

One point of the patch is "one more thing to watch for" in generated code,
namely temporary clobbering of synchronization variables, locks included,
due to overzealous optimization.  If this happens to the kernel, I guess
that the other quick workaround is to add alignment directives or padding.
(And yes, I have seen reports of non-kernel examples where gcc actually
does this sort of thing.)

But I don't feel strongly about this patch, not yet anyway, so will set
it aside for the time being.

							Thanx, Paul

On Wed, Dec 03, 2014 at 10:31:06PM -0800, Linus Torvalds wrote:
> NAK. Volatile on data structures (as opposed to in code) is a disease. This
> is wrong.
> 
> We very much pass spinlock values around. They aren't volatile. The only
> thing volatile is when you load the value from the lock itself. See for
> example the stuff we do when we load a reflock from memory. Once we've
> loaded the value into registers it's no longer volatile, and putting
> violators into the data structures is actively *wrong*.
> 
> So no, no, no. C got this wrong. Volatile data structures are a fundamental
> mistake and a bug. The only good volatile is in an access (ie like
> ACCESS_ONCE() and anything else is making things worse.
> 
> We're not catering to crazy compiler people that can't understand that
> fact. I'm not even seeing the point of your patch, since it seems to be
> entirely about "let's make excuses for crap compilers"
> 
>    Linus
> On Dec 3, 2014 10:20 PM, "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> wrote:
> 
> > More concern about compilers...
> >
> > Most architectures mark the fields in their arch_spinlock_t structures
> > as volatile, which forces the compiler to respect the integrity of
> > those fields.  Without volatile markings, the compiler is within its
> > rights to overwrite these fields, for example, by using a wide store
> > to update an adjacent field as long as it fixes them up later.  This
> > sort of thing could come as a rather nasty surprise to any task attempting
> > to concurrently acquire that lock.
> >
> > For example, on x86 for smallish NR_CPUS, arch_spinlock_t is 16 bits wide.
> > If there were three adjacent 16-bit fields in the structure containing
> > the arch_spinlock_t, a compiler might reasonably use 64-bit accesses
> > for those three fields.  After a 64-bit load, the arch_spinlock_t's
> > value would be available in a register, so that the compiler might use
> > a 64-bit store followed by a 16-bit fixup store to update the three
> > adjacent 16-bit fields.
> >
> > This commit therefore adds volatile to the arch_spinlock_t and
> > arch_rwlock_t fields that don't already have them.
> >
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > Cc: linux-arch@xxxxxxxxxxxxxxx
> > Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> > Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> >
> > diff --git a/arch/arm/include/asm/spinlock_types.h
> > b/arch/arm/include/asm/spinlock_types.h
> > index 47663fcb10ad..7d3b6ecb5301 100644
> > --- a/arch/arm/include/asm/spinlock_types.h
> > +++ b/arch/arm/include/asm/spinlock_types.h
> > @@ -9,14 +9,14 @@
> >
> >  typedef struct {
> >         union {
> > -               u32 slock;
> > +               volatile u32 slock;
> >                 struct __raw_tickets {
> >  #ifdef __ARMEB__
> > -                       u16 next;
> > -                       u16 owner;
> > +                       volatile u16 next;
> > +                       volatile u16 owner;
> >  #else
> > -                       u16 owner;
> > -                       u16 next;
> > +                       volatile u16 owner;
> > +                       volatile u16 next;
> >  #endif
> >                 } tickets;
> >         };
> > @@ -25,7 +25,7 @@ typedef struct {
> >  #define __ARCH_SPIN_LOCK_UNLOCKED      { { 0 } }
> >
> >  typedef struct {
> > -       u32 lock;
> > +       volatile u32 lock;
> >  } arch_rwlock_t;
> >
> >  #define __ARCH_RW_LOCK_UNLOCKED                { 0 }
> > diff --git a/arch/arm64/include/asm/spinlock_types.h
> > b/arch/arm64/include/asm/spinlock_types.h
> > index b8d383665f56..0f841378f0f3 100644
> > --- a/arch/arm64/include/asm/spinlock_types.h
> > +++ b/arch/arm64/include/asm/spinlock_types.h
> > @@ -24,11 +24,11 @@
> >
> >  typedef struct {
> >  #ifdef __AARCH64EB__
> > -       u16 next;
> > -       u16 owner;
> > +       volatile u16 next;
> > +       volatile u16 owner;
> >  #else
> > -       u16 owner;
> > -       u16 next;
> > +       volatile u16 owner;
> > +       volatile u16 next;
> >  #endif
> >  } __aligned(4) arch_spinlock_t;
> >
> > diff --git a/arch/mips/include/asm/spinlock_types.h
> > b/arch/mips/include/asm/spinlock_types.h
> > index 9b2528e612c0..10f04a0482dd 100644
> > --- a/arch/mips/include/asm/spinlock_types.h
> > +++ b/arch/mips/include/asm/spinlock_types.h
> > @@ -14,14 +14,14 @@ typedef union {
> >          * bits  0..15 : serving_now
> >          * bits 16..31 : ticket
> >          */
> > -       u32 lock;
> > +       volatile u32 lock;
> >         struct {
> >  #ifdef __BIG_ENDIAN
> > -               u16 ticket;
> > -               u16 serving_now;
> > +               volatile u16 ticket;
> > +               volatile u16 serving_now;
> >  #else
> > -               u16 serving_now;
> > -               u16 ticket;
> > +               volatile u16 serving_now;
> > +               volatile u16 ticket;
> >  #endif
> >         } h;
> >  } arch_spinlock_t;
> > diff --git a/arch/mn10300/include/asm/spinlock_types.h
> > b/arch/mn10300/include/asm/spinlock_types.h
> > index 653dc519b405..04fd2c622f81 100644
> > --- a/arch/mn10300/include/asm/spinlock_types.h
> > +++ b/arch/mn10300/include/asm/spinlock_types.h
> > @@ -6,13 +6,13 @@
> >  #endif
> >
> >  typedef struct arch_spinlock {
> > -       unsigned int slock;
> > +       volatile unsigned int slock;
> >  } arch_spinlock_t;
> >
> >  #define __ARCH_SPIN_LOCK_UNLOCKED      { 0 }
> >
> >  typedef struct {
> > -       unsigned int lock;
> > +       volatile unsigned int lock;
> >  } arch_rwlock_t;
> >
> >  #define __ARCH_RW_LOCK_UNLOCKED                { RW_LOCK_BIAS }
> > diff --git a/arch/s390/include/asm/spinlock_types.h
> > b/arch/s390/include/asm/spinlock_types.h
> > index d84b6939237c..0ccdda3b6842 100644
> > --- a/arch/s390/include/asm/spinlock_types.h
> > +++ b/arch/s390/include/asm/spinlock_types.h
> > @@ -6,14 +6,14 @@
> >  #endif
> >
> >  typedef struct {
> > -       unsigned int lock;
> > +       volatile unsigned int lock;
> >  } __attribute__ ((aligned (4))) arch_spinlock_t;
> >
> >  #define __ARCH_SPIN_LOCK_UNLOCKED { .lock = 0, }
> >
> >  typedef struct {
> > -       unsigned int lock;
> > -       unsigned int owner;
> > +       volatile unsigned int lock;
> > +       volatile unsigned int owner;
> >  } arch_rwlock_t;
> >
> >  #define __ARCH_RW_LOCK_UNLOCKED                { 0 }
> > diff --git a/arch/tile/include/asm/spinlock_types.h
> > b/arch/tile/include/asm/spinlock_types.h
> > index a71f59b49c50..29f70a14b979 100644
> > --- a/arch/tile/include/asm/spinlock_types.h
> > +++ b/arch/tile/include/asm/spinlock_types.h
> > @@ -23,14 +23,14 @@
> >
> >  /* Low 15 bits are "next"; high 15 bits are "current". */
> >  typedef struct arch_spinlock {
> > -       unsigned int lock;
> > +       volatile unsigned int lock;
> >  } arch_spinlock_t;
> >
> >  #define __ARCH_SPIN_LOCK_UNLOCKED      { 0 }
> >
> >  /* High bit is "writer owns"; low 31 bits are a count of readers. */
> >  typedef struct arch_rwlock {
> > -       unsigned int lock;
> > +       volatile unsigned int lock;
> >  } arch_rwlock_t;
> >
> >  #define __ARCH_RW_LOCK_UNLOCKED                { 0 }
> > @@ -39,9 +39,9 @@ typedef struct arch_rwlock {
> >
> >  typedef struct arch_spinlock {
> >         /* Next ticket number to hand out. */
> > -       int next_ticket;
> > +       volatile int next_ticket;
> >         /* The ticket number that currently owns this lock. */
> > -       int current_ticket;
> > +       volatile int current_ticket;
> >  } arch_spinlock_t;
> >
> >  #define __ARCH_SPIN_LOCK_UNLOCKED      { 0, 0 }
> > diff --git a/arch/x86/include/asm/spinlock_types.h
> > b/arch/x86/include/asm/spinlock_types.h
> > index 5f9d7572d82b..fead0ca82468 100644
> > --- a/arch/x86/include/asm/spinlock_types.h
> > +++ b/arch/x86/include/asm/spinlock_types.h
> > @@ -25,9 +25,9 @@ typedef u32 __ticketpair_t;
> >
> >  typedef struct arch_spinlock {
> >         union {
> > -               __ticketpair_t head_tail;
> > +               volatile __ticketpair_t head_tail;
> >                 struct __raw_tickets {
> > -                       __ticket_t head, tail;
> > +                       volatile __ticket_t head, tail;
> >                 } tickets;
> >         };
> >  } arch_spinlock_t;
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/