On some configs, sparse spinlock balance checking is broken

On some configs, sparse spinlock balance checking is broken

[Roland Dreier]

(Ingo -- you seem to be the last person to touch all this stuff, and I
can't untangle what you did, hence I'm sending this email to you)

On at least some of my configs on x86_64, when running sparse, I see
bogus 'warning: context imbalance in '<func>' - wrong count at exit'.

This seems to be because I have CONFIG_SMP=y, CONFIG_DEBUG_SPINLOCK=n
and CONFIG_PREEMPT=n.  Therefore, <linux/spinlock.h> does

	#define spin_lock(lock)			_spin_lock(lock)

which picks up

	void __lockfunc _spin_lock(spinlock_t *lock)		__acquires(lock);

from <linux/spinlock_api_smp.h>, but <linux/spinlock.h> also has:

	#if defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT) || \
		!defined(CONFIG_SMP)
	//...
	#else
	# define spin_unlock(lock)		__raw_spin_unlock(&(lock)->raw_lock)

and <asm-x86_64/spinlock.h> has:

	static inline void __raw_spin_unlock(raw_spinlock_t *lock)
	{
		asm volatile("movl $1,%0" :"=m" (lock->slock) :: "memory");
	}

so sparse doesn't see any __releases() to match the __acquires.

This all seems to go back to commit bda98685 ("x86: inline spin_unlock
if !CONFIG_DEBUG_SPINLOCK and !CONFIG_PREEMPT") but I don't know what
motivated that change.

Anyway, Ingo or anyone else, what's the best way to fix this?  Maybe
the right way to fix this is just to define away __acquires/__releases
unless CONFIG_DEBUG_SPINLOCK is set, but that seems suboptimal.

Thanks,
  Roland

Re: On some configs, sparse spinlock balance checking is broken

[Ingo Molnar]

* Roland Dreier <rdreier@cisco.com> wrote:

> (Ingo -- you seem to be the last person to touch all this stuff, and I 
> can't untangle what you did, hence I'm sending this email to you)
> 
> On at least some of my configs on x86_64, when running sparse, I see 
> bogus 'warning: context imbalance in '<func>' - wrong count at exit'.
> 
> This seems to be because I have CONFIG_SMP=y, CONFIG_DEBUG_SPINLOCK=n
> and CONFIG_PREEMPT=n.  Therefore, <linux/spinlock.h> does
> 
> 	#define spin_lock(lock)			_spin_lock(lock)
> 
> which picks up
> 
> 	void __lockfunc _spin_lock(spinlock_t *lock)		__acquires(lock);
> 
> from <linux/spinlock_api_smp.h>, but <linux/spinlock.h> also has:
> 
> 	#if defined(CONFIG_DEBUG_SPINLOCK) || defined(CONFIG_PREEMPT) || \
> 		!defined(CONFIG_SMP)
> 	//...
> 	#else
> 	# define spin_unlock(lock)		__raw_spin_unlock(&(lock)->raw_lock)

this is the direct-inlining speedup some people insisted on.

> and <asm-x86_64/spinlock.h> has:
> 
> 	static inline void __raw_spin_unlock(raw_spinlock_t *lock)
> 	{
> 		asm volatile("movl $1,%0" :"=m" (lock->slock) :: "memory");
> 	}
> 
> so sparse doesn't see any __releases() to match the __acquires.
> 
> This all seems to go back to commit bda98685 ("x86: inline spin_unlock
> if !CONFIG_DEBUG_SPINLOCK and !CONFIG_PREEMPT") but I don't know what
> motivated that change.
> 
> Anyway, Ingo or anyone else, what's the best way to fix this?  Maybe 
> the right way to fix this is just to define away __acquires/__releases 
> unless CONFIG_DEBUG_SPINLOCK is set, but that seems suboptimal.

i think the right way to fix it might be to define a _spin_unlock() 
within those #ifdef branches, and then to define spin_lock as:

static inline void spin_lock(spinlock_t *lock) __acquires(lock)
{
	_spin_lock(lock);
}

?

	Ingo

Re: On some configs, sparse spinlock balance checking is broken

[Roland Dreier]

 > i think the right way to fix it might be to define a _spin_unlock() 
 > within those #ifdef branches, and then to define spin_lock as:
 > 
 > static inline void spin_lock(spinlock_t *lock) __acquires(lock)

I tried a similar approach, but what got me was that sparse doesn't
pay attention to the "__acquires()" annotation there.  However I now
realized that putting "__acquire()" inside the implementation of the
function (which sparse can see for inline functions) actually works.

And actually the lock stuff is OK, since it's not inlined -- it's the
unlock stuff that goes directly to the __raw versions.  But something
like the following works for me; does it look OK to you?

---

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 94b767d..8ec4142 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -228,15 +228,45 @@ do {								\
 # define read_unlock_irq(lock)		_read_unlock_irq(lock)
 # define write_unlock_irq(lock)		_write_unlock_irq(lock)
 #else
-# define spin_unlock(lock)		__raw_spin_unlock(&(lock)->raw_lock)
-# define read_unlock(lock)		__raw_read_unlock(&(lock)->raw_lock)
-# define write_unlock(lock)		__raw_write_unlock(&(lock)->raw_lock)
-# define spin_unlock_irq(lock) \
-    do { __raw_spin_unlock(&(lock)->raw_lock); local_irq_enable(); } while (0)
-# define read_unlock_irq(lock) \
-    do { __raw_read_unlock(&(lock)->raw_lock); local_irq_enable(); } while (0)
-# define write_unlock_irq(lock) \
-    do { __raw_write_unlock(&(lock)->raw_lock); local_irq_enable(); } while (0)
+static inline void spin_unlock(spinlock_t *lock)
+{
+	__release(lock);
+	__raw_spin_unlock(&(lock)->raw_lock);
+}
+
+static inline void read_unlock(rwlock_t *lock)
+{
+	__release(lock);
+	__raw_read_unlock(&(lock)->raw_lock);
+}
+
+static inline void write_unlock(rwlock_t *lock)
+{
+	__release(lock);
+	__raw_write_unlock(&(lock)->raw_lock);
+}
+
+static inline void spin_unlock_irq(spinlock_t *lock)
+{
+	__release(lock);
+	__raw_spin_unlock(&(lock)->raw_lock);
+	local_irq_enable();
+}
+
+static inline void read_unlock_irq(rwlock_t *lock)
+{
+	__release(lock);
+	__raw_read_unlock(&(lock)->raw_lock);
+	local_irq_enable();
+}
+
+static inline void write_unlock_irq(rwlock_t *lock)
+{
+	__release(lock);
+	__raw_write_unlock(&(lock)->raw_lock);
+	local_irq_enable();
+}
+
 #endif
 
 #define spin_unlock_irqrestore(lock, flags) \

Re: On some configs, sparse spinlock balance checking is broken

[Ingo Molnar]

* Roland Dreier <rdreier@cisco.com> wrote:

> And actually the lock stuff is OK, since it's not inlined -- it's the 
> unlock stuff that goes directly to the __raw versions.  But something 
> like the following works for me; does it look OK to you?

yeah, it looks good to me too. Hopefully this will work with the include 
file ordering of all platforms.

	Ingo