lkml.org 
[lkml]   [2010]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/4 V2 ] futex,plist: pass the real head of the priority list to plist_del()
    On 12/21/2010 01:55 AM, Lai Jiangshan wrote:

    Hi Lai,

    Looks about ready to me, only a couple of more nitpics from me.

    >
    > Some plist_del()s in kernel/futex.c are passed a faked head of the
    > priority list.
    >
    > It can work because current code does not require the real head

    Awkward, how about:
    It does not fail because the current...

    > in plist_del(). The code of plist_del() just uses the head for checking,
    > so it will not cause bad result even when we use a faked head.

    a bad result

    >
    > But it is an undocumented usage:

    s/an//

    >
    > /**
    > * plist_del - Remove a @node from plist.
    > *
    > * @node: &struct plist_node pointer - entry to be removed
    > * @head: &struct plist_head pointer - list head
    > */
    >
    > The document said that @head is "list head" the head of the priority list.
    >
    > In futex code, several places use "plist_del(&q->list,&q->list.plist);",
    > they passes faked head, we fix them all.
    >
    > Thank to Darren Hart for many suggests.

    s/Thank/Thanks/
    s/suggests/suggestions/

    >
    > Signed-off-by: Lai Jiangshan<laijs@cn.fujitsu.com>
    > ---
    > diff --git a/kernel/futex.c b/kernel/futex.c
    > index 3019b92..d901f40 100644
    > --- a/kernel/futex.c
    > +++ b/kernel/futex.c
    > @@ -740,6 +740,23 @@ retry:
    > return ret;
    > }
    >
    > +/**
    > + * __unqueue_futex() - Remove the futex_q from its futex_hash_bucket
    > + * @q: The futex_q to unqueue
    > + *
    > + * The q->lock_ptr must not be NULL and must be held by the caller.
    > + */
    > +static void __unqueue_futex(struct futex_q *q)
    > +{
    > + struct futex_hash_bucket *hb;
    > +
    > + if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)))
    > + return;
    > +
    > + hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
    > + plist_del(&q->list,&hb->chain);
    > +}

    I like this approach better than the previous version.

    > +
    > /*
    > * The hash bucket lock must be held when this is called.
    > * Afterwards, the futex_q must not be accessed.
    > @@ -757,7 +774,7 @@ static void wake_futex(struct futex_q *q)
    > */
    > get_task_struct(p);
    >
    > - plist_del(&q->list,&q->list.plist);
    > + __unqueue_futex(q);
    > /*
    > * The waiting task can free the futex_q as soon as
    > * q->lock_ptr = NULL is written, without taking any locks. A
    > @@ -1067,7 +1084,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
    > q->key = *key;
    >
    > WARN_ON(plist_node_empty(&q->list));
    > - plist_del(&q->list,&q->list.plist);
    > + __unqueue_futex(q);

    The WARN_ON here is used several times, but there is no longer an
    explicit plist operation to follow. Suggest moving the WARN_ON into
    __unqueue_futex() and keep it together with the plist_del.


    > WARN_ON(!q->rt_waiter);
    > q->rt_waiter = NULL;
    > @@ -1471,7 +1488,7 @@ retry:
    > goto retry;
    > }
    > WARN_ON(plist_node_empty(&q->list));
    > - plist_del(&q->list,&q->list.plist);
    > + __unqueue_futex(q);

    here too

    >
    > BUG_ON(q->pi_state);
    >
    > @@ -1492,7 +1509,7 @@ static void unqueue_me_pi(struct futex_q *q)
    > __releases(q->lock_ptr)
    > {
    > WARN_ON(plist_node_empty(&q->list));
    > - plist_del(&q->list,&q->list.plist);
    > + __unqueue_futex(q);

    and here

    >
    > BUG_ON(!q->pi_state);
    > free_pi_state(q->pi_state);
    > @@ -2133,7 +2150,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
    > * We were woken prior to requeue by a timeout or a signal.
    > * Unqueue the futex_q and determine which it was.
    > */
    > - plist_del(&q->list,&q->list.plist);
    > + plist_del(&q->list,&hb->chain);
    >
    > /* Handle spurious wakeups gracefully */
    > ret = -EWOULDBLOCK;

    Thanks Lai!

    --
    Darren Hart
    Yocto Linux Kernel


    \
     
     \ /
      Last update: 2010-12-21 20:15    [W:0.028 / U:63.688 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site