2010-12-21 09:53:41

by Lai Jiangshan

[permalink] [raw]
Subject: [PATCH 1/4 V2 ] 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 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 <[email protected]>
---
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;


2010-12-21 19:13:05

by Darren Hart

[permalink] [raw]
Subject: Re: [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<[email protected]>
> ---
> 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

2010-12-22 06:17:19

by Lai Jiangshan

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

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 <[email protected]>
---
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;

2010-12-22 06:50:11

by Darren Hart

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

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<[email protected]>
> ---
> 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

2010-12-22 09:00:54

by Lai Jiangshan

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

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

2010-12-22 16:23:22

by Darren Hart

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

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<[email protected]>

Acked-by: Darren Hart <[email protected]>

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

2010-12-22 16:22:45

by Darren Hart

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

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

2011-03-12 10:58:18

by Lai Jiangshan

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

Commit-ID: 2e12978a9f7a7abd54e8eb9ce70a7718767b8b2c
Gitweb: http://git.kernel.org/tip/2e12978a9f7a7abd54e8eb9ce70a7718767b8b2c
Author: Lai Jiangshan <[email protected]>
AuthorDate: Wed, 22 Dec 2010 14:18:50 +0800
Committer: Steven Rostedt <[email protected]>
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 <[email protected]>
Signed-off-by: Lai Jiangshan <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
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;