Re: [PATCH 1/4 V2 ] futex,plist: pass the real head of the priority list to plist_del()

[Darren Hart <dvhart@linux.intel.com>]

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