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

---

kernel/futex.c +6 -0

index 3019b92..d901f40 100644
@@ -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.
+ */

kernel/futex.c +2 -1

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

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.

Quoted 6 lines · 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.

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

---

kernel/futex.c +23 -8

index 3019b92..d4f7252 100644
@@ -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;

Quoted 3 lines · Lai Jiangshan

Quoted 2 lines · Darren Hart

On 12/21/2010 01:55 AM, Lai Jiangshan wrote:

Quoted 6 lines

Quoted 4 lines

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?

Quoted 11 lines · Darren Hart

Quoted 9 lines · Lai Jiangshan

Quoted 6 lines · 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.

Fixed, thanks.

Quoted 2 lines

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.applied renamed from result +15 -15

-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

Quoted 6 lines · 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.

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

---

kernel/futex.c +23 -8

index 3019b92..d4f7252 100644
@@ -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;