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.043 / U:154.192 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site