spin-is-locked is evil patchkit v2

spin-is-locked is evil patchkit v2

[Andi Kleen]

Here's an updated patchkit to mostly exorcise spin_is_locked.
See 11/13 on why.

Now comes with a checkpatch rule and better documentation, and various
feedbacks on v1 were addressed. 

Should be ready for merging now.

-Andi

[PATCH 01/13] block: use lockdep_assert_held for queue locking

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Instead of an ugly open coded variant.

Cc: axboe@kernel.dk
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 block/blk-throttle.c   |    2 +-
 include/linux/blkdev.h |   18 +++++++-----------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 5eed6a7..f2ddb94 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1218,7 +1218,7 @@ void blk_throtl_drain(struct request_queue *q)
 	struct bio_list bl;
 	struct bio *bio;
 
-	WARN_ON_ONCE(!queue_is_locked(q));
+	queue_lockdep_assert_held(q);
 
 	bio_list_init(&bl);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 606cf33..2aa2466 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -426,14 +426,10 @@ struct request_queue {
 				 (1 << QUEUE_FLAG_SAME_COMP)	|	\
 				 (1 << QUEUE_FLAG_ADD_RANDOM))
 
-static inline int queue_is_locked(struct request_queue *q)
+static inline void queue_lockdep_assert_held(struct request_queue *q)
 {
-#ifdef CONFIG_SMP
-	spinlock_t *lock = q->queue_lock;
-	return lock && spin_is_locked(lock);
-#else
-	return 1;
-#endif
+	if (q->queue_lock)
+		lockdep_assert_held(q->queue_lock);
 }
 
 static inline void queue_flag_set_unlocked(unsigned int flag,
@@ -445,7 +441,7 @@ static inline void queue_flag_set_unlocked(unsigned int flag,
 static inline int queue_flag_test_and_clear(unsigned int flag,
 					    struct request_queue *q)
 {
-	WARN_ON_ONCE(!queue_is_locked(q));
+	queue_lockdep_assert_held(q);
 
 	if (test_bit(flag, &q->queue_flags)) {
 		__clear_bit(flag, &q->queue_flags);
@@ -458,7 +454,7 @@ static inline int queue_flag_test_and_clear(unsigned int flag,
 static inline int queue_flag_test_and_set(unsigned int flag,
 					  struct request_queue *q)
 {
-	WARN_ON_ONCE(!queue_is_locked(q));
+	queue_lockdep_assert_held(q);
 
 	if (!test_bit(flag, &q->queue_flags)) {
 		__set_bit(flag, &q->queue_flags);
@@ -470,7 +466,7 @@ static inline int queue_flag_test_and_set(unsigned int flag,
 
 static inline void queue_flag_set(unsigned int flag, struct request_queue *q)
 {
-	WARN_ON_ONCE(!queue_is_locked(q));
+	queue_lockdep_assert_held(q);
 	__set_bit(flag, &q->queue_flags);
 }
 
@@ -487,7 +483,7 @@ static inline int queue_in_flight(struct request_queue *q)
 
 static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 {
-	WARN_ON_ONCE(!queue_is_locked(q));
+	queue_lockdep_assert_held(q);
 	__clear_bit(flag, &q->queue_flags);
 }
 
-- 
1.7.7.6

[PATCH 02/13] sgi-xp: Use lockdep_assert_held

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

!spin_is_locked() is always true on a up build, so cannot
be used for assert.  Replace with lockdep_assert_held.

I realize UP builds are not very likely for this driver,
but it's still better to fix it.

Acked-by: Robin Holt <holt@sgi.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/misc/sgi-xp/xpc_channel.c |    6 +++---
 drivers/misc/sgi-xp/xpc_sn2.c     |    2 +-
 drivers/misc/sgi-xp/xpc_uv.c      |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/sgi-xp/xpc_channel.c b/drivers/misc/sgi-xp/xpc_channel.c
index 652593f..1655c21 100644
--- a/drivers/misc/sgi-xp/xpc_channel.c
+++ b/drivers/misc/sgi-xp/xpc_channel.c
@@ -28,7 +28,7 @@ xpc_process_connect(struct xpc_channel *ch, unsigned long *irq_flags)
 {
 	enum xp_retval ret;
 
-	DBUG_ON(!spin_is_locked(&ch->lock));
+	lockdep_assert_held(&ch->lock);
 
 	if (!(ch->flags & XPC_C_OPENREQUEST) ||
 	    !(ch->flags & XPC_C_ROPENREQUEST)) {
@@ -82,7 +82,7 @@ xpc_process_disconnect(struct xpc_channel *ch, unsigned long *irq_flags)
 	struct xpc_partition *part = &xpc_partitions[ch->partid];
 	u32 channel_was_connected = (ch->flags & XPC_C_WASCONNECTED);
 
-	DBUG_ON(!spin_is_locked(&ch->lock));
+	lockdep_assert_held(&ch->lock);
 
 	if (!(ch->flags & XPC_C_DISCONNECTING))
 		return;
@@ -758,7 +758,7 @@ xpc_disconnect_channel(const int line, struct xpc_channel *ch,
 {
 	u32 channel_was_connected = (ch->flags & XPC_C_CONNECTED);
 
-	DBUG_ON(!spin_is_locked(&ch->lock));
+	lockdep_assert_held(&ch->lock);
 
 	if (ch->flags & (XPC_C_DISCONNECTING | XPC_C_DISCONNECTED))
 		return;
diff --git a/drivers/misc/sgi-xp/xpc_sn2.c b/drivers/misc/sgi-xp/xpc_sn2.c
index 7d71c04..9d3b113 100644
--- a/drivers/misc/sgi-xp/xpc_sn2.c
+++ b/drivers/misc/sgi-xp/xpc_sn2.c
@@ -1674,7 +1674,7 @@ xpc_teardown_msg_structures_sn2(struct xpc_channel *ch)
 {
 	struct xpc_channel_sn2 *ch_sn2 = &ch->sn.sn2;
 
-	DBUG_ON(!spin_is_locked(&ch->lock));
+	lockdep_assert_held(&ch->lock);
 
 	ch_sn2->remote_msgqueue_pa = 0;
 
diff --git a/drivers/misc/sgi-xp/xpc_uv.c b/drivers/misc/sgi-xp/xpc_uv.c
index 17bbacb..666b081 100644
--- a/drivers/misc/sgi-xp/xpc_uv.c
+++ b/drivers/misc/sgi-xp/xpc_uv.c
@@ -1181,7 +1181,7 @@ xpc_teardown_msg_structures_uv(struct xpc_channel *ch)
 {
 	struct xpc_channel_uv *ch_uv = &ch->sn.uv;
 
-	DBUG_ON(!spin_is_locked(&ch->lock));
+	lockdep_assert_held(&ch->lock);
 
 	kfree(ch_uv->cached_notify_gru_mq_desc);
 	ch_uv->cached_notify_gru_mq_desc = NULL;
-- 
1.7.7.6

[PATCH 03/13] ada152x: Remove broken usage of spin_is_locked

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Remove racy usage of spin_is_locked. The author seems to have been
unclear on the concept of locking.

This is debug code normally not enabled, but I caught it on a tree sweep.

Cc: James.Bottomley@HansenPartnership.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/scsi/aha152x.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index f17c92c..c56b617 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -288,9 +288,6 @@ static LIST_HEAD(aha152x_host_list);
 
 #define DO_LOCK(flags)	\
 	do { \
-		if(spin_is_locked(&QLOCK)) { \
-			DPRINTK(debug_intr, DEBUG_LEAD "(%s:%d) already locked at %s:%d\n", CMDINFO(CURRENT_SC), __func__, __LINE__, QLOCKER, QLOCKERL); \
-		} \
 		DPRINTK(debug_locking, DEBUG_LEAD "(%s:%d) locking\n", CMDINFO(CURRENT_SC), __func__, __LINE__); \
 		spin_lock_irqsave(&QLOCK,flags); \
 		DPRINTK(debug_locking, DEBUG_LEAD "(%s:%d) locked\n", CMDINFO(CURRENT_SC), __func__, __LINE__); \
-- 
1.7.7.6

[PATCH 04/13] staging/zmem: Use lockdep_assert_held instead of spin_is_locked

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

WARN_ON(!spin_is_locked()) will always trigger on UP.
Use lockdep_assert_held instead.

Cc: gregkh@linuxfoundation.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/staging/zcache/tmem.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/zcache/tmem.h b/drivers/staging/zcache/tmem.h
index ed147c4..0d4aa82 100644
--- a/drivers/staging/zcache/tmem.h
+++ b/drivers/staging/zcache/tmem.h
@@ -47,7 +47,7 @@
 #define ASSERT_INVERTED_SENTINEL(_x, _y) do { } while (0)
 #endif
 
-#define ASSERT_SPINLOCK(_l)	WARN_ON(!spin_is_locked(_l))
+#define ASSERT_SPINLOCK(_l)	lockdep_assert_held(_l)
 
 /*
  * A pool is the highest-level data structure managed by tmem and
-- 
1.7.7.6

[PATCH 05/13] XFS: Fix lock ASSERT on UP

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

ASSERT(!spin_is_locked()) doesn't work on UP builds. Replace with a standard
lockdep_assert_held()

Cc: xfs-masters@oss.sgi.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 fs/xfs/xfs_iget.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index 8c3e463..8654d78 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -76,7 +76,7 @@ xfs_inode_alloc(
 	}
 
 	ASSERT(atomic_read(&ip->i_pincount) == 0);
-	ASSERT(!spin_is_locked(&ip->i_flags_lock));
+	lockdep_assert_held(&ip->i_flags_lock);
 	ASSERT(!xfs_isiflocked(ip));
 	ASSERT(ip->i_ino == 0);
 
@@ -147,7 +147,7 @@ xfs_inode_free(
 
 	/* asserts to verify all state is correct here */
 	ASSERT(atomic_read(&ip->i_pincount) == 0);
-	ASSERT(!spin_is_locked(&ip->i_flags_lock));
+	lockdep_assert_held(&ip->i_flags_lock);
 	ASSERT(!xfs_isiflocked(ip));
 
 	/*
-- 
1.7.7.6

[PATCH 06/13] huge-memory: Use lockdep_assert_held

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use lockdep_assert_held to check for locks instead of an opencoded
variant.

Cc: aarcange@redhat.com
Acked-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 mm/huge_memory.c |    4 ++--
 mm/swap.c        |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 91d3efb..28669c6 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2083,7 +2083,7 @@ static void collect_mm_slot(struct mm_slot *mm_slot)
 {
 	struct mm_struct *mm = mm_slot->mm;
 
-	VM_BUG_ON(NR_CPUS != 1 && !spin_is_locked(&khugepaged_mm_lock));
+	lockdep_assert_held(&khugepaged_mm_lock);
 
 	if (khugepaged_test_exit(mm)) {
 		/* free mm_slot */
@@ -2113,7 +2113,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
 	int progress = 0;
 
 	VM_BUG_ON(!pages);
-	VM_BUG_ON(NR_CPUS != 1 && !spin_is_locked(&khugepaged_mm_lock));
+	lockdep_assert_held(&khugepaged_mm_lock);
 
 	if (khugepaged_scan.mm_slot)
 		mm_slot = khugepaged_scan.mm_slot;
diff --git a/mm/swap.c b/mm/swap.c
index fff1ff7..811e615 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -659,7 +659,7 @@ void lru_add_page_tail(struct zone* zone,
 	VM_BUG_ON(!PageHead(page));
 	VM_BUG_ON(PageCompound(page_tail));
 	VM_BUG_ON(PageLRU(page_tail));
-	VM_BUG_ON(NR_CPUS != 1 && !spin_is_locked(&zone->lru_lock));
+	lockdep_assert_held(&zone->lru_lock);
 
 	SetPageLRU(page_tail);
 
-- 
1.7.7.6

[PATCH 07/13] futex: Use lockdep_assert_held() for lock checking

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use lockdep_assert_held() for lock checking instead of a strange
homegrown variant. This removes the return for this case, but
that is unlikely to be useful anyways.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 kernel/futex.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 1614be2..a77dda7 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -823,8 +823,9 @@ static void __unqueue_futex(struct futex_q *q)
 {
 	struct futex_hash_bucket *hb;
 
-	if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
-	    || WARN_ON(plist_node_empty(&q->list)))
+	if (q->lock_ptr)
+		lockdep_assert_held(q->lock_ptr);
+	if (WARN_ON(plist_node_empty(&q->list)))
 		return;
 
 	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
-- 
1.7.7.6

[PATCH 08/13] irda: remove spin_is_locked

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

It's hard to imagine how this spin_is_locked debugging check is not
totally racy.  Remove it.

Cc: netdev@vger.kernel.org
Cc: samuel@sortiz.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/net/irda/sir_dev.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/net/irda/sir_dev.c b/drivers/net/irda/sir_dev.c
index 5039f08..90a1b3e 100644
--- a/drivers/net/irda/sir_dev.c
+++ b/drivers/net/irda/sir_dev.c
@@ -632,11 +632,6 @@ static netdev_tx_t sirdev_hard_xmit(struct sk_buff *skb,
 	/* Init tx buffer*/
 	dev->tx_buff.data = dev->tx_buff.head;
 
-	/* Check problems */
-	if(spin_is_locked(&dev->tx_lock)) {
-		IRDA_DEBUG(3, "%s(), write not completed\n", __func__);
-	}
-
 	/* serialize with write completion */
 	spin_lock_irqsave(&dev->tx_lock, flags);
 
-- 
1.7.7.6

[PATCH 09/13] usb: gadget: f_fs: Remove lock is held before freeing checks

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

lock debugging already supports this, no need to do it explicitely.

Cc: balbi@ti.com
Cc: gregkh@linuxfoundation.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/usb/gadget/f_fs.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f63dc6c..c5e8994 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1258,9 +1258,7 @@ static void ffs_data_put(struct ffs_data *ffs)
 	if (unlikely(atomic_dec_and_test(&ffs->ref))) {
 		pr_info("%s(): freeing\n", __func__);
 		ffs_data_clear(ffs);
-		BUG_ON(mutex_is_locked(&ffs->mutex) ||
-		       spin_is_locked(&ffs->ev.waitq.lock) ||
-		       waitqueue_active(&ffs->ev.waitq) ||
+		BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
 		       waitqueue_active(&ffs->ep0req_completion.wait));
 		kfree(ffs);
 	}
-- 
1.7.7.6

[PATCH 10/13] smsc911x: Use lockdep_assert_held instead of home grown buggy construct

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Cc: netdev@vger.kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/net/ethernet/smsc/smsc911x.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/smsc/smsc911x.h b/drivers/net/ethernet/smsc/smsc911x.h
index 9ad5e5d..4fbb697 100644
--- a/drivers/net/ethernet/smsc/smsc911x.h
+++ b/drivers/net/ethernet/smsc/smsc911x.h
@@ -52,7 +52,7 @@
 
 #ifdef CONFIG_DEBUG_SPINLOCK
 #define SMSC_ASSERT_MAC_LOCK(pdata) \
-		WARN_ON(!spin_is_locked(&pdata->mac_lock))
+		lockdep_assert_held(&(pdata)->mac_lock)
 #else
 #define SMSC_ASSERT_MAC_LOCK(pdata) do {} while (0)
 #endif				/* CONFIG_DEBUG_SPINLOCK */
-- 
1.7.7.6

[PATCH 11/13] Add a discussion on why spin_is_locked() is bad to spinlocks.txt

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 Documentation/spinlocks.txt |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/Documentation/spinlocks.txt b/Documentation/spinlocks.txt
index 9dbe885..1787229 100644
--- a/Documentation/spinlocks.txt
+++ b/Documentation/spinlocks.txt
@@ -146,6 +146,49 @@ indeed), while write-locks need to protect themselves against interrupts.
 
 ----
 
+spin_is_locked is a bad idea
+
+spin_is_locked checks if a lock is currently hold.  On uniprocessor kernels
+it always returns 0. In general this function should be avoided because most 
+uses of it are either redundant or broken.
+
+People often use spin_is_locked() to check if a particular lock is hold when a function
+is called to enforce a locking discipline, like
+
+	WARN_ON(!spin_is_locked(!my_lock))
+
+or 
+
+	BUG_ON(!spin_is_locked(!my_lock))
+
+or some variant of those.
+
+This does not work on uniprocessor kernels because they will always fail.
+While there are ways around that they are ugly and not recommended.
+Better use lockdep_assert_held(). This also only checks on a lock debugging
+kernel (which you should occasionally run on your code anyways because
+it catches many more problems). 
+
+In generally this would be better done with static annotation anyways 
+(there's some support for it in sparse)
+
+	BUG_ON(spin_is_locked(obj->lock));
+	kfree(obj);
+
+Another usage is checking whether a lock is not hold when freeing an object.
+However this is redundant because lock debugging supports this anyways
+without explicit code. Just delete the BUG_ON.
+
+A third usage is to check in a console function if a lock is hold, to get
+a panic crash dump out even when some other thread died in it.
+This is better implemented with spin_try_lock() et.al. and a timeout.
+
+Other usages are usually simply races.
+
+In summary just don't use it.
+
+----
+
 Reference information:
 
 For dynamic initialization, use spin_lock_init() or rwlock_init() as
-- 
1.7.7.6

[PATCH 12/13] Add a kerneldoc comment to spin_is_locked() that discourages its usage

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 include/linux/spinlock.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 7df6c17..4805e4d 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -360,6 +360,15 @@ static inline void spin_unlock_wait(spinlock_t *lock)
 	raw_spin_unlock_wait(&lock->rlock);
 }
 
+/**
+ * spin_is_locked() - Check if a spinlock is being held.
+ * @lock: Lock to check.
+ *
+ * This function should normally not be used. Especially using it in
+ * WARN and BUG_ONs is usually incorrect or redundant.
+ * If you want to check if a lock is held in a function
+ * use lockdep_assert_held(). A lot of other usages are racy.
+ */
 static inline int spin_is_locked(spinlock_t *lock)
 {
 	return raw_spin_is_locked(&lock->rlock);
-- 
1.7.7.6

[PATCH 13/13] checkpatch: Check for spin_is_locked

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

spin_is_locked is usually misued. In checkpatch.pl

- warn when it is used at all
- error out when it is asserted on free, because that's usually broken
(e.g. doesn't work on on uni processor builds). Recommend
lockdep_assert_held() instead.

v2: improvements from Joe Perches
Cc: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 scripts/checkpatch.pl |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a3b9782..002bce3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3330,6 +3330,20 @@ sub process {
 			}
 		}
 
+# spin_is_locked is usually misused. warn about it.
+		if ($line =~ /\bspin_is_locked\s*\(/) {
+			# BUG_ON/WARN_ON(!spin_is_locked() is generally a bug
+			if ($line =~ /(BUG_ON|WARN_ON|ASSERT)\s*\(!spin_is_locked/) {
+				ERROR("SPIN_IS_LOCKED",
+				     "Use lockdep_assert_held() instead of asserts on !spin_is_locked\n"
+				      . $herecurr);
+			} else {
+				WARN("SPIN_IS_LOCKED",
+			     "spin_is_locked is usually misused. See Documentation/spinlocks.txt\n"
+					. $herecurr)
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {
-- 
1.7.7.6

Re: [PATCH 08/13] irda: remove spin_is_locked

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Tue, 27 Mar 2012 17:47:12 -0700

> From: Andi Kleen <ak@linux.intel.com>
> 
> It's hard to imagine how this spin_is_locked debugging check is not
> totally racy.  Remove it.
> 
> Cc: netdev@vger.kernel.org
> Cc: samuel@sortiz.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 10/13] smsc911x: Use lockdep_assert_held instead of home grown buggy construct

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Tue, 27 Mar 2012 17:47:14 -0700

> From: Andi Kleen <ak@linux.intel.com>
> 
> Cc: netdev@vger.kernel.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 11/13] Add a discussion on why spin_is_locked() is bad to spinlocks.txt

[Wolfram Sang]

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

On Tue, Mar 27, 2012 at 05:47:15PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  Documentation/spinlocks.txt |   43 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/spinlocks.txt b/Documentation/spinlocks.txt
> index 9dbe885..1787229 100644
> --- a/Documentation/spinlocks.txt
> +++ b/Documentation/spinlocks.txt
> @@ -146,6 +146,49 @@ indeed), while write-locks need to protect themselves against interrupts.
>  
>  ----
>  
> +spin_is_locked is a bad idea
> +
> +spin_is_locked checks if a lock is currently hold.  On uniprocessor kernels
> +it always returns 0. In general this function should be avoided because most 
> +uses of it are either redundant or broken.
> +
> +People often use spin_is_locked() to check if a particular lock is hold when a function
> +is called to enforce a locking discipline, like
> +
> +	WARN_ON(!spin_is_locked(!my_lock))
> +
> +or 
> +
> +	BUG_ON(!spin_is_locked(!my_lock))

'&my_lock' instead of '!my_lock' probably.

> +
> +or some variant of those.
> +
> +This does not work on uniprocessor kernels because they will always fail.
> +While there are ways around that they are ugly and not recommended.
> +Better use lockdep_assert_held(). This also only checks on a lock debugging
> +kernel (which you should occasionally run on your code anyways because
> +it catches many more problems). 
> +
> +In generally this would be better done with static annotation anyways 
> +(there's some support for it in sparse)
> +
> +	BUG_ON(spin_is_locked(obj->lock));
> +	kfree(obj);
> +
> +Another usage is checking whether a lock is not hold when freeing an object.

I'd suggest to move this sentence above the code example. On first read,
I was confused what the code should tell me regarding annotations :)

> +However this is redundant because lock debugging supports this anyways
> +without explicit code. Just delete the BUG_ON.
> +
> +A third usage is to check in a console function if a lock is hold, to get
> +a panic crash dump out even when some other thread died in it.
> +This is better implemented with spin_try_lock() et.al. and a timeout.
> +
> +Other usages are usually simply races.
> +
> +In summary just don't use it.

At this point, I was wondering when it actually can be used? Otherwise
it probably would have been removed from the kernel or marked
deprecated, I'd think?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 12/13] Add a kerneldoc comment to spin_is_locked() that discourages its usage

[Wolfram Sang]

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


> +/**
> + * spin_is_locked() - Check if a spinlock is being held.
> + * @lock: Lock to check.
> + *
> + * This function should normally not be used. Especially using it in
> + * WARN and BUG_ONs is usually incorrect or redundant.
> + * If you want to check if a lock is held in a function
> + * use lockdep_assert_held(). A lot of other usages are racy.
> + */

Maybe add a reference to the section you previously added to spinlock.txt?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 11/13] Add a discussion on why spin_is_locked() is bad to spinlocks.txt

[Andi Kleen]

> > +
> > +or some variant of those.
> > +
> > +This does not work on uniprocessor kernels because they will always fail.
> > +While there are ways around that they are ugly and not recommended.
> > +Better use lockdep_assert_held(). This also only checks on a lock debugging
> > +kernel (which you should occasionally run on your code anyways because
> > +it catches many more problems). 
> > +
> > +In generally this would be better done with static annotation anyways 
> > +(there's some support for it in sparse)
> > +
> > +	BUG_ON(spin_is_locked(obj->lock));
> > +	kfree(obj);
> > +
> > +Another usage is checking whether a lock is not hold when freeing an object.
> 
> I'd suggest to move this sentence above the code example. On first read,
> I was confused what the code should tell me regarding annotations :)

Both fixed.

> 
> > +However this is redundant because lock debugging supports this anyways
> > +without explicit code. Just delete the BUG_ON.
> > +
> > +A third usage is to check in a console function if a lock is hold, to get
> > +a panic crash dump out even when some other thread died in it.
> > +This is better implemented with spin_try_lock() et.al. and a timeout.
> > +
> > +Other usages are usually simply races.
> > +
> > +In summary just don't use it.
> 
> At this point, I was wondering when it actually can be used? Otherwise
> it probably would have been removed from the kernel or marked
> deprecated, I'd think?

Even after the patchkit there are a few users left (mostly third category
and some print outs). Eventually these should be fixed or removed too.
Then the function could be truly deprected.

lockdep still needs it I believe, but it should probably use some 
private interface. 

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 05/13] XFS: Fix lock ASSERT on UP

[Christoph Hellwig]

On Tue, Mar 27, 2012 at 05:47:09PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> ASSERT(!spin_is_locked()) doesn't work on UP builds. Replace with a standard
> lockdep_assert_held()

The "standard" is assert_spin_locked() - which not only is much cheaper
but also has the advantage of working in non-lockdep builds.

Re: [PATCH 05/13] XFS: Fix lock ASSERT on UP

[Andi Kleen]

On Thu, Mar 29, 2012 at 07:21:14PM -0400, Christoph Hellwig wrote:
> On Tue, Mar 27, 2012 at 05:47:09PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > ASSERT(!spin_is_locked()) doesn't work on UP builds. Replace with a standard
> > lockdep_assert_held()
> 
> The "standard" is assert_spin_locked() - which not only is much cheaper
> but also has the advantage of working in non-lockdep builds.

But then you have it unconditional, not just on debug builds.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 05/13] XFS: Fix lock ASSERT on UP

[Dave Chinner]

On Fri, Mar 30, 2012 at 01:52:01AM +0200, Andi Kleen wrote:
> On Thu, Mar 29, 2012 at 07:21:14PM -0400, Christoph Hellwig wrote:
> > On Tue, Mar 27, 2012 at 05:47:09PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > ASSERT(!spin_is_locked()) doesn't work on UP builds. Replace with a standard
> > > lockdep_assert_held()
> > 
> > The "standard" is assert_spin_locked() - which not only is much cheaper
> > but also has the advantage of working in non-lockdep builds.
> 
> But then you have it unconditional, not just on debug builds.

And the problem with that is what? There is so little overhead to the
check it doesn't matter that it is enabled in production kernels...

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/13] block: use lockdep_assert_held for queue locking

[Jens Axboe]

On 03/28/2012 02:47 AM, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Instead of an ugly open coded variant.

Thanks Andi, applied. This is definitely an improvement.

-- 
Jens Axboe

Re: [PATCH 05/13] XFS: Fix lock ASSERT on UP

[Andi Kleen]

On Fri, Mar 30, 2012 at 03:13:48PM +1100, Dave Chinner wrote:
> On Fri, Mar 30, 2012 at 01:52:01AM +0200, Andi Kleen wrote:
> > On Thu, Mar 29, 2012 at 07:21:14PM -0400, Christoph Hellwig wrote:
> > > On Tue, Mar 27, 2012 at 05:47:09PM -0700, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > ASSERT(!spin_is_locked()) doesn't work on UP builds. Replace with a standard
> > > > lockdep_assert_held()
> > > 
> > > The "standard" is assert_spin_locked() - which not only is much cheaper
> > > but also has the advantage of working in non-lockdep builds.
> > 
> > But then you have it unconditional, not just on debug builds.
> 
> And the problem with that is what? There is so little overhead to the
> check it doesn't matter that it is enabled in production kernels...

It's really interesting how much you guys argue for your buggy construct
which you clearly never tested on a UP build...

Not sure if that is a hot path, but on highly contended locks every cache line
fetch is quite expensive on larger systems.

also I doubt the thing really catches bugs, and if it did you would be 
probably better off with a sparse notation or so.

Anyways I will turn it into the normal assert.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 05/13] XFS: Fix lock ASSERT on UP

[Christoph Hellwig]

On Fri, Mar 30, 2012 at 04:04:57PM +0200, Andi Kleen wrote:
> It's really interesting how much you guys argue for your buggy construct
> which you clearly never tested on a UP build...


spin_is_locked always return 0 on UP builds, and given that XFS only
has !spin_is_locked asserts things will work just fine on UP builds
(not the !CONFIG_SMP and !CONFIG_XFS_DEBUG would be a common
combination).

In fact if you check the archives the one use of assert_spin_locked was
added exactly because some still tripped over the spin_is_locked
behaviour on UP relatively soon.

> Not sure if that is a hot path, but on highly contended locks every cache line
> fetch is quite expensive on larger systems.

It's not an overly contended lock.

> also I doubt the thing really catches bugs, and if it did you would be 
> probably better off with a sparse notation or so.

This one probably doesn't - the first occurance is just after a blocking
allocation, and the second one just before taking the lock.  So they
probably could as well just be removed.

The point that still stands is that lockdep_assert_held isn't actually
a useful replacement.

Re: [PATCH 09/13] usb: gadget: f_fs: Remove lock is held before freeing checks

[Felipe Balbi]

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

On Tue, Mar 27, 2012 at 05:47:13PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> lock debugging already supports this, no need to do it explicitely.
> 
> Cc: balbi@ti.com
> Cc: gregkh@linuxfoundation.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

since this is part of a separate series, I think it's better you carry
this with the series:

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 05/13] XFS: Fix lock ASSERT on UP

[Andrew Morton]

On Fri, 30 Mar 2012 15:13:48 +1100
Dave Chinner <david@fromorbit.com> wrote:

> On Fri, Mar 30, 2012 at 01:52:01AM +0200, Andi Kleen wrote:
> > On Thu, Mar 29, 2012 at 07:21:14PM -0400, Christoph Hellwig wrote:
> > > On Tue, Mar 27, 2012 at 05:47:09PM -0700, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > ASSERT(!spin_is_locked()) doesn't work on UP builds. Replace with a standard
> > > > lockdep_assert_held()
> > > 
> > > The "standard" is assert_spin_locked() - which not only is much cheaper
> > > but also has the advantage of working in non-lockdep builds.
> > 
> > But then you have it unconditional, not just on debug builds.
> 
> And the problem with that is what? There is so little overhead to the
> check it doesn't matter that it is enabled in production kernels...
> 

(old thread)

Perhaps assert_spin_locked() would be better - any advantages of
lockdep_assert_held() don't seem to outweigh the cost of using a new
interface.

And I'm not sure that I buy the performance argument - if an assertion
is in such a hot path, just remove the dang thing.


OTOH, one argument in favour of using lockdep_assert_held() is that
(afaict) it applies to spinlocks and to mutexes and to rw_semaphores. 
Not sure about rwlocks?

Now, having an API which can apply to different types is a bit
unpleasant - this ain't C++.  I think we should overlay
lockdep_assert_held() with a properly-typed API written in C and slap
the lockdep guys.  But the fact that this mechanism applies to all(?)
lock types is attractive.

Anyway, these patches are still floating around in my tree so please
let's finish this off one way or another.