2014-06-11 20:46:44

by Thomas Gleixner

[permalink] [raw]
Subject: [patch 2/5] futex: Use futex_top_waiter() in lookup_pi_state()

No point in open coding the same function again.

Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/futex.c | 128 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 63 insertions(+), 65 deletions(-)

Index: linux/kernel/futex.c
===================================================================
--- linux.orig/kernel/futex.c
+++ linux/kernel/futex.c
@@ -796,87 +796,85 @@ static int
lookup_pi_state(u32 uval, struct futex_hash_bucket *hb,
union futex_key *key, struct futex_pi_state **ps)
{
+ struct futex_q *match = futex_top_waiter(hb, key);
struct futex_pi_state *pi_state = NULL;
- struct futex_q *this, *next;
struct task_struct *p;
pid_t pid = uval & FUTEX_TID_MASK;

- plist_for_each_entry_safe(this, next, &hb->chain, list) {
- if (match_futex(&this->key, key)) {
+ if (match) {
+ /*
+ * Sanity check the waiter before increasing the
+ * refcount and attaching to it.
+ */
+ pi_state = match->pi_state;
+ /*
+ * Userspace might have messed up non-PI and PI
+ * futexes [3]
+ */
+ if (unlikely(!pi_state))
+ return -EINVAL;
+
+ WARN_ON(!atomic_read(&pi_state->refcount));
+
+ /*
+ * Handle the owner died case:
+ */
+ if (uval & FUTEX_OWNER_DIED) {
/*
- * Sanity check the waiter before increasing
- * the refcount and attaching to it.
+ * exit_pi_state_list sets owner to NULL and
+ * wakes the topmost waiter. The task which
+ * acquires the pi_state->rt_mutex will fixup
+ * owner.
*/
- pi_state = this->pi_state;
- /*
- * Userspace might have messed up non-PI and
- * PI futexes [3]
- */
- if (unlikely(!pi_state))
- return -EINVAL;
-
- WARN_ON(!atomic_read(&pi_state->refcount));
-
- /*
- * Handle the owner died case:
- */
- if (uval & FUTEX_OWNER_DIED) {
- /*
- * exit_pi_state_list sets owner to NULL and
- * wakes the topmost waiter. The task which
- * acquires the pi_state->rt_mutex will fixup
- * owner.
- */
- if (!pi_state->owner) {
- /*
- * No pi state owner, but the user
- * space TID is not 0. Inconsistent
- * state. [5]
- */
- if (pid)
- return -EINVAL;
- /*
- * Take a ref on the state and
- * return. [4]
- */
- goto out_state;
- }
-
+ if (!pi_state->owner) {
/*
- * If TID is 0, then either the dying owner
- * has not yet executed exit_pi_state_list()
- * or some waiter acquired the rtmutex in the
- * pi state, but did not yet fixup the TID in
- * user space.
- *
- * Take a ref on the state and return. [6]
+ * No pi state owner, but the user
+ * space TID is not 0. Inconsistent
+ * state. [5]
*/
- if (!pid)
- goto out_state;
- } else {
+ if (pid)
+ return -EINVAL;
/*
- * If the owner died bit is not set,
- * then the pi_state must have an
- * owner. [7]
+ * Take a ref on the state and
+ * return. [4]
*/
- if (!pi_state->owner)
- return -EINVAL;
+ goto out_state;
}

/*
- * Bail out if user space manipulated the
- * futex value. If pi state exists then the
- * owner TID must be the same as the user
- * space TID. [9/10]
+ * If TID is 0, then either the dying owner
+ * has not yet executed exit_pi_state_list()
+ * or some waiter acquired the rtmutex in the
+ * pi state, but did not yet fixup the TID in
+ * user space.
+ *
+ * Take a ref on the state and return. [6]
*/
- if (pid != task_pid_vnr(pi_state->owner))
+ if (!pid)
+ goto out_state;
+ } else {
+ /*
+ * If the owner died bit is not set,
+ * then the pi_state must have an
+ * owner. [7]
+ */
+ if (!pi_state->owner)
return -EINVAL;
-
- out_state:
- atomic_inc(&pi_state->refcount);
- *ps = pi_state;
- return 0;
}
+
+ /*
+ * Bail out if user space manipulated the
+ * futex value. If pi state exists then the
+ * owner TID must be the same as the user
+ * space TID. [9/10]
+ */
+ if (pid != task_pid_vnr(pi_state->owner))
+ return -EINVAL;
+
+ out_state:
+ atomic_inc(&pi_state->refcount);
+ *ps = pi_state;
+ return 0;
}

/*


2014-06-16 16:43:59

by Darren Hart

[permalink] [raw]
Subject: Re: [patch 2/5] futex: Use futex_top_waiter() in lookup_pi_state()

On Wed, 2014-06-11 at 20:45 +0000, Thomas Gleixner wrote:
> No point in open coding the same function again.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> kernel/futex.c | 128
> ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 63 insertions(+), 65 deletions(-)
>
> Index: linux/kernel/futex.c
> ===================================================================
> --- linux.orig/kernel/futex.c
> +++ linux/kernel/futex.c
> @@ -796,87 +796,85 @@ static int
> lookup_pi_state(u32 uval, struct futex_hash_bucket *hb,
> union futex_key *key, struct futex_pi_state **ps)
> {
> + struct futex_q *match = futex_top_waiter(hb, key);
> struct futex_pi_state *pi_state = NULL;
> - struct futex_q *this, *next;
> struct task_struct *p;
> pid_t pid = uval & FUTEX_TID_MASK;
>
> - plist_for_each_entry_safe(this, next, &hb->chain, list) {
> - if (match_futex(&this->key, key)) {
> + if (match) {
> + /*
> + * Sanity check the waiter before increasing the
> + * refcount and attaching to it.
> + */
> + pi_state = match->pi_state;
> + /*
> + * Userspace might have messed up non-PI and PI
> + * futexes [3]
> + */
> + if (unlikely(!pi_state))
> + return -EINVAL;
> +
> + WARN_ON(!atomic_read(&pi_state->refcount));
> +
> + /*
> + * Handle the owner died case:
> + */
> + if (uval & FUTEX_OWNER_DIED) {
> /*
> - * Sanity check the waiter before increasing
> - * the refcount and attaching to it.
> + * exit_pi_state_list sets owner to NULL and
> + * wakes the topmost waiter. The task which
> + * acquires the pi_state->rt_mutex will fixup
> + * owner.
> */
> - pi_state = this->pi_state;
> - /*
> - * Userspace might have messed up non-PI and
> - * PI futexes [3]
> - */
> - if (unlikely(!pi_state))
> - return -EINVAL;
> -
> - WARN_ON(!atomic_read(&pi_state->refcount));
> -
> - /*
> - * Handle the owner died case:
> - */
> - if (uval & FUTEX_OWNER_DIED) {
> - /*
> - * exit_pi_state_list sets owner to
> NULL and
> - * wakes the topmost waiter. The task
> which
> - * acquires the pi_state->rt_mutex
> will fixup
> - * owner.
> - */
> - if (!pi_state->owner) {
> - /*
> - * No pi state owner, but the
> user
> - * space TID is not 0.
> Inconsistent
> - * state. [5]
> - */
> - if (pid)
> - return -EINVAL;
> - /*
> - * Take a ref on the state and
> - * return. [4]
> - */
> - goto out_state;
> - }
> -
> + if (!pi_state->owner) {
> /*
> - * If TID is 0, then either the dying
> owner
> - * has not yet executed
> exit_pi_state_list()
> - * or some waiter acquired the rtmutex
> in the
> - * pi state, but did not yet fixup the
> TID in
> - * user space.
> - *
> - * Take a ref on the state and return.
> [6]
> + * No pi state owner, but the user
> + * space TID is not 0. Inconsistent
> + * state. [5]
> */
> - if (!pid)
> - goto out_state;
> - } else {
> + if (pid)
> + return -EINVAL;
> /*
> - * If the owner died bit is not set,
> - * then the pi_state must have an
> - * owner. [7]
> + * Take a ref on the state and
> + * return. [4]
> */
> - if (!pi_state->owner)
> - return -EINVAL;
> + goto out_state;
> }
>
> /*
> - * Bail out if user space manipulated the
> - * futex value. If pi state exists then the
> - * owner TID must be the same as the user
> - * space TID. [9/10]
> + * If TID is 0, then either the dying owner
> + * has not yet executed exit_pi_state_list()
> + * or some waiter acquired the rtmutex in the
> + * pi state, but did not yet fixup the TID in
> + * user space.
> + *
> + * Take a ref on the state and return. [6]
> */
> - if (pid != task_pid_vnr(pi_state->owner))
> + if (!pid)
> + goto out_state;
> + } else {
> + /*
> + * If the owner died bit is not set,
> + * then the pi_state must have an
> + * owner. [7]
> + */
> + if (!pi_state->owner)
> return -EINVAL;
> -
> - out_state:
> - atomic_inc(&pi_state->refcount);
> - *ps = pi_state;
> - return 0;
> }
> +
> + /*
> + * Bail out if user space manipulated the
> + * futex value. If pi state exists then the
> + * owner TID must be the same as the user
> + * space TID. [9/10]
> + */
> + if (pid != task_pid_vnr(pi_state->owner))
> + return -EINVAL;
> +
> + out_state:
> + atomic_inc(&pi_state->refcount);
> + *ps = pi_state;
> + return 0;


Surely there is a better tool for viewing such "shift left one tab"
patches that don't make one track three if blocks in parallel in one's
head to ensure they all assemble back to the same thing? What do other
people use? Ouch...

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

--
Darren Hart
Intel Open Source Technology Center

Subject: [tip:locking/core] futex: Use futex_top_waiter() in lookup_pi_state()

Commit-ID: bd1dbcc67cd2c1181e2c01daac51eabf1b964dd8
Gitweb: http://git.kernel.org/tip/bd1dbcc67cd2c1181e2c01daac51eabf1b964dd8
Author: Thomas Gleixner <[email protected]>
AuthorDate: Wed, 11 Jun 2014 20:45:39 +0000
Committer: Thomas Gleixner <[email protected]>
CommitDate: Sat, 21 Jun 2014 22:26:23 +0200

futex: Use futex_top_waiter() in lookup_pi_state()

No point in open coding the same function again.

Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Darren Hart <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/futex.c | 124 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 346d5c2..fff1ed9 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -796,87 +796,85 @@ static int
lookup_pi_state(u32 uval, struct futex_hash_bucket *hb,
union futex_key *key, struct futex_pi_state **ps)
{
+ struct futex_q *match = futex_top_waiter(hb, key);
struct futex_pi_state *pi_state = NULL;
- struct futex_q *this, *next;
struct task_struct *p;
pid_t pid = uval & FUTEX_TID_MASK;

- plist_for_each_entry_safe(this, next, &hb->chain, list) {
- if (match_futex(&this->key, key)) {
- /*
- * Sanity check the waiter before increasing
- * the refcount and attaching to it.
- */
- pi_state = this->pi_state;
- /*
- * Userspace might have messed up non-PI and
- * PI futexes [3]
- */
- if (unlikely(!pi_state))
- return -EINVAL;
+ if (match) {
+ /*
+ * Sanity check the waiter before increasing the
+ * refcount and attaching to it.
+ */
+ pi_state = match->pi_state;
+ /*
+ * Userspace might have messed up non-PI and PI
+ * futexes [3]
+ */
+ if (unlikely(!pi_state))
+ return -EINVAL;

- WARN_ON(!atomic_read(&pi_state->refcount));
+ WARN_ON(!atomic_read(&pi_state->refcount));

+ /*
+ * Handle the owner died case:
+ */
+ if (uval & FUTEX_OWNER_DIED) {
/*
- * Handle the owner died case:
+ * exit_pi_state_list sets owner to NULL and
+ * wakes the topmost waiter. The task which
+ * acquires the pi_state->rt_mutex will fixup
+ * owner.
*/
- if (uval & FUTEX_OWNER_DIED) {
+ if (!pi_state->owner) {
/*
- * exit_pi_state_list sets owner to NULL and
- * wakes the topmost waiter. The task which
- * acquires the pi_state->rt_mutex will fixup
- * owner.
+ * No pi state owner, but the user
+ * space TID is not 0. Inconsistent
+ * state. [5]
*/
- if (!pi_state->owner) {
- /*
- * No pi state owner, but the user
- * space TID is not 0. Inconsistent
- * state. [5]
- */
- if (pid)
- return -EINVAL;
- /*
- * Take a ref on the state and
- * return. [4]
- */
- goto out_state;
- }
-
- /*
- * If TID is 0, then either the dying owner
- * has not yet executed exit_pi_state_list()
- * or some waiter acquired the rtmutex in the
- * pi state, but did not yet fixup the TID in
- * user space.
- *
- * Take a ref on the state and return. [6]
- */
- if (!pid)
- goto out_state;
- } else {
+ if (pid)
+ return -EINVAL;
/*
- * If the owner died bit is not set,
- * then the pi_state must have an
- * owner. [7]
+ * Take a ref on the state and
+ * return. [4]
*/
- if (!pi_state->owner)
- return -EINVAL;
+ goto out_state;
}

/*
- * Bail out if user space manipulated the
- * futex value. If pi state exists then the
- * owner TID must be the same as the user
- * space TID. [9/10]
+ * If TID is 0, then either the dying owner
+ * has not yet executed exit_pi_state_list()
+ * or some waiter acquired the rtmutex in the
+ * pi state, but did not yet fixup the TID in
+ * user space.
+ *
+ * Take a ref on the state and return. [6]
+ */
+ if (!pid)
+ goto out_state;
+ } else {
+ /*
+ * If the owner died bit is not set,
+ * then the pi_state must have an
+ * owner. [7]
*/
- if (pid != task_pid_vnr(pi_state->owner))
+ if (!pi_state->owner)
return -EINVAL;
-
- out_state:
- atomic_inc(&pi_state->refcount);
- *ps = pi_state;
- return 0;
}
+
+ /*
+ * Bail out if user space manipulated the
+ * futex value. If pi state exists then the
+ * owner TID must be the same as the user
+ * space TID. [9/10]
+ */
+ if (pid != task_pid_vnr(pi_state->owner))
+ return -EINVAL;
+
+ out_state:
+ atomic_inc(&pi_state->refcount);
+ *ps = pi_state;
+ return 0;
}

/*