WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-changelog

[Xen-changelog] [xen-unstable] Remove spin_barrier_irq(). It is pointles

To: xen-changelog@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-changelog] [xen-unstable] Remove spin_barrier_irq(). It is pointless.
From: Xen patchbot-unstable <patchbot@xxxxxxx>
Date: Wed, 30 Mar 2011 21:50:13 +0100
Delivery-date: Wed, 30 Mar 2011 13:51:50 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-changelog-request@lists.xensource.com?subject=help>
List-id: BK change log <xen-changelog.lists.xensource.com>
List-post: <mailto:xen-changelog@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=unsubscribe>
Reply-to: xen-devel@xxxxxxxxxxxxxxxxxxx
Sender: xen-changelog-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User Keir Fraser <keir@xxxxxxx>
# Date 1301128061 0
# Node ID ef8dd40422c8e238ac4d4745c1bb2ecfb55ed3ca
# Parent  612171ff82ea51aaf65d98fd1a551eb8d50fb481
Remove spin_barrier_irq(). It is pointless.

Add a barrier-appropriate consistency check to spinlock.c, and add
code comments to explain why barrier operations are more relaxed than
lock-acquisition operations.

Signed-off-by: Keir Fraser <keir@xxxxxxx>
---


diff -r 612171ff82ea -r ef8dd40422c8 xen/common/event_channel.c
--- a/xen/common/event_channel.c        Sat Mar 26 08:03:21 2011 +0000
+++ b/xen/common/event_channel.c        Sat Mar 26 08:27:41 2011 +0000
@@ -417,7 +417,7 @@
             if ( v->virq_to_evtchn[chn1->u.virq] != port1 )
                 continue;
             v->virq_to_evtchn[chn1->u.virq] = 0;
-            spin_barrier_irq(&v->virq_lock);
+            spin_barrier(&v->virq_lock);
         }
         break;
 
diff -r 612171ff82ea -r ef8dd40422c8 xen/common/spinlock.c
--- a/xen/common/spinlock.c     Sat Mar 26 08:03:21 2011 +0000
+++ b/xen/common/spinlock.c     Sat Mar 26 08:27:41 2011 +0000
@@ -23,6 +23,24 @@
     /* A few places take liberties with this. */
     /* BUG_ON(in_irq() && !irq_safe); */
 
+    /*
+     * We partition locks into IRQ-safe (always held with IRQs disabled) and
+     * IRQ-unsafe (always held with IRQs enabled) types. The convention for
+     * every lock must be consistently observed else we can deadlock in
+     * IRQ-context rendezvous functions (a rendezvous which gets every CPU
+     * into IRQ context before any CPU is released from the rendezvous).
+     * 
+     * If we can mix IRQ-disabled and IRQ-enabled callers, the following can
+     * happen:
+     *  * Lock is held by CPU A, with IRQs enabled
+     *  * CPU B is spinning on same lock, with IRQs disabled
+     *  * Rendezvous starts -- CPU A takes interrupt and enters rendezbous spin
+     *  * DEADLOCK -- CPU B will never enter rendezvous, CPU A will never exit
+     *                the rendezvous, and will hence never release the lock.
+     * 
+     * To guard against this subtle bug we latch the IRQ safety of every
+     * spinlock in the system, on first use.
+     */
     if ( unlikely(debug->irq_safe != irq_safe) )
     {
         int seen = cmpxchg(&debug->irq_safe, -1, irq_safe);
@@ -30,6 +48,25 @@
     }
 }
 
+static void check_barrier(struct lock_debug *debug)
+{
+    if ( unlikely(atomic_read(&spin_debug) <= 0) )
+        return;
+
+    /*
+     * For a barrier, we have a relaxed IRQ-safety-consistency check.
+     * 
+     * It is always safe to spin at the barrier with IRQs enabled -- that does
+     * not prevent us from entering an IRQ-context rendezvous, and nor are
+     * we preventing anyone else from doing so (since we do not actually
+     * acquire the lock during a barrier operation).
+     * 
+     * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
+     * is clearly wrong, for the same reason outlined in check_lock() above.
+     */
+    BUG_ON(!local_irq_is_enabled() && (debug->irq_safe == 0));
+}
+
 void spin_debug_enable(void)
 {
     atomic_inc(&spin_debug);
@@ -171,7 +208,7 @@
     s_time_t block = NOW();
     u64      loop = 0;
 
-    check_lock(&lock->debug);
+    check_barrier(&lock->debug);
     do { mb(); loop++;} while ( _raw_spin_is_locked(&lock->raw) );
     if (loop > 1)
     {
@@ -179,20 +216,12 @@
         lock->profile.block_cnt++;
     }
 #else
-    check_lock(&lock->debug);
+    check_barrier(&lock->debug);
     do { mb(); } while ( _raw_spin_is_locked(&lock->raw) );
 #endif
     mb();
 }
 
-void _spin_barrier_irq(spinlock_t *lock)
-{
-    unsigned long flags;
-    local_irq_save(flags);
-    _spin_barrier(lock);
-    local_irq_restore(flags);
-}
-
 int _spin_trylock_recursive(spinlock_t *lock)
 {
     int cpu = smp_processor_id();
diff -r 612171ff82ea -r ef8dd40422c8 xen/include/xen/spinlock.h
--- a/xen/include/xen/spinlock.h        Sat Mar 26 08:03:21 2011 +0000
+++ b/xen/include/xen/spinlock.h        Sat Mar 26 08:27:41 2011 +0000
@@ -144,7 +144,6 @@
 int _spin_is_locked(spinlock_t *lock);
 int _spin_trylock(spinlock_t *lock);
 void _spin_barrier(spinlock_t *lock);
-void _spin_barrier_irq(spinlock_t *lock);
 
 int _spin_trylock_recursive(spinlock_t *lock);
 void _spin_lock_recursive(spinlock_t *lock);
@@ -191,7 +190,6 @@
 
 /* Ensure a lock is quiescent between two critical operations. */
 #define spin_barrier(l)               _spin_barrier(l)
-#define spin_barrier_irq(l)           _spin_barrier_irq(l)
 
 /*
  * spin_[un]lock_recursive(): Use these forms when the lock can (safely!) be

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-changelog] [xen-unstable] Remove spin_barrier_irq(). It is pointless., Xen patchbot-unstable <=