On Mon, Oct 11, 2010 at 10:42:18PM +0200, Arnd Bergmann wrote:
> On Monday 11 October 2010 20:54:56 Josh Triplett wrote:
> > On Mon, Oct 11, 2010 at 12:52:06PM +0200, Johannes Berg wrote:
> >
> > > I don't know. Could be related to trylock issues, could be just historic
> > > since semaphores can't really be annotated, or could be something else
> > > entirely... I would expect a huge amount of warnings from sparse though
> > > if you "just" annotate them since there are things like rtnl_lock()
> > > which would have to propagate context.
> >
> > As far as I know, no reason exists to not just annotate mutexes; I think
> > mutexes just came along later and nobody happened to add the appropriate
> > annotations. (Also, sparse does handle trylock.)
> >
> > But yes, annotating mutexes will then introduce a giant pile of lock
> > warnings that need further annotation propagation. It will also
> > introduce lock warnings that represent actual bugs, making it important
> > to not just blindly propagate annotations to make warnings go away.
>
> I've given it a try, wrapping the trylock/interruptible/killable variants
> into macros and the number of additional warnings was much less than
> I had feared. This is the diff for today's sparse with today's linux-next
> x86_64_defconfig:
>
[...snip...]
>
> And this is the patch I used for testing. There may still be some flaws in it,
> but it seems to do the trick.
>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
Thanks!
One minor suggetsion below to simplify the patch; with that added,
Reviewed-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index f363bc8..d4940af 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -14,6 +14,7 @@
> #include <linux/spinlock_types.h>
> #include <linux/linkage.h>
> #include <linux/lockdep.h>
> +#include <linux/compiler.h>
>
> #include <asm/atomic.h>
>
> @@ -131,20 +132,35 @@ static inline int mutex_is_locked(struct mutex *lock)
> * Also see Documentation/mutex-design.txt.
> */
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> -extern void mutex_lock_nested(struct mutex *lock, unsigned int subclass);
> -extern int __must_check mutex_lock_interruptible_nested(struct mutex *lock,
> - unsigned int subclass);
> -extern int __must_check mutex_lock_killable_nested(struct mutex *lock,
> - unsigned int subclass);
> -
> +extern void mutex_lock_nested(struct mutex *lock, unsigned int subclass)
> + __acquires(lock);
> +extern int __must_check __mutex_lock_interruptible_nested(struct mutex *lock,
> + unsigned int subclass);
> +extern int __must_check __mutex_lock_killable_nested(struct mutex *lock,
> + unsigned int subclass);
> +#define mutex_lock_interruptible_nested(lock, subclass) ({ \
> + int __mutex_ret = __mutex_lock_interruptible_nested(lock, subclass); \
> + if (!__mutex_ret) __acquire(lock); __mutex_ret; \
> +})
> +#define mutex_lock_killable_nested(lock, subclass) ({ \
> + int __mutex_ret = __mutex_lock_killable_nested(lock, subclass); \
> + if (!__mutex_ret) __acquire(lock); __mutex_ret; \
> +})
Assuming that the underlying function only returns zero/non-zero and
that the actual return value doesn't matter, then you can use the
__cond_lock macro from compiler.h for this:
# define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0)
> #define mutex_lock(lock) mutex_lock_nested(lock, 0)
> #define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock, 0)
> #define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0)
> #else
> -extern void mutex_lock(struct mutex *lock);
> -extern int __must_check mutex_lock_interruptible(struct mutex *lock);
> -extern int __must_check mutex_lock_killable(struct mutex *lock);
> -
> +extern void mutex_lock(struct mutex *lock) __acquires(lock);
> +extern int __must_check __mutex_lock_interruptible(struct mutex *lock);
> +extern int __must_check __mutex_lock_killable(struct mutex *lock);
> +#define mutex_lock_interruptible(lock) ({ \
> + int __mutex_ret = __mutex_lock_interruptible(lock); \
> + if (!__mutex_ret) __acquire(lock); __mutex_ret; \
> +})
> +#define mutex_lock_killable(lock) ({ \
> + int __mutex_ret = __mutex_lock_killable(lock); \
> + if (!__mutex_ret) __acquire(lock); __mutex_ret; \
> +})
Same here.
> # define mutex_lock_nested(lock, subclass) mutex_lock(lock)
> # define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock)
> # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock)
> @@ -156,8 +172,16 @@ extern int __must_check mutex_lock_killable(struct mutex *lock);
> *
> * Returns 1 if the mutex has been acquired successfully, and 0 on contention.
> */
> -extern int mutex_trylock(struct mutex *lock);
> -extern void mutex_unlock(struct mutex *lock);
> -extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
> +extern int __mutex_trylock(struct mutex *lock);
> +#define mutex_trylock(lock) ({ \
> + int __mutex_ret = __mutex_trylock(lock); \
> + if (__mutex_ret) __acquire(lock); __mutex_ret; \
> +})
> +extern void mutex_unlock(struct mutex *lock) __releases(lock);
> +extern int __atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
> +#define atomic_dec_and_mutex_lock(cnt, lock) ({ \
> + int __mutex_ret = __atomic_dec_and_mutex_lock(cnt, lock); \
> + if (__mutex_ret) __acquire(lock); __mutex_ret; \
> +})
And here.
- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
