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

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

[Lai Jiangshan]

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
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.

But it is an undocumented usage:

/**
 * 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.

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);
+}
+
 /*
  * 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);
 
 	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);
 
 		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);
 
 	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;

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

[Darren Hart]

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

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

[Lai Jiangshan]

On 12/22/2010 03:13 AM, Darren Hart wrote:
> 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.
> 

Fixed, thanks.

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

Some plist_del()s in kernel/futex.c are passed a faked head of the
priority list.

It does not fail because the current code does not require the real head
in plist_del(). The current code of plist_del() just uses the head for checking,
so it will not cause a bad result even when we use a faked head.

But it is undocumented usage:

/**
 * 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.

Thanks to Darren Hart for many suggestions.

Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/futex.c b/kernel/futex.c
index 3019b92..d4f7252 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -740,6 +740,24 @@ 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)
+			|| plist_node_empty(&q->list)))
+		return;
+
+	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
+	plist_del(&q->list, &hb->chain);
+}
+
 /*
  * The hash bucket lock must be held when this is called.
  * Afterwards, the futex_q must not be accessed.
@@ -757,7 +775,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
@@ -1066,8 +1084,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
 	get_futex_key_refs(key);
 	q->key = *key;
 
-	WARN_ON(plist_node_empty(&q->list));
-	plist_del(&q->list, &q->list.plist);
+	__unqueue_futex(q);
 
 	WARN_ON(!q->rt_waiter);
 	q->rt_waiter = NULL;
@@ -1470,8 +1487,7 @@ retry:
 			spin_unlock(lock_ptr);
 			goto retry;
 		}
-		WARN_ON(plist_node_empty(&q->list));
-		plist_del(&q->list, &q->list.plist);
+		__unqueue_futex(q);
 
 		BUG_ON(q->pi_state);
 
@@ -1491,8 +1507,7 @@ retry:
 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);
 
 	BUG_ON(!q->pi_state);
 	free_pi_state(q->pi_state);
@@ -2133,7 +2148,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;

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

[Darren Hart]

On 12/21/2010 10:18 PM, Lai Jiangshan wrote:
> On 12/22/2010 03:13 AM, Darren Hart wrote:
>> 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.
>>
>
> Fixed, thanks.

Looks good to me. How did the futextest runs go?

--
Darren

>
> Subject: [PATCH 1/4 V3 ] futex,plist: pass the real head of the priority list to plist_del()
>
> Some plist_del()s in kernel/futex.c are passed a faked head of the
> priority list.
>
> It does not fail because the current code does not require the real head
> in plist_del(). The current code of plist_del() just uses the head for checking,
> so it will not cause a bad result even when we use a faked head.
>
> But it is undocumented usage:
>
> /**
>   * 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.
>
> Thanks to Darren Hart for many suggestions.
>
> Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>
> ---
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 3019b92..d4f7252 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -740,6 +740,24 @@ 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)
> +			|| plist_node_empty(&q->list)))
> +		return;
> +
> +	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
> +	plist_del(&q->list,&hb->chain);
> +}
> +
>   /*
>    * The hash bucket lock must be held when this is called.
>    * Afterwards, the futex_q must not be accessed.
> @@ -757,7 +775,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
> @@ -1066,8 +1084,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
>   	get_futex_key_refs(key);
>   	q->key = *key;
>
> -	WARN_ON(plist_node_empty(&q->list));
> -	plist_del(&q->list,&q->list.plist);
> +	__unqueue_futex(q);
>
>   	WARN_ON(!q->rt_waiter);
>   	q->rt_waiter = NULL;
> @@ -1470,8 +1487,7 @@ retry:
>   			spin_unlock(lock_ptr);
>   			goto retry;
>   		}
> -		WARN_ON(plist_node_empty(&q->list));
> -		plist_del(&q->list,&q->list.plist);
> +		__unqueue_futex(q);
>
>   		BUG_ON(q->pi_state);
>
> @@ -1491,8 +1507,7 @@ retry:
>   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);
>
>   	BUG_ON(!q->pi_state);
>   	free_pi_state(q->pi_state);
> @@ -2133,7 +2148,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;


-- 
Darren Hart
Yocto Linux Kernel

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

[Lai Jiangshan]

On 12/22/2010 02:50 PM, Darren Hart wrote:
> On 12/21/2010 10:18 PM, Lai Jiangshan wrote:
>> On 12/22/2010 03:13 AM, Darren Hart wrote:
>>> 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.
>>>
>>
>> Fixed, thanks.
> 
> Looks good to me. How did the futextest runs go?
> 

It's OK, the result are almost the same.

kernel version: 2.6.37-rc6-tip
Intel(R) Pentium(R) D CPU 2.80GHz
MemTotal:         961208 kB

# diff result result.applied -u | grep '^[+-]'
--- result	2010-12-22 16:57:45.000000000 +0800
+++ result.applied	2010-12-22 16:56:39.000000000 +0800
-Result: 12048 Kiter/s
+Result: 12210 Kiter/s
-Result: 4037 Kiter/s
+Result: 3554 Kiter/s
-Result: 3331 Kiter/s
+Result: 3347 Kiter/s
-Result: 2996 Kiter/s
+Result: 3124 Kiter/s
-Result: 2998 Kiter/s
+Result: 3087 Kiter/s
-Result: 2894 Kiter/s
+Result: 3008 Kiter/s
-Result: 2863 Kiter/s
+Result: 2882 Kiter/s
-Result: 3098 Kiter/s
+Result: 3265 Kiter/s
-Result: 2888 Kiter/s
+Result: 2811 Kiter/s
-Result: 2876 Kiter/s
+Result: 2995 Kiter/s
-Result: 2893 Kiter/s
+Result: 2980 Kiter/s
-Result: 3100 Kiter/s
+Result: 2860 Kiter/s
-Result: 2915 Kiter/s
+Result: 2964 Kiter/s
-Result: 2940 Kiter/s
+Result: 3035 Kiter/s
-Result: 2671 Kiter/s
+Result: 2939 Kiter/s

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

[Darren Hart]

On 12/22/2010 01:02 AM, Lai Jiangshan wrote:
> On 12/22/2010 02:50 PM, Darren Hart wrote:
>> On 12/21/2010 10:18 PM, Lai Jiangshan wrote:
>>> On 12/22/2010 03:13 AM, Darren Hart wrote:
>>>> 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.
>>>>
>>>
>>> Fixed, thanks.
>>
>> Looks good to me. How did the futextest runs go?
>>
>
> It's OK, the result are almost the same.
>
> kernel version: 2.6.37-rc6-tip
> Intel(R) Pentium(R) D CPU 2.80GHz
> MemTotal:         961208 kB
>

This is futex_wait.c I presume? Delta looks to be in the realm of 
statistical noise to me. I'll ack the two patches.

--
Darren

> # diff result result.applied -u | grep '^[+-]'
> --- result	2010-12-22 16:57:45.000000000 +0800
> +++ result.applied	2010-12-22 16:56:39.000000000 +0800
> -Result: 12048 Kiter/s
> +Result: 12210 Kiter/s
> -Result: 4037 Kiter/s
> +Result: 3554 Kiter/s
> -Result: 3331 Kiter/s
> +Result: 3347 Kiter/s
> -Result: 2996 Kiter/s
> +Result: 3124 Kiter/s
> -Result: 2998 Kiter/s
> +Result: 3087 Kiter/s
> -Result: 2894 Kiter/s
> +Result: 3008 Kiter/s
> -Result: 2863 Kiter/s
> +Result: 2882 Kiter/s
> -Result: 3098 Kiter/s
> +Result: 3265 Kiter/s
> -Result: 2888 Kiter/s
> +Result: 2811 Kiter/s
> -Result: 2876 Kiter/s
> +Result: 2995 Kiter/s
> -Result: 2893 Kiter/s
> +Result: 2980 Kiter/s
> -Result: 3100 Kiter/s
> +Result: 2860 Kiter/s
> -Result: 2915 Kiter/s
> +Result: 2964 Kiter/s
> -Result: 2940 Kiter/s
> +Result: 3035 Kiter/s
> -Result: 2671 Kiter/s
> +Result: 2939 Kiter/s


-- 
Darren Hart
Yocto Linux Kernel

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

[Darren Hart]

On 12/21/2010 10:18 PM, Lai Jiangshan wrote:
> On 12/22/2010 03:13 AM, Darren Hart wrote:
>> 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.
>>
>
> Fixed, thanks.
>
> Subject: [PATCH 1/4 V3 ] futex,plist: pass the real head of the priority list to plist_del()
>
> Some plist_del()s in kernel/futex.c are passed a faked head of the
> priority list.
>
> It does not fail because the current code does not require the real head
> in plist_del(). The current code of plist_del() just uses the head for checking,
> so it will not cause a bad result even when we use a faked head.
>
> But it is undocumented usage:
>
> /**
>   * 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.
>
> Thanks to Darren Hart for many suggestions.
>
> Signed-off-by:  Lai Jiangshan<laijs@cn.fujitsu.com>

Acked-by: Darren Hart <dvhart@linux.intel.com>

> ---
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 3019b92..d4f7252 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -740,6 +740,24 @@ 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)
> +			|| plist_node_empty(&q->list)))
> +		return;
> +
> +	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
> +	plist_del(&q->list,&hb->chain);
> +}
> +
>   /*
>    * The hash bucket lock must be held when this is called.
>    * Afterwards, the futex_q must not be accessed.
> @@ -757,7 +775,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
> @@ -1066,8 +1084,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
>   	get_futex_key_refs(key);
>   	q->key = *key;
>
> -	WARN_ON(plist_node_empty(&q->list));
> -	plist_del(&q->list,&q->list.plist);
> +	__unqueue_futex(q);
>
>   	WARN_ON(!q->rt_waiter);
>   	q->rt_waiter = NULL;
> @@ -1470,8 +1487,7 @@ retry:
>   			spin_unlock(lock_ptr);
>   			goto retry;
>   		}
> -		WARN_ON(plist_node_empty(&q->list));
> -		plist_del(&q->list,&q->list.plist);
> +		__unqueue_futex(q);
>
>   		BUG_ON(q->pi_state);
>
> @@ -1491,8 +1507,7 @@ retry:
>   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);
>
>   	BUG_ON(!q->pi_state);
>   	free_pi_state(q->pi_state);
> @@ -2133,7 +2148,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;


-- 
Darren Hart
Yocto Linux Kernel

[tip:core/futexes] futex,plist: Pass the real head of the priority list to plist_del()

[tip-bot for Lai Jiangshan]

Commit-ID:  2e12978a9f7a7abd54e8eb9ce70a7718767b8b2c
Gitweb:     http://git.kernel.org/tip/2e12978a9f7a7abd54e8eb9ce70a7718767b8b2c
Author:     Lai Jiangshan <laijs@cn.fujitsu.com>
AuthorDate: Wed, 22 Dec 2010 14:18:50 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 11 Mar 2011 15:09:52 -0500

futex,plist: Pass the real head of the priority list to plist_del()

Some plist_del()s in kernel/futex.c are passed a faked head of the
priority list.

It does not fail because the current code does not require the real head
in plist_del(). The current code of plist_del() just uses the head for checking,
so it will not cause a bad result even when we use a faked head.

But it is undocumented usage:

/**
 * 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 says that the @head is the "list head" head of the priority list.

In futex code, several places use "plist_del(&q->list, &q->list.plist);",
they pass a fake head. We need to fix them all.

Thanks to Darren Hart for many suggestions.

Acked-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
LKML-Reference: <4D11984A.5030203@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/futex.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index b766d28..6feeea4 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -775,6 +775,24 @@ 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)
+			|| plist_node_empty(&q->list)))
+		return;
+
+	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
+	plist_del(&q->list, &hb->chain);
+}
+
 /*
  * The hash bucket lock must be held when this is called.
  * Afterwards, the futex_q must not be accessed.
@@ -792,7 +810,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
@@ -1100,8 +1118,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
 	get_futex_key_refs(key);
 	q->key = *key;
 
-	WARN_ON(plist_node_empty(&q->list));
-	plist_del(&q->list, &q->list.plist);
+	__unqueue_futex(q);
 
 	WARN_ON(!q->rt_waiter);
 	q->rt_waiter = NULL;
@@ -1504,8 +1521,7 @@ retry:
 			spin_unlock(lock_ptr);
 			goto retry;
 		}
-		WARN_ON(plist_node_empty(&q->list));
-		plist_del(&q->list, &q->list.plist);
+		__unqueue_futex(q);
 
 		BUG_ON(q->pi_state);
 
@@ -1525,8 +1541,7 @@ retry:
 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);
 
 	BUG_ON(!q->pi_state);
 	free_pi_state(q->pi_state);
@@ -2167,7 +2182,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;