[PATCH 0/3] XSA-114 follow-ups

[PATCH 0/3] XSA-114 follow-ups

[Jan Beulich]

1: spinlock: use local_irq_disable() instead of local_irq_save() where possible
2: rwlock: allow arch to override read_unlock() atomic
3: rwlock: allow arch to override write_unlock() atomic

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/3] spinlock: use local_irq_disable() instead of local_irq_save() where possible

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

... as generally being a cheaper operation.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -162,7 +162,7 @@ unsigned long _spin_lock_irqsave(spinloc
         local_irq_restore(flags);
         while ( likely(_raw_spin_is_locked(&lock->raw)) )
             cpu_relax();
-        local_irq_save(flags);
+        local_irq_disable();
     }
     LOCK_PROFILE_GOT;
     preempt_disable();
@@ -313,7 +313,7 @@ unsigned long _read_lock_irqsave(rwlock_
             local_irq_restore(flags);
             while ( (x = lock->lock) & RW_WRITE_FLAG )
                 cpu_relax();
-            local_irq_save(flags);
+            local_irq_disable();
         }
     } while ( cmpxchg(&lock->lock, x, x+1) != x );
     preempt_disable();
@@ -409,7 +409,7 @@ unsigned long _write_lock_irqsave(rwlock
             local_irq_restore(flags);
             while ( (x = lock->lock) & RW_WRITE_FLAG )
                 cpu_relax();
-            local_irq_save(flags);
+            local_irq_disable();
         }
     } while ( cmpxchg(&lock->lock, x, x|RW_WRITE_FLAG) != x );
     while ( x != 0 )




[-- Attachment #2: spinlock-use-irqenable.patch --]
[-- Type: text/plain, Size: 1255 bytes --]

spinlock: use local_irq_disable() instead of local_irq_save() where possible

... as generally being a cheaper operation.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -162,7 +162,7 @@ unsigned long _spin_lock_irqsave(spinloc
         local_irq_restore(flags);
         while ( likely(_raw_spin_is_locked(&lock->raw)) )
             cpu_relax();
-        local_irq_save(flags);
+        local_irq_disable();
     }
     LOCK_PROFILE_GOT;
     preempt_disable();
@@ -313,7 +313,7 @@ unsigned long _read_lock_irqsave(rwlock_
             local_irq_restore(flags);
             while ( (x = lock->lock) & RW_WRITE_FLAG )
                 cpu_relax();
-            local_irq_save(flags);
+            local_irq_disable();
         }
     } while ( cmpxchg(&lock->lock, x, x+1) != x );
     preempt_disable();
@@ -409,7 +409,7 @@ unsigned long _write_lock_irqsave(rwlock
             local_irq_restore(flags);
             while ( (x = lock->lock) & RW_WRITE_FLAG )
                 cpu_relax();
-            local_irq_save(flags);
+            local_irq_disable();
         }
     } while ( cmpxchg(&lock->lock, x, x|RW_WRITE_FLAG) != x );
     while ( x != 0 )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/3] spinlock: use local_irq_disable() instead of local_irq_save() where possible

[Konrad Rzeszutek Wilk]

On Thu, Jan 08, 2015 at 04:13:03PM +0000, Jan Beulich wrote:
> ... as generally being a cheaper operation.

I was wondering if it would be possible to change some of the
EFLAGS after when we go in the 'cpu_relax' - and an interrupt
happens, we process it, alter the EFLAGS, then when we are
done, the EFLAGS are different - which the original code would
save when it was done sitting on the cpu_relax() loop.

Actually that sounds bad - we only want to restore the flags
that we had when going in this spin lock. Would make sense
to add an ASSERT to check for flags being different from the
EFLAGS?


> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -162,7 +162,7 @@ unsigned long _spin_lock_irqsave(spinloc
>          local_irq_restore(flags);
>          while ( likely(_raw_spin_is_locked(&lock->raw)) )
>              cpu_relax();
> -        local_irq_save(flags);
> +        local_irq_disable();
>      }
>      LOCK_PROFILE_GOT;
>      preempt_disable();
> @@ -313,7 +313,7 @@ unsigned long _read_lock_irqsave(rwlock_
>              local_irq_restore(flags);
>              while ( (x = lock->lock) & RW_WRITE_FLAG )
>                  cpu_relax();
> -            local_irq_save(flags);
> +            local_irq_disable();
>          }
>      } while ( cmpxchg(&lock->lock, x, x+1) != x );
>      preempt_disable();
> @@ -409,7 +409,7 @@ unsigned long _write_lock_irqsave(rwlock
>              local_irq_restore(flags);
>              while ( (x = lock->lock) & RW_WRITE_FLAG )
>                  cpu_relax();
> -            local_irq_save(flags);
> +            local_irq_disable();
>          }
>      } while ( cmpxchg(&lock->lock, x, x|RW_WRITE_FLAG) != x );
>      while ( x != 0 )
> 
> 
> 

> spinlock: use local_irq_disable() instead of local_irq_save() where possible
> 
> ... as generally being a cheaper operation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -162,7 +162,7 @@ unsigned long _spin_lock_irqsave(spinloc
>          local_irq_restore(flags);
>          while ( likely(_raw_spin_is_locked(&lock->raw)) )
>              cpu_relax();
> -        local_irq_save(flags);
> +        local_irq_disable();
>      }
>      LOCK_PROFILE_GOT;
>      preempt_disable();
> @@ -313,7 +313,7 @@ unsigned long _read_lock_irqsave(rwlock_
>              local_irq_restore(flags);
>              while ( (x = lock->lock) & RW_WRITE_FLAG )
>                  cpu_relax();
> -            local_irq_save(flags);
> +            local_irq_disable();
>          }
>      } while ( cmpxchg(&lock->lock, x, x+1) != x );
>      preempt_disable();
> @@ -409,7 +409,7 @@ unsigned long _write_lock_irqsave(rwlock
>              local_irq_restore(flags);
>              while ( (x = lock->lock) & RW_WRITE_FLAG )
>                  cpu_relax();
> -            local_irq_save(flags);
> +            local_irq_disable();
>          }
>      } while ( cmpxchg(&lock->lock, x, x|RW_WRITE_FLAG) != x );
>      while ( x != 0 )

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/3] spinlock: use local_irq_disable() instead of local_irq_save() where possible

[Jan Beulich]

>>> On 09.01.15 at 17:02, <konrad.wilk@oracle.com> wrote:
> On Thu, Jan 08, 2015 at 04:13:03PM +0000, Jan Beulich wrote:
>> ... as generally being a cheaper operation.
> 
> I was wondering if it would be possible to change some of the
> EFLAGS after when we go in the 'cpu_relax' - and an interrupt
> happens, we process it, alter the EFLAGS, then when we are
> done, the EFLAGS are different - which the original code would
> save when it was done sitting on the cpu_relax() loop.
> 
> Actually that sounds bad - we only want to restore the flags
> that we had when going in this spin lock. Would make sense
> to add an ASSERT to check for flags being different from the
> EFLAGS?

I'm not sure I'm following what you try to tell me. For one,
interrupts don't change EFLAGS - the IRET restores them. And
second, local_irq_restore() is only guaranteed to restore
EFLAGS.IF - whether it touches other flags if largely undefined.

Jan

>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -162,7 +162,7 @@ unsigned long _spin_lock_irqsave(spinloc
>>          local_irq_restore(flags);
>>          while ( likely(_raw_spin_is_locked(&lock->raw)) )
>>              cpu_relax();
>> -        local_irq_save(flags);
>> +        local_irq_disable();
>>      }
>>      LOCK_PROFILE_GOT;
>>      preempt_disable();
>> @@ -313,7 +313,7 @@ unsigned long _read_lock_irqsave(rwlock_
>>              local_irq_restore(flags);
>>              while ( (x = lock->lock) & RW_WRITE_FLAG )
>>                  cpu_relax();
>> -            local_irq_save(flags);
>> +            local_irq_disable();
>>          }
>>      } while ( cmpxchg(&lock->lock, x, x+1) != x );
>>      preempt_disable();
>> @@ -409,7 +409,7 @@ unsigned long _write_lock_irqsave(rwlock
>>              local_irq_restore(flags);
>>              while ( (x = lock->lock) & RW_WRITE_FLAG )
>>                  cpu_relax();
>> -            local_irq_save(flags);
>> +            local_irq_disable();
>>          }
>>      } while ( cmpxchg(&lock->lock, x, x|RW_WRITE_FLAG) != x );
>>      while ( x != 0 )

Re: [PATCH 1/3] spinlock: use local_irq_disable() instead of local_irq_save() where possible

[Andrew Cooper]

On 09/01/15 16:02, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 08, 2015 at 04:13:03PM +0000, Jan Beulich wrote:
>> ... as generally being a cheaper operation.
> I was wondering if it would be possible to change some of the
> EFLAGS after when we go in the 'cpu_relax' - and an interrupt
> happens, we process it, alter the EFLAGS, then when we are
> done, the EFLAGS are different - which the original code would
> save when it was done sitting on the cpu_relax() loop.
>
> Actually that sounds bad - we only want to restore the flags
> that we had when going in this spin lock. Would make sense
> to add an ASSERT to check for flags being different from the
> EFLAGS?

local_irq_restore() only restores the interrupt flag from flags.  All
other bits in EFLAGS are unmodified.

~Andrew

Re: [PATCH 1/3] spinlock: use local_irq_disable() instead of local_irq_save() where possible

[Konrad Rzeszutek Wilk]

On Fri, Jan 09, 2015 at 04:09:30PM +0000, Andrew Cooper wrote:
> On 09/01/15 16:02, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jan 08, 2015 at 04:13:03PM +0000, Jan Beulich wrote:
> >> ... as generally being a cheaper operation.
> > I was wondering if it would be possible to change some of the
> > EFLAGS after when we go in the 'cpu_relax' - and an interrupt
> > happens, we process it, alter the EFLAGS, then when we are
> > done, the EFLAGS are different - which the original code would
> > save when it was done sitting on the cpu_relax() loop.
> >
> > Actually that sounds bad - we only want to restore the flags
> > that we had when going in this spin lock. Would make sense
> > to add an ASSERT to check for flags being different from the
> > EFLAGS?
> 
> local_irq_restore() only restores the interrupt flag from flags.  All
> other bits in EFLAGS are unmodified.

which I would have found out if I read the code from local_irq_restore().

Sorry about the noise - should have looked at the code before asking
questions!
> 
> ~Andrew

[PATCH 2/3] rwlock: allow arch to override read_unlock() atomic

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

On x86, LOCK DEC is cheaper than LOCK CMPXCHG and doesn't require a
retry loop around it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -333,14 +333,18 @@ int _read_trylock(rwlock_t *lock)
     return 1;
 }
 
-void _read_unlock(rwlock_t *lock)
-{
-    uint32_t x, y;
+#ifndef _raw_read_unlock
+# define _raw_read_unlock(l) do {                      \
+    uint32_t x = (l)->lock, y;                         \
+    while ( (y = cmpxchg(&(l)->lock, x, x - 1)) != x ) \
+        x = y;                                         \
+} while (0)
+#endif
 
+inline void _read_unlock(rwlock_t *lock)
+{
     preempt_enable();
-    x = lock->lock;
-    while ( (y = cmpxchg(&lock->lock, x, x-1)) != x )
-        x = y;
+    _raw_read_unlock(lock);
 }
 
 void _read_unlock_irq(rwlock_t *lock)
--- 2014-08-25.orig/xen/include/asm-x86/spinlock.h	2014-10-07 14:42:41.000000000 +0200
+++ 2014-08-25/xen/include/asm-x86/spinlock.h	2014-10-07 15:33:18.000000000 +0200
@@ -31,4 +31,7 @@ static always_inline int _raw_spin_trylo
     return (oldval > 0);
 }
 
+#define _raw_read_unlock(l) \
+    asm volatile ( "lock; dec%z0 %0" : "+m" ((l)->lock) :: "memory" )
+
 #endif /* __ASM_SPINLOCK_H */




[-- Attachment #2: arch-read-unlock.patch --]
[-- Type: text/plain, Size: 1334 bytes --]

rwlock: allow arch to override read_unlock() atomic

On x86, LOCK DEC is cheaper than LOCK CMPXCHG and doesn't require a
retry loop around it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -333,14 +333,18 @@ int _read_trylock(rwlock_t *lock)
     return 1;
 }
 
-void _read_unlock(rwlock_t *lock)
-{
-    uint32_t x, y;
+#ifndef _raw_read_unlock
+# define _raw_read_unlock(l) do {                      \
+    uint32_t x = (l)->lock, y;                         \
+    while ( (y = cmpxchg(&(l)->lock, x, x - 1)) != x ) \
+        x = y;                                         \
+} while (0)
+#endif
 
+inline void _read_unlock(rwlock_t *lock)
+{
     preempt_enable();
-    x = lock->lock;
-    while ( (y = cmpxchg(&lock->lock, x, x-1)) != x )
-        x = y;
+    _raw_read_unlock(lock);
 }
 
 void _read_unlock_irq(rwlock_t *lock)
--- 2014-08-25.orig/xen/include/asm-x86/spinlock.h	2014-10-07 14:42:41.000000000 +0200
+++ 2014-08-25/xen/include/asm-x86/spinlock.h	2014-10-07 15:33:18.000000000 +0200
@@ -31,4 +31,7 @@ static always_inline int _raw_spin_trylo
     return (oldval > 0);
 }
 
+#define _raw_read_unlock(l) \
+    asm volatile ( "lock; dec%z0 %0" : "+m" ((l)->lock) :: "memory" )
+
 #endif /* __ASM_SPINLOCK_H */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/3] rwlock: allow arch to override write_unlock() atomic

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

... (for consistency with read_unlock()), and default it to xchg(),
being generally cheaper than cmpxchg().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -438,10 +438,14 @@ int _write_trylock(rwlock_t *lock)
     return 1;
 }
 
-void _write_unlock(rwlock_t *lock)
+#ifndef _raw_write_unlock
+# define _raw_write_unlock(l) xchg(&(l)->lock, 0)
+#endif
+
+inline void _write_unlock(rwlock_t *lock)
 {
     preempt_enable();
-    if ( cmpxchg(&lock->lock, RW_WRITE_FLAG, 0) != RW_WRITE_FLAG )
+    if ( _raw_write_unlock(lock) != RW_WRITE_FLAG )
         BUG();
 }
 




[-- Attachment #2: arch-write-unlock.patch --]
[-- Type: text/plain, Size: 705 bytes --]

rwlock: allow arch to override write_unlock() atomic

... (for consistency with read_unlock()), and default it to xchg(),
being generally cheaper than cmpxchg().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -438,10 +438,14 @@ int _write_trylock(rwlock_t *lock)
     return 1;
 }
 
-void _write_unlock(rwlock_t *lock)
+#ifndef _raw_write_unlock
+# define _raw_write_unlock(l) xchg(&(l)->lock, 0)
+#endif
+
+inline void _write_unlock(rwlock_t *lock)
 {
     preempt_enable();
-    if ( cmpxchg(&lock->lock, RW_WRITE_FLAG, 0) != RW_WRITE_FLAG )
+    if ( _raw_write_unlock(lock) != RW_WRITE_FLAG )
         BUG();
 }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/3] XSA-114 follow-ups

[Tim Deegan]

At 16:00 +0000 on 08 Jan (1420729255), Jan Beulich wrote:
> 1: spinlock: use local_irq_disable() instead of local_irq_save() where possible
> 2: rwlock: allow arch to override read_unlock() atomic
> 3: rwlock: allow arch to override write_unlock() atomic
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH 0/3] XSA-114 follow-ups

[Andrew Cooper]

On 08/01/15 16:28, Tim Deegan wrote:
> At 16:00 +0000 on 08 Jan (1420729255), Jan Beulich wrote:
>> 1: spinlock: use local_irq_disable() instead of local_irq_save() where possible
>> 2: rwlock: allow arch to override read_unlock() atomic
>> 3: rwlock: allow arch to override write_unlock() atomic
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Tim Deegan <tim@xen.org>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>