When writing code for UP without CONFIG_DEBUG_SPINLOCK it's easy to get
the first argument to the spinlock/rwlock functions wrong. This is
because the parameter is not actually used in this configuration.
Typically you will only find out it's wrong
* by rebuilding with CONFIG_SMP or CONFIG_DEBUG_SPINLOCK
* after you've submitted your beautiful patch series.
The first means a long wait, and the latter is a bit late.
Add typechecking on the first argument of these macro functions. Note
that since the typecheck now references the variable, the explicit read
is redundant and can be removed.
This change causes compiler warnings in net/ipv4/route.c, as this passes
NULL as the first argument in the UP configuration. Simply cast this.
Signed-off-by: David Kilroy <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
I've checked the assembly and object output resulting from this change
for x86. As far as I can tell there are only differences in the
debugging symbols used to refer to some variables. The stripped objects
which I checked are identical.
I've also done an allyesconfig build (with various lock debugging
options off to keep CONFIG_DEBUG_SPINLOCK off), and there are no extra
compiler warnings (except those addressed in route.c).
---
include/linux/spinlock_api_up.h | 90 +++++++++++++++++++-------------------
net/ipv4/route.c | 2 +-
2 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h
index 04e1d31..7780138 100644
--- a/include/linux/spinlock_api_up.h
+++ b/include/linux/spinlock_api_up.h
@@ -24,58 +24,58 @@
* flags straight, to suppress compiler warnings of unused lock
* variables, and to add the proper checker annotations:
*/
-#define __LOCK(lock) \
- do { preempt_disable(); __acquire(lock); (void)(lock); } while (0)
+#define __LOCK(type, lock) \
+ do { typecheck(type*, lock); preempt_disable(); __acquire(lock); } while (0)
-#define __LOCK_BH(lock) \
- do { local_bh_disable(); __LOCK(lock); } while (0)
+#define __LOCK_BH(type, lock) \
+ do { local_bh_disable(); __LOCK(type, lock); } while (0)
-#define __LOCK_IRQ(lock) \
- do { local_irq_disable(); __LOCK(lock); } while (0)
+#define __LOCK_IRQ(type, lock) \
+ do { local_irq_disable(); __LOCK(type, lock); } while (0)
-#define __LOCK_IRQSAVE(lock, flags) \
- do { local_irq_save(flags); __LOCK(lock); } while (0)
+#define __LOCK_IRQSAVE(type, lock, flags) \
+ do { local_irq_save(flags); __LOCK(type, lock); } while (0)
-#define __UNLOCK(lock) \
- do { preempt_enable(); __release(lock); (void)(lock); } while (0)
+#define __UNLOCK(type, lock) \
+ do { typecheck(type*, lock); preempt_enable(); __release(lock); } while (0)
-#define __UNLOCK_BH(lock) \
- do { preempt_enable_no_resched(); local_bh_enable(); __release(lock); (void)(lock); } while (0)
+#define __UNLOCK_BH(type, lock) \
+ do { typecheck(type*, lock); preempt_enable_no_resched(); local_bh_enable(); __release(lock); } while (0)
-#define __UNLOCK_IRQ(lock) \
- do { local_irq_enable(); __UNLOCK(lock); } while (0)
+#define __UNLOCK_IRQ(type, lock) \
+ do { local_irq_enable(); __UNLOCK(type, lock); } while (0)
-#define __UNLOCK_IRQRESTORE(lock, flags) \
- do { local_irq_restore(flags); __UNLOCK(lock); } while (0)
+#define __UNLOCK_IRQRESTORE(type, lock, flags) \
+ do { local_irq_restore(flags); __UNLOCK(type, lock); } while (0)
-#define _spin_lock(lock) __LOCK(lock)
-#define _spin_lock_nested(lock, subclass) __LOCK(lock)
-#define _read_lock(lock) __LOCK(lock)
-#define _write_lock(lock) __LOCK(lock)
-#define _spin_lock_bh(lock) __LOCK_BH(lock)
-#define _read_lock_bh(lock) __LOCK_BH(lock)
-#define _write_lock_bh(lock) __LOCK_BH(lock)
-#define _spin_lock_irq(lock) __LOCK_IRQ(lock)
-#define _read_lock_irq(lock) __LOCK_IRQ(lock)
-#define _write_lock_irq(lock) __LOCK_IRQ(lock)
-#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
-#define _read_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
-#define _write_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
-#define _spin_trylock(lock) ({ __LOCK(lock); 1; })
-#define _read_trylock(lock) ({ __LOCK(lock); 1; })
-#define _write_trylock(lock) ({ __LOCK(lock); 1; })
-#define _spin_trylock_bh(lock) ({ __LOCK_BH(lock); 1; })
-#define _spin_unlock(lock) __UNLOCK(lock)
-#define _read_unlock(lock) __UNLOCK(lock)
-#define _write_unlock(lock) __UNLOCK(lock)
-#define _spin_unlock_bh(lock) __UNLOCK_BH(lock)
-#define _write_unlock_bh(lock) __UNLOCK_BH(lock)
-#define _read_unlock_bh(lock) __UNLOCK_BH(lock)
-#define _spin_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _read_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _write_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _spin_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(lock, flags)
-#define _read_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(lock, flags)
-#define _write_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(lock, flags)
+#define _spin_lock(lock) __LOCK(spinlock_t, lock)
+#define _spin_lock_nested(lock, subclass) __LOCK(spinlock_t, lock)
+#define _read_lock(lock) __LOCK(rwlock_t, lock)
+#define _write_lock(lock) __LOCK(rwlock_t, lock)
+#define _spin_lock_bh(lock) __LOCK_BH(spinlock_t, lock)
+#define _read_lock_bh(lock) __LOCK_BH(rwlock_t, lock)
+#define _write_lock_bh(lock) __LOCK_BH(rwlock_t, lock)
+#define _spin_lock_irq(lock) __LOCK_IRQ(spinlock_t, lock)
+#define _read_lock_irq(lock) __LOCK_IRQ(rwlock_t, lock)
+#define _write_lock_irq(lock) __LOCK_IRQ(rwlock_t, lock)
+#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(spinlock_t, lock, flags)
+#define _read_lock_irqsave(lock, flags) __LOCK_IRQSAVE(rwlock_t, lock, flags)
+#define _write_lock_irqsave(lock, flags) __LOCK_IRQSAVE(rwlock_t, lock, flags)
+#define _spin_trylock(lock) ({ __LOCK(spinlock_t, lock); 1; })
+#define _read_trylock(lock) ({ __LOCK(rwlock_t, lock); 1; })
+#define _write_trylock(lock) ({ __LOCK(rwlock_t, lock); 1; })
+#define _spin_trylock_bh(lock) ({ __LOCK_BH(spinlock_t, lock); 1; })
+#define _spin_unlock(lock) __UNLOCK(spinlock_t, lock)
+#define _read_unlock(lock) __UNLOCK(rwlock_t, lock)
+#define _write_unlock(lock) __UNLOCK(rwlock_t, lock)
+#define _spin_unlock_bh(lock) __UNLOCK_BH(spinlock_t, lock)
+#define _write_unlock_bh(lock) __UNLOCK_BH(rwlock_t, lock)
+#define _read_unlock_bh(lock) __UNLOCK_BH(rwlock_t, lock)
+#define _spin_unlock_irq(lock) __UNLOCK_IRQ(spinlock_t, lock)
+#define _read_unlock_irq(lock) __UNLOCK_IRQ(rwlock_t, lock)
+#define _write_unlock_irq(lock) __UNLOCK_IRQ(rwlock_t, lock)
+#define _spin_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(spinlock_t, lock, flags)
+#define _read_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(rwlock_t, lock, flags)
+#define _write_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(rwlock_t, lock, flags)
#endif /* __LINUX_SPINLOCK_API_UP_H */
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 28205e5..8edfffd 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -242,7 +242,7 @@ static __init void rt_hash_lock_init(void)
spin_lock_init(&rt_hash_locks[i]);
}
#else
-# define rt_hash_lock_addr(slot) NULL
+# define rt_hash_lock_addr(slot) ((spinlock_t *) NULL)
static inline void rt_hash_lock_init(void)
{
--
1.6.0.6
* David Kilroy <[email protected]> wrote:
> When writing code for UP without CONFIG_DEBUG_SPINLOCK it's easy
> to get the first argument to the spinlock/rwlock functions wrong.
> This is because the parameter is not actually used in this
> configuration.
>
> Typically you will only find out it's wrong
> * by rebuilding with CONFIG_SMP or CONFIG_DEBUG_SPINLOCK
> * after you've submitted your beautiful patch series.
>
> The first means a long wait, and the latter is a bit late.
>
> Add typechecking on the first argument of these macro functions.
> Note that since the typecheck now references the variable, the
> explicit read is redundant and can be removed.
>
> This change causes compiler warnings in net/ipv4/route.c, as this
> passes NULL as the first argument in the UP configuration. Simply
> cast this.
Wondering - can the wrappers be moved from CPP land to C land by
turning them into inlines? (i havent checked all usages so there
might be some surprises, but by and large it ought to be possible.)
Ingo
Ingo Molnar wrote:
> * David Kilroy <[email protected]> wrote:
>
>> When writing code for UP without CONFIG_DEBUG_SPINLOCK it's easy
>> to get the first argument to the spinlock/rwlock functions wrong.
>> This is because the parameter is not actually used in this
>> configuration.
>>
>> Typically you will only find out it's wrong
>> * by rebuilding with CONFIG_SMP or CONFIG_DEBUG_SPINLOCK
>> * after you've submitted your beautiful patch series.
>>
>> The first means a long wait, and the latter is a bit late.
>>
>> Add typechecking on the first argument of these macro functions.
>> Note that since the typecheck now references the variable, the
>> explicit read is redundant and can be removed.
>>
>> This change causes compiler warnings in net/ipv4/route.c, as this
>> passes NULL as the first argument in the UP configuration. Simply
>> cast this.
>
> Wondering - can the wrappers be moved from CPP land to C land by
> turning them into inlines? (i havent checked all usages so there
> might be some surprises, but by and large it ought to be possible.)
I thought about doing it that way. I decided not to because I suspected
it would be harder to verify that the behaviour is unchanged.
Also the _lock_irqsave functions output to the flags parameter (which
isn't a pointer) so that has to remain a macro.
If you'd really rather an inline version, I can spend some time looking
into it.
Dave.
* Dave <[email protected]> wrote:
> Ingo Molnar wrote:
> > * David Kilroy <[email protected]> wrote:
> >
> >> When writing code for UP without CONFIG_DEBUG_SPINLOCK it's easy
> >> to get the first argument to the spinlock/rwlock functions wrong.
> >> This is because the parameter is not actually used in this
> >> configuration.
> >>
> >> Typically you will only find out it's wrong
> >> * by rebuilding with CONFIG_SMP or CONFIG_DEBUG_SPINLOCK
> >> * after you've submitted your beautiful patch series.
> >>
> >> The first means a long wait, and the latter is a bit late.
> >>
> >> Add typechecking on the first argument of these macro functions.
> >> Note that since the typecheck now references the variable, the
> >> explicit read is redundant and can be removed.
> >>
> >> This change causes compiler warnings in net/ipv4/route.c, as this
> >> passes NULL as the first argument in the UP configuration. Simply
> >> cast this.
> >
> > Wondering - can the wrappers be moved from CPP land to C land by
> > turning them into inlines? (i havent checked all usages so there
> > might be some surprises, but by and large it ought to be
> > possible.)
>
> I thought about doing it that way. I decided not to because I
> suspected it would be harder to verify that the behaviour is
> unchanged.
These things break noisily if they are wrong so i wouldnt be worried
about that aspect.
> Also the _lock_irqsave functions output to the flags parameter
> (which isn't a pointer) so that has to remain a macro.
Do we still need it? I remember it was originally due to some
sparc32-ness, but meanwhile that's fixed in Sparc so we can
generally pass irq flags around at will.
> If you'd really rather an inline version, I can spend some time
> looking into it.
Would be nice.
Ingo
When writing code for UP without CONFIG_DEBUG_SPINLOCK it's easy to get
the first argument to the spinlock/rwlock functions wrong. This is
because the parameter is not actually used in this configuration.
Typically you will only find out it's wrong
* by rebuilding with CONFIG_SMP or CONFIG_DEBUG_SPINLOCK
* after you've submitted your beautiful patch series.
The first means a long wait, and the latter is a bit late.
Change the intermediate macros into inline functions.
Signed-off-by: David Kilroy <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
The previous version of this patch achieved the typechecking by
explicitly calling typecheck() within the macros. This version, at the
behest of Ingo, turns the macros into proper C functions. One benefit of
this is that the compiler no longer complains about how net/ipv4/route.c
passes NULL into spinlock_xx()
I've made the functions static inline, so there should be minimal impact
on generated code (assuming the compiler is listening). I haven't done
any comparisons yet.
In an allyesconfig build, compiler warnings are unchanged. One
side-effect of this is that sparse (with -Wcontext) reports certain
locking errors against spinlock_api_up.h instead of the file containing
the function doing the spinlock call. For example:
CHECK kernel/rtmutex.c
kernel/rtmutex.c:149:5: warning: symbol 'max_lock_depth' was not declared. Should it be static?
kernel/rtmutex.c:291:2: warning: context imbalance in 'rt_mutex_adjust_prio_chain' - different lock contexts for basic block
kernel/rtmutex.c:675:3: warning: context imbalance in '__rt_mutex_slowlock' - unexpected unlock
kernel/rtmutex.c:1029:5: warning: context imbalance in 'rt_mutex_start_proxy_lock' - wrong count at exit
CC kernel/rtmutex.o
becomes...
CHECK kernel/rtmutex.c
kernel/rtmutex.c:149:5: warning: symbol 'max_lock_depth' was not declared. Should it be static?
kernel/rtmutex.c:291:2: warning: context imbalance in 'rt_mutex_adjust_prio_chain' - different lock contexts for basic block
include/linux/spinlock_api_up.h:103:3: warning: context imbalance in '__rt_mutex_slowlock' - unexpected unlock
kernel/rtmutex.c:1029:5: warning: context imbalance in 'rt_mutex_start_proxy_lock' - wrong count at exit
CC kernel/rtmutex.o
This might just be my version of sparse. I tried adding the __acquires()
tag to the inline functions, but sparse didn't behave any differently.
Ingo - is this what you were thinking of?
Thanks,
Dave.
---
include/linux/spinlock.h | 6 +-
include/linux/spinlock_api_up.h | 118 +++++++++++++++++++++++++++++----------
2 files changed, 92 insertions(+), 32 deletions(-)
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 252b245..6462de1 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -244,17 +244,17 @@ do { \
#define spin_lock_irqsave(lock, flags) \
do { \
typecheck(unsigned long, flags); \
- _spin_lock_irqsave(lock, flags); \
+ _spin_lock_irqsave(lock, &flags); \
} while (0)
#define read_lock_irqsave(lock, flags) \
do { \
typecheck(unsigned long, flags); \
- _read_lock_irqsave(lock, flags); \
+ _read_lock_irqsave(lock, &flags); \
} while (0)
#define write_lock_irqsave(lock, flags) \
do { \
typecheck(unsigned long, flags); \
- _write_lock_irqsave(lock, flags); \
+ _write_lock_irqsave(lock, &flags); \
} while (0)
#define spin_lock_irqsave_nested(lock, flags, subclass) \
spin_lock_irqsave(lock, flags)
diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h
index 04e1d31..ed9197d 100644
--- a/include/linux/spinlock_api_up.h
+++ b/include/linux/spinlock_api_up.h
@@ -48,34 +48,94 @@
#define __UNLOCK_IRQRESTORE(lock, flags) \
do { local_irq_restore(flags); __UNLOCK(lock); } while (0)
-#define _spin_lock(lock) __LOCK(lock)
-#define _spin_lock_nested(lock, subclass) __LOCK(lock)
-#define _read_lock(lock) __LOCK(lock)
-#define _write_lock(lock) __LOCK(lock)
-#define _spin_lock_bh(lock) __LOCK_BH(lock)
-#define _read_lock_bh(lock) __LOCK_BH(lock)
-#define _write_lock_bh(lock) __LOCK_BH(lock)
-#define _spin_lock_irq(lock) __LOCK_IRQ(lock)
-#define _read_lock_irq(lock) __LOCK_IRQ(lock)
-#define _write_lock_irq(lock) __LOCK_IRQ(lock)
-#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
-#define _read_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
-#define _write_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
-#define _spin_trylock(lock) ({ __LOCK(lock); 1; })
-#define _read_trylock(lock) ({ __LOCK(lock); 1; })
-#define _write_trylock(lock) ({ __LOCK(lock); 1; })
-#define _spin_trylock_bh(lock) ({ __LOCK_BH(lock); 1; })
-#define _spin_unlock(lock) __UNLOCK(lock)
-#define _read_unlock(lock) __UNLOCK(lock)
-#define _write_unlock(lock) __UNLOCK(lock)
-#define _spin_unlock_bh(lock) __UNLOCK_BH(lock)
-#define _write_unlock_bh(lock) __UNLOCK_BH(lock)
-#define _read_unlock_bh(lock) __UNLOCK_BH(lock)
-#define _spin_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _read_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _write_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _spin_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(lock, flags)
-#define _read_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(lock, flags)
-#define _write_unlock_irqrestore(lock, flags) __UNLOCK_IRQRESTORE(lock, flags)
+static inline void _spin_lock(spinlock_t *lock)
+{ __LOCK(lock); }
+
+static inline void _spin_lock_nested(spinlock_t *lock, int subclass)
+{ __LOCK(lock); }
+
+static inline void _read_lock(rwlock_t *lock)
+{ __LOCK(lock); }
+
+static inline void _write_lock(rwlock_t *lock)
+{ __LOCK(lock); }
+
+static inline void _spin_lock_bh(spinlock_t *lock)
+{ __LOCK_BH(lock); }
+
+static inline void _read_lock_bh(rwlock_t *lock)
+{ __LOCK_BH(lock); }
+
+static inline void _write_lock_bh(rwlock_t *lock)
+{ __LOCK_BH(lock); }
+
+static inline void _spin_lock_irq(spinlock_t *lock)
+{ __LOCK_IRQ(lock); }
+
+static inline void _read_lock_irq(rwlock_t *lock)
+{ __LOCK_IRQ(lock); }
+
+static inline void _write_lock_irq(rwlock_t *lock)
+{ __LOCK_IRQ(lock); }
+
+static inline void _spin_lock_irqsave(spinlock_t *lock, unsigned long *flags)
+{ __LOCK_IRQSAVE(lock, *flags); }
+
+static inline void _read_lock_irqsave(rwlock_t *lock, unsigned long *flags)
+{ __LOCK_IRQSAVE(lock, *flags); }
+
+static inline void _write_lock_irqsave(rwlock_t *lock, unsigned long *flags)
+{ __LOCK_IRQSAVE(lock, *flags); }
+
+static inline int _spin_trylock(spinlock_t *lock)
+{ __LOCK(lock); return 1; }
+
+static inline int _read_trylock(rwlock_t *lock)
+{ __LOCK(lock); return 1; }
+
+static inline int _write_trylock(rwlock_t *lock)
+{ __LOCK(lock); return 1; }
+
+static inline int _spin_trylock_bh(spinlock_t *lock)
+{ __LOCK(lock); return 1; }
+
+static inline void _spin_unlock(spinlock_t *lock)
+{ __UNLOCK(lock); }
+
+static inline void _read_unlock(rwlock_t *lock)
+{ __UNLOCK(lock); }
+
+static inline void _write_unlock(rwlock_t *lock)
+{ __UNLOCK(lock); }
+
+static inline void _spin_unlock_bh(spinlock_t *lock)
+{ __UNLOCK_BH(lock); }
+
+static inline void _read_unlock_bh(rwlock_t *lock)
+{ __UNLOCK_BH(lock); }
+
+static inline void _write_unlock_bh(rwlock_t *lock)
+{ __UNLOCK_BH(lock); }
+
+static inline void _spin_unlock_irq(spinlock_t *lock)
+{ __UNLOCK_IRQ(lock); }
+
+static inline void _read_unlock_irq(rwlock_t *lock)
+{ __UNLOCK_IRQ(lock); }
+
+static inline void _write_unlock_irq(rwlock_t *lock)
+{ __UNLOCK_IRQ(lock); }
+
+static inline void _spin_unlock_irqrestore(spinlock_t *lock,
+ unsigned long flags)
+{ __UNLOCK_IRQRESTORE(lock, flags); }
+
+static inline void _read_unlock_irqrestore(rwlock_t *lock,
+ unsigned long flags)
+{ __UNLOCK_IRQRESTORE(lock, flags); }
+
+static inline void _write_unlock_irqrestore(rwlock_t *lock,
+ unsigned long flags)
+{ __UNLOCK_IRQRESTORE(lock, flags); }
#endif /* __LINUX_SPINLOCK_API_UP_H */
--
1.6.3.3