Tree sweep for spin_is_locked misuses and start warning about it

Tree sweep for spin_is_locked misuses and start warning about it

[Andi Kleen]

I did a tree sweep for spin_is_locked() users and many of them were
wrong, got UP incorrect, were obviously racy and had other problems.
I replaced a lot of users with calls to the correct lock debugging functions,
like lockdep_assert_held()

Generally I think spin_is_locked() should be deprecated because it 
is rarely used correctly. There are a few legitimate users so we cannot
outright remove it unfortunately.

But most users can be removed or changed.

After this start warning in checkpatch.pl for spin_is_locked() because
it's rarely wrong. Also error out there for the broken
(WARN|BUG)_ON(!spin_is_locked()) which does not work on UP builds.

-Andi

[PATCH 01/11] 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/11] 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.

Cc: 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/11] 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/11] 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/11] 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/11] 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
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/11] 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/11] 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/11] 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/11] 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/11] 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.

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..9904547 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("ASSERT_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 very rarely correctly used. Please reconsider\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 02/11] sgi-xp: Use lockdep_assert_held

[Robin Holt]

On Fri, Mar 16, 2012 at 12:00:55PM -0700, Andi Kleen wrote:
> 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>

Re: [PATCH 11/11] checkpatch: Check for spin_is_locked

[Joe Perches]

On Fri, 2012-03-16 at 12:01 -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> spin_is_locked is usually misused. In checkpatch.pl
> - warn when it is used at all
> - error out when it is asserted on free, because that's usually broken
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> +# 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("ASSERT_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 very rarely correctly used. Please reconsider\n"
> +					. $herecurr)
> +			}
> +		}
> +

I suggest you use a single --ignore string of
"SPIN_IS_LOCKED" instead of different ones.

Grammar might be improved in the WARN.  Maybe:
"Using spin_is_locked() is generally wrong. See [foo documentation]\n"

Also, like what was done for yield(), perhaps
some kernel-doc content would be useful.

See -next commit 8e3fabfde445a872c8aec2296846badf24d7c8b4

Re: [PATCH 06/11] huge-memory: Use lockdep_assert_held

[Andrea Arcangeli]

Hi Andi,

On Fri, Mar 16, 2012 at 12:00:59PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Use lockdep_assert_held to check for locks instead of an opencoded
> variant.
> 
> Cc: aarcange@redhat.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);

This check was intended to be a VM debug check, so getting enabled by
DEBUG_VM, not through lockdep. I mean I always have DEBUG_VM enabled
in all my kernels, but lockdep only enabled on my test system. So it's
not an opencoded variant strictly speaking.

My estimate is that what gets more tested is BUG_ON, second VM_BUG_ON,
third LOCKDEP. But hey, this code has been tested for a while so I'm
neutral and if you prefer the lockdep version it's up to you.

Thanks,
Andrea

Re: [PATCH 11/11] checkpatch: Check for spin_is_locked

[Andi Kleen]

On Fri, Mar 16, 2012 at 12:22:35PM -0700, Joe Perches wrote:
> On Fri, 2012-03-16 at 12:01 -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > spin_is_locked is usually misused. In checkpatch.pl
> > - warn when it is used at all
> > - error out when it is asserted on free, because that's usually broken
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > +# 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("ASSERT_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 very rarely correctly used. Please reconsider\n"
> > +					. $herecurr)
> > +			}
> > +		}
> > +
> 
> I suggest you use a single --ignore string of
> "SPIN_IS_LOCKED" instead of different ones.

Done.
> 
> Grammar might be improved in the WARN.  Maybe:
> "Using spin_is_locked() is generally wrong. See [foo documentation]\n"

Done.

> 
> Also, like what was done for yield(), perhaps
> some kernel-doc content would be useful.

Added to spinlocks.txt and spinlock.h


+/**
+ * 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.
+ */


+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.






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

Re: [PATCH 06/11] huge-memory: Use lockdep_assert_held

[Hugh Dickins]

On Fri, 16 Mar 2012, Andrea Arcangeli wrote:
> On Fri, Mar 16, 2012 at 12:00:59PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Use lockdep_assert_held to check for locks instead of an opencoded
> > variant.
> > 
> > Cc: aarcange@redhat.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);
> 
> This check was intended to be a VM debug check, so getting enabled by
> DEBUG_VM, not through lockdep. I mean I always have DEBUG_VM enabled
> in all my kernels, but lockdep only enabled on my test system. So it's
> not an opencoded variant strictly speaking.
> 
> My estimate is that what gets more tested is BUG_ON, second VM_BUG_ON,
> third LOCKDEP.

My estimate too.

> But hey, this code has been tested for a while so I'm
> neutral and if you prefer the lockdep version it's up to you.

Yes, it's not as if these are public interfaces used by drivers and
filesystems: they have very few callsites, and by now those lines
are mostly useful as documentation.

Andrew did mention assert_spin_locked() when I added those NR_CPUS
conditions, but we preferred VM_BUG_ON to BUG_ON, and I'm content
with Andi's further restriction to lockdep.

Acked-by: Hugh Dickins <hughd@google.com>

Hugh

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

[Dave Chinner]

On Fri, Mar 16, 2012 at 12:00:58PM -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()
> 
> 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));

So this means we only ever check that the spinlock is held when
lockdep is turned on instead of whenever CONFIG_XFS_DEBUG is set?
That means it will rarely get checked during development instead of
all the time. That's not an improvement IMO....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Andi Kleen]

> So this means we only ever check that the spinlock is held when
> lockdep is turned on instead of whenever CONFIG_XFS_DEBUG is set?

You should regularly test with lockdep anyways. If you don't you clearly
have a testing gap. lockdep is likely to find many more locking bugs
than any of your very sparse manual annotations.

> That means it will rarely get checked during development instead of
> all the time. That's not an improvement IMO....

It's an improvement that an !CONFIG_SMP && CONFIG_XFS_DEBUG kernel
will not blow up anymore.

-Andi

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

[Dave Chinner]

On Mon, Mar 19, 2012 at 07:28:12PM -0700, Andi Kleen wrote:
> > So this means we only ever check that the spinlock is held when
> > lockdep is turned on instead of whenever CONFIG_XFS_DEBUG is set?
> 
> You should regularly test with lockdep anyways.

I do. Once or twice a release cycle I'll do a test cycle with
lockdep enabled. i don't use it on day-to-day tst cycles because of
it's overhead, and well, I understand the lock ordering of most of
the code I'm modifying....

> If you don't you clearly
> have a testing gap. lockdep is likely to find many more locking bugs
> than any of your very sparse manual annotations.

That's not true. Lockdep validates lock order but it doesn't find
race conditions. Nor does lockdep tell us that the correct lock is
held when entering the function. Nor does it tell us that the state
of a RCU freed object is correct when it is reallocated out of the
kmem_cache it belongs to. These things need manual annotations, and
they find more problems during development than lockdep does.

Not to mention that we have to annotate our read/write locks with
extra state when CONFIG_XFS_DEBUG is enabled to ensure that the lock
mode is correct because rwsems can't tell us the difference between
a read hold and a write hold.

Hell, we only check the inode lock state in about 40 different
locations via xfs_isilocked(), but many of these locations have
multiple callers. e.g.  there's a check in xfs_trans_ijoin(), and it
has about 60 callers.  So the "very sparse manual annotations" for
locking behaviour in XFS are far more comprehensive than you claim
and catch many more problems early in development under
CONFIG_XFS_DEBUG than lockdep is even aware exists....

> > That means it will rarely get checked during development instead of
> > all the time. That's not an improvement IMO....
> 
> It's an improvement that an !CONFIG_SMP && CONFIG_XFS_DEBUG kernel
> will not blow up anymore.

<shrug>

Whatever, it's not worth arguing over. There's only two places that
XFS uses spin_is_locked(), and there are other checks around it that
will catch the same race conditions with CONFIG_XFS_DEBUG enabled,
so go ahead and use lockdep for them.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com