lkml.org 
[lkml]   [2006]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch] lockdep, annotate slocks: turn lockdep off for them
On 6/30/06, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> > > bh_lock_sock(sk);
> > > - if (!sock_owned_by_user(sk))
> > > + if (!sock_owned_by_user(sk)) {
> > > + /*
> > > + * trylock + unlock semantics:
> > > + */
> > > + spin_release(&sk->sk_lock.slock.dep_map, 1, _RET_IP_);
> > > + mutex_acquire(&sk->sk_lock.dep_map, 0, 1, _RET_IP_);
> >
> > Although it would seem that keeping the spin lock would fit the actual
> > semantics better. I suppose there must be a technical reason why this
> > wouldn't work.
>
> good point. The basic issue is the 'virtual lock inversion' that occurs
> in the lock vs. release paths. [between taking the slock and taking the
> new sk_lock type]
>
> The situation is like this: we construct 'complex' lock types [mutex,
> rwsem, sk_lock] out of 'primitive' lock types [spinlock, rwlock]. Both
> the complex type and the primite types exist separately, and might have
> lock-validator acquire/release operations. These locks can interact and
> if we do the complex lock acquire/release while holding the primitive
> lock, the validator sees inverse ordering between them.
>
> For the mutex code i solved the inversion problem by using a
> raw_spinlock for the primitive type (which has no lockdep operations),
> hence the complex lock type.
>
> But in this particular sk_lock case we can do it even more cleanly i
> think and can preserve the lockdep awareness of the primitive type too:
> by releasing the complex lock before taking the primitive lock in the
> release_sock() unlock path. The updated patch below does this - and thus
> i was able to remove the dropping of the primitive spinlock type.
>
> it is not a problem that the release of the complex lock type does not
> happen inside the critical section: from the point where we release the
> complex lock-type _this_ context cannot take any other locks, so there
> are no dependencies missed.
>
> As you can see, the lock validator can easily cover completely new lock
> types like sk_lock too, as long as the new lock type has some
> minimalistic "works like a lock" properties. (such as owner-does-unlock)
>
> later on i'll try the same cleanup for the mutex code too - it should be
> possible. (that way the implementation of complex lock types can be
> lock-validator checked too)
>
> Ingo
>
> --------------->
> Subject: lockdep, annotate sk_locks
> From: Ingo Molnar <mingo@elte.hu>
>
> Teach sk_lock semantics to the lock validator. In the softirq path
> the slock has mutex_trylock()+mutex_unlock() semantics, in the
> process context sock_lock() case it has mutex_lock()/mutex_unlock()
> semantics.
>
> Thus we treat sock_owned_by_user() flagged areas as an exclusion
> area too, not just those areas covered by a held sk_lock.slock.
>
> Effect on non-lockdep kernels: minimal, sk_lock_sock_init() has
> been turned into an inline function.
>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
> include/net/sock.h | 20 +++++------
> net/core/sock.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 93 insertions(+), 19 deletions(-)
>
> Index: linux/include/net/sock.h
> ===================================================================
> --- linux.orig/include/net/sock.h
> +++ linux/include/net/sock.h
> @@ -44,6 +44,7 @@
> #include <linux/timer.h>
> #include <linux/cache.h>
> #include <linux/module.h>
> +#include <linux/lockdep.h>
> #include <linux/netdevice.h>
> #include <linux/skbuff.h> /* struct sk_buff */
> #include <linux/security.h>
> @@ -78,18 +79,17 @@ typedef struct {
> spinlock_t slock;
> struct sock_iocb *owner;
> wait_queue_head_t wq;
> + /*
> + * We express the mutex-alike socket_lock semantics
> + * to the lock validator by explicitly managing
> + * the slock as a lock variant (in addition to
> + * the slock itself):
> + */
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> + struct lockdep_map dep_map;
> +#endif
> } socket_lock_t;
>
> -extern struct lock_class_key af_family_keys[AF_MAX];
> -
> -#define sock_lock_init(__sk) \
> -do { spin_lock_init(&((__sk)->sk_lock.slock)); \
> - lockdep_set_class(&(__sk)->sk_lock.slock, \
> - af_family_keys + (__sk)->sk_family); \
> - (__sk)->sk_lock.owner = NULL; \
> - init_waitqueue_head(&((__sk)->sk_lock.wq)); \
> -} while(0)
> -
> struct sock;
> struct proto;
>
> Index: linux/net/core/sock.c
> ===================================================================
> --- linux.orig/net/core/sock.c
> +++ linux/net/core/sock.c
> @@ -134,7 +134,40 @@
> * Each address family might have different locking rules, so we have
> * one slock key per address family:
> */
> -struct lock_class_key af_family_keys[AF_MAX];
> +static struct lock_class_key af_family_keys[AF_MAX];
> +static struct lock_class_key af_family_slock_keys[AF_MAX];
> +
> +#ifdef CONFIG_LOCKDEP
> +/*
> + * Make lock validator output more readable:
> + */
> +static const char *af_family_key_strings[AF_MAX+1] = {
> + "sk_lock-AF_UNSPEC", "sk_lock-AF_UNIX" , "sk_lock-AF_INET" ,
> + "sk_lock-AF_AX25" , "sk_lock-AF_IPX" , "sk_lock-AF_APPLETALK",
> + "sk_lock-AF_NETROM", "sk_lock-AF_BRIDGE" , "sk_lock-AF_ATMPVC" ,
> + "sk_lock-AF_X25" , "sk_lock-AF_INET6" , "sk_lock-AF_ROSE" ,
> + "sk_lock-AF_DECnet", "sk_lock-AF_NETBEUI" , "sk_lock-AF_SECURITY" ,
> + "sk_lock-AF_KEY" , "sk_lock-AF_NETLINK" , "sk_lock-AF_PACKET" ,
> + "sk_lock-AF_ASH" , "sk_lock-AF_ECONET" , "sk_lock-AF_ATMSVC" ,
> + "sk_lock-21" , "sk_lock-AF_SNA" , "sk_lock-AF_IRDA" ,
> + "sk_lock-AF_PPPOX" , "sk_lock-AF_WANPIPE" , "sk_lock-AF_LLC" ,
> + "sk_lock-27" , "sk_lock-28" , "sk_lock-29" ,
> + "sk_lock-AF_TIPC" , "sk_lock-AF_BLUETOOTH", "sk_lock-AF_MAX"
> +};
> +static const char *af_family_slock_key_strings[AF_MAX+1] = {
> + "slock-AF_UNSPEC", "slock-AF_UNIX" , "slock-AF_INET" ,
> + "slock-AF_AX25" , "slock-AF_IPX" , "slock-AF_APPLETALK",
> + "slock-AF_NETROM", "slock-AF_BRIDGE" , "slock-AF_ATMPVC" ,
> + "slock-AF_X25" , "slock-AF_INET6" , "slock-AF_ROSE" ,
> + "slock-AF_DECnet", "slock-AF_NETBEUI" , "slock-AF_SECURITY" ,
> + "slock-AF_KEY" , "slock-AF_NETLINK" , "slock-AF_PACKET" ,
> + "slock-AF_ASH" , "slock-AF_ECONET" , "slock-AF_ATMSVC" ,
> + "slock-21" , "slock-AF_SNA" , "slock-AF_IRDA" ,
> + "slock-AF_PPPOX" , "slock-AF_WANPIPE" , "slock-AF_LLC" ,
> + "slock-27" , "slock-28" , "slock-29" ,
> + "slock-AF_TIPC" , "slock-AF_BLUETOOTH", "slock-AF_MAX"
> +};
> +#endif
>
> /*
> * sk_callback_lock locking rules are per-address-family,
> @@ -250,9 +283,16 @@ int sk_receive_skb(struct sock *sk, stru
> skb->dev = NULL;
>
> bh_lock_sock(sk);
> - if (!sock_owned_by_user(sk))
> + if (!sock_owned_by_user(sk)) {
> + /*
> + * trylock + unlock semantics:
> + */
> + mutex_acquire(&sk->sk_lock.dep_map, 0, 1, _RET_IP_);
> +
> rc = sk->sk_backlog_rcv(sk, skb);
> - else
> +
> + mutex_release(&sk->sk_lock.dep_map, 1, _RET_IP_);
> + } else
> sk_add_backlog(sk, skb);
> bh_unlock_sock(sk);
> out:
> @@ -762,6 +802,30 @@ lenout:
> return 0;
> }
>
> +/*
> + * Initialize an sk_lock.
> + *
> + * (We also register the sk_lock with the lock validator.)
> + */
> +static void inline sock_lock_init(struct sock *sk)
> +{
> + spin_lock_init(&sk->sk_lock.slock);
> + lockdep_set_class_and_name(&sk->sk_lock.slock,
> + af_family_slock_keys + sk->sk_family,
> + af_family_slock_key_strings[sk->sk_family]);
> + sk->sk_lock.owner = NULL;
> + init_waitqueue_head(&sk->sk_lock.wq);
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> + /*
> + * Make sure we are not reinitializing a held lock:
> + */
> + debug_check_no_locks_freed((void *)&sk->sk_lock, sizeof(sk->sk_lock));
> + lockdep_init_map(&sk->sk_lock.dep_map,
> + af_family_key_strings[sk->sk_family],
> + af_family_keys + sk->sk_family);
> +#endif
> +}
> +
> /**
> * sk_alloc - All socket objects are allocated here
> * @family: protocol family
> @@ -1466,24 +1530,34 @@ void sock_init_data(struct socket *sock,
> void fastcall lock_sock(struct sock *sk)
> {
> might_sleep();
> - spin_lock_bh(&(sk->sk_lock.slock));
> + spin_lock_bh(&sk->sk_lock.slock);
> if (sk->sk_lock.owner)
> __lock_sock(sk);
> sk->sk_lock.owner = (void *)1;
> - spin_unlock_bh(&(sk->sk_lock.slock));
> + spin_unlock(&sk->sk_lock.slock);
> + /*
> + * The sk_lock has mutex_lock() semantics here:
> + */
> + mutex_acquire(&sk->sk_lock.dep_map, 0, 0, _RET_IP_);
> + local_bh_enable();
> }
>
> EXPORT_SYMBOL(lock_sock);
>
> void fastcall release_sock(struct sock *sk)
> {
> - spin_lock_bh(&(sk->sk_lock.slock));
> + /*
> + * The sk_lock has mutex_unlock() semantics:
> + */
> + mutex_release(&sk->sk_lock.dep_map, 1, _RET_IP_);
> +
> + spin_lock_bh(&sk->sk_lock.slock);
> if (sk->sk_backlog.tail)
> __release_sock(sk);
> sk->sk_lock.owner = NULL;
> - if (waitqueue_active(&(sk->sk_lock.wq)))
> - wake_up(&(sk->sk_lock.wq));
> - spin_unlock_bh(&(sk->sk_lock.slock));
> + if (waitqueue_active(&sk->sk_lock.wq))
> + wake_up(&sk->sk_lock.wq);
> + spin_unlock_bh(&sk->sk_lock.slock);
> }
> EXPORT_SYMBOL(release_sock);
>

I cannot get this patch to apply cleanly to 2.6.17-mm4.
Since the patch listed in this message covers the same files
as your previous lockdep-annotate-slock.patch, I am assuming
this is supposed to replace it. I should also still apply
lockdep-core-add-set-class-and-name.patch, correct?

patch -p1 -l --dry-run < ../molnar-latest.patch
patching file include/net/sock.h
patching file net/core/sock.c
Hunk #1 FAILED at 134.
Hunk #3 succeeded at 802 with fuzz 2.
1 out of 4 hunks FAILED -- saving rejects to file net/core/sock.c.rej
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-07-02 00:08    [W:0.103 / U:1.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site