Re: [PATCH RFC V9 5/19] xen/pvticketlock: Xen implementation for PV ticket locks

2013-06-04 Thread Raghavendra K T 

On 06/03/2013 09:33 PM, Konrad Rzeszutek Wilk wrote:

On Sat, Jun 01, 2013 at 12:23:14PM -0700, Raghavendra K T wrote:

xen/pvticketlock: Xen implementation for PV ticket locks

From: Jeremy Fitzhardinge jeremy.fitzhardi...@citrix.com

Replace the old Xen implementation of PV spinlocks with and implementation
of xen_lock_spinning and xen_unlock_kick.

xen_lock_spinning simply registers the cpu in its entry in lock_waiting,
adds itself to the waiting_cpus set, and blocks on an event channel
until the channel becomes pending.

xen_unlock_kick searches the cpus in waiting_cpus looking for the one
which next wants this lock with the next ticket, if any.  If found,
it kicks it by making its event channel pending, which wakes it up.

We need to make sure interrupts are disabled while we're relying on the
contents of the per-cpu lock_waiting values, otherwise an interrupt
handler could come in, try to take some other lock, block, and overwrite
our values.

Raghu: use function + enum instead of macro, cmpxchg for zero status reset

Signed-off-by: Jeremy Fitzhardinge jeremy.fitzhardi...@citrix.com
Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
Signed-off-by: Raghavendra K T raghavendra...@linux.vnet.ibm.com
---
  arch/x86/xen/spinlock.c |  347 +++
  1 file changed, 78 insertions(+), 269 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index d6481a9..860e190 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -16,45 +16,44 @@
  #include xen-ops.h
  #include debugfs.h

-#ifdef CONFIG_XEN_DEBUG_FS
-static struct xen_spinlock_stats
-{
-   u64 taken;
-   u32 taken_slow;
-   u32 taken_slow_nested;
-   u32 taken_slow_pickup;
-   u32 taken_slow_spurious;
-   u32 taken_slow_irqenable;
+enum xen_contention_stat {
+   TAKEN_SLOW,
+   TAKEN_SLOW_PICKUP,
+   TAKEN_SLOW_SPURIOUS,
+   RELEASED_SLOW,
+   RELEASED_SLOW_KICKED,
+   NR_CONTENTION_STATS
+};

-   u64 released;
-   u32 released_slow;
-   u32 released_slow_kicked;

+#ifdef CONFIG_XEN_DEBUG_FS
  #define HISTO_BUCKETS 30
-   u32 histo_spin_total[HISTO_BUCKETS+1];
-   u32 histo_spin_spinning[HISTO_BUCKETS+1];
+static struct xen_spinlock_stats
+{
+   u32 contention_stats[NR_CONTENTION_STATS];
u32 histo_spin_blocked[HISTO_BUCKETS+1];
-
-   u64 time_total;
-   u64 time_spinning;
u64 time_blocked;
  } spinlock_stats;

  static u8 zero_stats;

-static unsigned lock_timeout = 1  10;
-#define TIMEOUT lock_timeout
-
  static inline void check_zero(void)
  {
-   if (unlikely(zero_stats)) {
-   memset(spinlock_stats, 0, sizeof(spinlock_stats));
-   zero_stats = 0;
+   u8 ret;
+   u8 old = ACCESS_ONCE(zero_stats);
+   if (unlikely(old)) {
+   ret = cmpxchg(zero_stats, old, 0);
+   /* This ensures only one fellow resets the stat */
+   if (ret == old)
+   memset(spinlock_stats, 0, sizeof(spinlock_stats));
}
  }

-#define ADD_STATS(elem, val)   \
-   do { check_zero(); spinlock_stats.elem += (val); } while(0)
+static inline void add_stats(enum xen_contention_stat var, u32 val)
+{
+   check_zero();
+   spinlock_stats.contention_stats[var] += val;
+}

  static inline u64 spin_time_start(void)
  {
@@ -73,22 +72,6 @@ static void __spin_time_accum(u64 delta, u32 *array)
array[HISTO_BUCKETS]++;
  }

-static inline void spin_time_accum_spinning(u64 start)
-{
-   u32 delta = xen_clocksource_read() - start;
-
-   __spin_time_accum(delta, spinlock_stats.histo_spin_spinning);
-   spinlock_stats.time_spinning += delta;
-}
-
-static inline void spin_time_accum_total(u64 start)
-{
-   u32 delta = xen_clocksource_read() - start;
-
-   __spin_time_accum(delta, spinlock_stats.histo_spin_total);
-   spinlock_stats.time_total += delta;
-}
-
  static inline void spin_time_accum_blocked(u64 start)
  {
u32 delta = xen_clocksource_read() - start;
@@ -98,19 +81,15 @@ static inline void spin_time_accum_blocked(u64 start)
  }
  #else  /* !CONFIG_XEN_DEBUG_FS */
  #define TIMEOUT   (1  10)
-#define ADD_STATS(elem, val)   do { (void)(val); } while(0)
+static inline void add_stats(enum xen_contention_stat var, u32 val)
+{
+}

  static inline u64 spin_time_start(void)
  {
return 0;
  }

-static inline void spin_time_accum_total(u64 start)
-{
-}
-static inline void spin_time_accum_spinning(u64 start)
-{
-}
  static inline void spin_time_accum_blocked(u64 start)
  {
  }
@@ -133,229 +112,82 @@ typedef u16 xen_spinners_t;
asm(LOCK_PREFIX  decw %0 : +m ((xl)-spinners) : : memory);
  #endif

-struct xen_spinlock {
-   unsigned char lock; /* 0 - free; 1 - locked */
-   xen_spinners_t spinners;/* count of waiting cpus */
+struct xen_lock_waiting {
+   struct arch_spinlock *lock;
+

Re: [PATCH RFC V9 5/19] xen/pvticketlock: Xen implementation for PV ticket locks

2013-06-03 Thread Konrad Rzeszutek Wilk 
On Sat, Jun 01, 2013 at 12:23:14PM -0700, Raghavendra K T wrote:
 xen/pvticketlock: Xen implementation for PV ticket locks
 
 From: Jeremy Fitzhardinge jeremy.fitzhardi...@citrix.com
 
 Replace the old Xen implementation of PV spinlocks with and implementation
 of xen_lock_spinning and xen_unlock_kick.
 
 xen_lock_spinning simply registers the cpu in its entry in lock_waiting,
 adds itself to the waiting_cpus set, and blocks on an event channel
 until the channel becomes pending.
 
 xen_unlock_kick searches the cpus in waiting_cpus looking for the one
 which next wants this lock with the next ticket, if any.  If found,
 it kicks it by making its event channel pending, which wakes it up.
 
 We need to make sure interrupts are disabled while we're relying on the
 contents of the per-cpu lock_waiting values, otherwise an interrupt
 handler could come in, try to take some other lock, block, and overwrite
 our values.
 
 Raghu: use function + enum instead of macro, cmpxchg for zero status reset
 
 Signed-off-by: Jeremy Fitzhardinge jeremy.fitzhardi...@citrix.com
 Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
 Signed-off-by: Raghavendra K T raghavendra...@linux.vnet.ibm.com
 ---
  arch/x86/xen/spinlock.c |  347 
 +++
  1 file changed, 78 insertions(+), 269 deletions(-)
 
 diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
 index d6481a9..860e190 100644
 --- a/arch/x86/xen/spinlock.c
 +++ b/arch/x86/xen/spinlock.c
 @@ -16,45 +16,44 @@
  #include xen-ops.h
  #include debugfs.h
  
 -#ifdef CONFIG_XEN_DEBUG_FS
 -static struct xen_spinlock_stats
 -{
 - u64 taken;
 - u32 taken_slow;
 - u32 taken_slow_nested;
 - u32 taken_slow_pickup;
 - u32 taken_slow_spurious;
 - u32 taken_slow_irqenable;
 +enum xen_contention_stat {
 + TAKEN_SLOW,
 + TAKEN_SLOW_PICKUP,
 + TAKEN_SLOW_SPURIOUS,
 + RELEASED_SLOW,
 + RELEASED_SLOW_KICKED,
 + NR_CONTENTION_STATS
 +};
  
 - u64 released;
 - u32 released_slow;
 - u32 released_slow_kicked;
  
 +#ifdef CONFIG_XEN_DEBUG_FS
  #define HISTO_BUCKETS30
 - u32 histo_spin_total[HISTO_BUCKETS+1];
 - u32 histo_spin_spinning[HISTO_BUCKETS+1];
 +static struct xen_spinlock_stats
 +{
 + u32 contention_stats[NR_CONTENTION_STATS];
   u32 histo_spin_blocked[HISTO_BUCKETS+1];
 -
 - u64 time_total;
 - u64 time_spinning;
   u64 time_blocked;
  } spinlock_stats;
  
  static u8 zero_stats;
  
 -static unsigned lock_timeout = 1  10;
 -#define TIMEOUT lock_timeout
 -
  static inline void check_zero(void)
  {
 - if (unlikely(zero_stats)) {
 - memset(spinlock_stats, 0, sizeof(spinlock_stats));
 - zero_stats = 0;
 + u8 ret;
 + u8 old = ACCESS_ONCE(zero_stats);
 + if (unlikely(old)) {
 + ret = cmpxchg(zero_stats, old, 0);
 + /* This ensures only one fellow resets the stat */
 + if (ret == old)
 + memset(spinlock_stats, 0, sizeof(spinlock_stats));
   }
  }
  
 -#define ADD_STATS(elem, val) \
 - do { check_zero(); spinlock_stats.elem += (val); } while(0)
 +static inline void add_stats(enum xen_contention_stat var, u32 val)
 +{
 + check_zero();
 + spinlock_stats.contention_stats[var] += val;
 +}
  
  static inline u64 spin_time_start(void)
  {
 @@ -73,22 +72,6 @@ static void __spin_time_accum(u64 delta, u32 *array)
   array[HISTO_BUCKETS]++;
  }
  
 -static inline void spin_time_accum_spinning(u64 start)
 -{
 - u32 delta = xen_clocksource_read() - start;
 -
 - __spin_time_accum(delta, spinlock_stats.histo_spin_spinning);
 - spinlock_stats.time_spinning += delta;
 -}
 -
 -static inline void spin_time_accum_total(u64 start)
 -{
 - u32 delta = xen_clocksource_read() - start;
 -
 - __spin_time_accum(delta, spinlock_stats.histo_spin_total);
 - spinlock_stats.time_total += delta;
 -}
 -
  static inline void spin_time_accum_blocked(u64 start)
  {
   u32 delta = xen_clocksource_read() - start;
 @@ -98,19 +81,15 @@ static inline void spin_time_accum_blocked(u64 start)
  }
  #else  /* !CONFIG_XEN_DEBUG_FS */
  #define TIMEOUT  (1  10)
 -#define ADD_STATS(elem, val) do { (void)(val); } while(0)
 +static inline void add_stats(enum xen_contention_stat var, u32 val)
 +{
 +}
  
  static inline u64 spin_time_start(void)
  {
   return 0;
  }
  
 -static inline void spin_time_accum_total(u64 start)
 -{
 -}
 -static inline void spin_time_accum_spinning(u64 start)
 -{
 -}
  static inline void spin_time_accum_blocked(u64 start)
  {
  }
 @@ -133,229 +112,82 @@ typedef u16 xen_spinners_t;
   asm(LOCK_PREFIX  decw %0 : +m ((xl)-spinners) : : memory);
  #endif
  
 -struct xen_spinlock {
 - unsigned char lock; /* 0 - free; 1 - locked */
 - xen_spinners_t spinners;/* count of waiting cpus */
 +struct xen_lock_waiting {
 + struct arch_spinlock *lock;
 +

[PATCH RFC V9 5/19] xen/pvticketlock: Xen implementation for PV ticket locks

2013-06-01 Thread Raghavendra K T 
xen/pvticketlock: Xen implementation for PV ticket locks

From: Jeremy Fitzhardinge jeremy.fitzhardi...@citrix.com

Replace the old Xen implementation of PV spinlocks with and implementation
of xen_lock_spinning and xen_unlock_kick.

xen_lock_spinning simply registers the cpu in its entry in lock_waiting,
adds itself to the waiting_cpus set, and blocks on an event channel
until the channel becomes pending.

xen_unlock_kick searches the cpus in waiting_cpus looking for the one
which next wants this lock with the next ticket, if any.  If found,
it kicks it by making its event channel pending, which wakes it up.

We need to make sure interrupts are disabled while we're relying on the
contents of the per-cpu lock_waiting values, otherwise an interrupt
handler could come in, try to take some other lock, block, and overwrite
our values.

Raghu: use function + enum instead of macro, cmpxchg for zero status reset

Signed-off-by: Jeremy Fitzhardinge jeremy.fitzhardi...@citrix.com
Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
Signed-off-by: Raghavendra K T raghavendra...@linux.vnet.ibm.com
---
 arch/x86/xen/spinlock.c |  347 +++
 1 file changed, 78 insertions(+), 269 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index d6481a9..860e190 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -16,45 +16,44 @@
 #include xen-ops.h
 #include debugfs.h
 
-#ifdef CONFIG_XEN_DEBUG_FS
-static struct xen_spinlock_stats
-{
-   u64 taken;
-   u32 taken_slow;
-   u32 taken_slow_nested;
-   u32 taken_slow_pickup;
-   u32 taken_slow_spurious;
-   u32 taken_slow_irqenable;
+enum xen_contention_stat {
+   TAKEN_SLOW,
+   TAKEN_SLOW_PICKUP,
+   TAKEN_SLOW_SPURIOUS,
+   RELEASED_SLOW,
+   RELEASED_SLOW_KICKED,
+   NR_CONTENTION_STATS
+};
 
-   u64 released;
-   u32 released_slow;
-   u32 released_slow_kicked;
 
+#ifdef CONFIG_XEN_DEBUG_FS
 #define HISTO_BUCKETS  30
-   u32 histo_spin_total[HISTO_BUCKETS+1];
-   u32 histo_spin_spinning[HISTO_BUCKETS+1];
+static struct xen_spinlock_stats
+{
+   u32 contention_stats[NR_CONTENTION_STATS];
u32 histo_spin_blocked[HISTO_BUCKETS+1];
-
-   u64 time_total;
-   u64 time_spinning;
u64 time_blocked;
 } spinlock_stats;
 
 static u8 zero_stats;
 
-static unsigned lock_timeout = 1  10;
-#define TIMEOUT lock_timeout
-
 static inline void check_zero(void)
 {
-   if (unlikely(zero_stats)) {
-   memset(spinlock_stats, 0, sizeof(spinlock_stats));
-   zero_stats = 0;
+   u8 ret;
+   u8 old = ACCESS_ONCE(zero_stats);
+   if (unlikely(old)) {
+   ret = cmpxchg(zero_stats, old, 0);
+   /* This ensures only one fellow resets the stat */
+   if (ret == old)
+   memset(spinlock_stats, 0, sizeof(spinlock_stats));
}
 }
 
-#define ADD_STATS(elem, val)   \
-   do { check_zero(); spinlock_stats.elem += (val); } while(0)
+static inline void add_stats(enum xen_contention_stat var, u32 val)
+{
+   check_zero();
+   spinlock_stats.contention_stats[var] += val;
+}
 
 static inline u64 spin_time_start(void)
 {
@@ -73,22 +72,6 @@ static void __spin_time_accum(u64 delta, u32 *array)
array[HISTO_BUCKETS]++;
 }
 
-static inline void spin_time_accum_spinning(u64 start)
-{
-   u32 delta = xen_clocksource_read() - start;
-
-   __spin_time_accum(delta, spinlock_stats.histo_spin_spinning);
-   spinlock_stats.time_spinning += delta;
-}
-
-static inline void spin_time_accum_total(u64 start)
-{
-   u32 delta = xen_clocksource_read() - start;
-
-   __spin_time_accum(delta, spinlock_stats.histo_spin_total);
-   spinlock_stats.time_total += delta;
-}
-
 static inline void spin_time_accum_blocked(u64 start)
 {
u32 delta = xen_clocksource_read() - start;
@@ -98,19 +81,15 @@ static inline void spin_time_accum_blocked(u64 start)
 }
 #else  /* !CONFIG_XEN_DEBUG_FS */
 #define TIMEOUT(1  10)
-#define ADD_STATS(elem, val)   do { (void)(val); } while(0)
+static inline void add_stats(enum xen_contention_stat var, u32 val)
+{
+}
 
 static inline u64 spin_time_start(void)
 {
return 0;
 }
 
-static inline void spin_time_accum_total(u64 start)
-{
-}
-static inline void spin_time_accum_spinning(u64 start)
-{
-}
 static inline void spin_time_accum_blocked(u64 start)
 {
 }
@@ -133,229 +112,82 @@ typedef u16 xen_spinners_t;
asm(LOCK_PREFIX  decw %0 : +m ((xl)-spinners) : : memory);
 #endif
 
-struct xen_spinlock {
-   unsigned char lock; /* 0 - free; 1 - locked */
-   xen_spinners_t spinners;/* count of waiting cpus */
+struct xen_lock_waiting {
+   struct arch_spinlock *lock;
+   __ticket_t want;
 };
 
 static DEFINE_PER_CPU(int, lock_kicker_irq) = -1;
+static DEFINE_PER_CPU(struct xen_lock_waiting,