Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 23 Jun 2020 21:25:07 +0800
From:   Aaron Lu <aaron.lwe@gmail.com>
To:     Peter Oskolkov <posk@posk.io>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Darren Hart <dvhart@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Peter Oskolkov <posk@google.com>, avagin@google.com,
        "pjt@google.com" <pjt@google.com>, Ben Segall <bsegall@google.com>
Subject: Re: [RFC PATCH 1/3 v2] futex: introduce FUTEX_SWAP operation
Message-ID: <20200623132507.GA875@aaronlu-desktop>
References: <cf7c83d0b58aa4912b465392ce8e40a974f250bb.camel@posk.io>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cf7c83d0b58aa4912b465392ce8e40a974f250bb.camel@posk.io>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 16, 2020 at 10:22:26AM -0700, Peter Oskolkov wrote:
>  static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
> -				struct hrtimer_sleeper *timeout)
> +				struct hrtimer_sleeper *timeout,
> +				struct task_struct *next)
>  {
>  	/*
>  	 * The task state is guaranteed to be set before another task can
> @@ -2627,10 +2644,27 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
>  		 * flagged for rescheduling. Only call schedule if there
>  		 * is no timeout, or if it has yet to expire.
>  		 */
> -		if (!timeout || timeout->task)
> +		if (!timeout || timeout->task) {
> +			if (next) {
> +				/*
> +				 * wake_up_process() below will be replaced
> +				 * in the next patch with
> +				 * wake_up_process_prefer_current_cpu().
> +				 */
> +				wake_up_process(next);
> +				put_task_struct(next);
> +				next = NULL;
> +			}

So in futex_swap case, the wake up occurs in futex_wait_queue_me(). I
personally think it's more natural to do the wakeup in futex_swap()
instead.

>  			freezable_schedule();
> +		}
>  	}
>  	__set_current_state(TASK_RUNNING);
> +
> +	if (next) {
> +		/* Maybe call wake_up_process_prefer_current_cpu()? */
> +		wake_up_process(next);
> +		put_task_struct(next);
> +	}
>  }
> 
>  /**

> +static int futex_swap(u32 __user *uaddr, unsigned int flags, u32 val,
> +		      ktime_t *abs_time, u32 __user *uaddr2)
> +{
> +	u32 bitset = FUTEX_BITSET_MATCH_ANY;
> +	struct task_struct *next = NULL;
> +	DEFINE_WAKE_Q(wake_q);
> +	int ret;
> +
> +	ret = prepare_wake_q(uaddr2, flags, 1, bitset, &wake_q);
> +	if (!wake_q_empty(&wake_q)) {
> +		/* Pull the first wakee out of the queue to swap into. */
> +		next = container_of(wake_q.first, struct task_struct, wake_q);
> +		wake_q.first = wake_q.first->next;
> +		next->wake_q.next = NULL;
> +		/*
> +		 * Note that wake_up_q does not touch wake_q.last, so we
> +		 * do not bother with it here.
> +		 */
> +		wake_up_q(&wake_q);

wake_up_q() doesn't seem to serve any purpose in that the above
assignment of wake_q.first shall make it an empty queue now?
Also, I don't see a need to touch wake_q.first either so I think we can
get rid of wake_q altogether here.

> +	}
> +	if (ret < 0)
> +		return ret;
> +
> +	return futex_wait(uaddr, flags, val, abs_time, bitset, next);
> +}

I've cooked the below diff, on top of your patchset. It survived your
self test and schbench. Feel free to ignore it if you don't like it, or
merge it into your patchset if you think it looks better.

do wake up in futex_swap()

---
 kernel/futex.c | 43 +++++++++++--------------------------------
 1 file changed, 11 insertions(+), 32 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index a426671e4bbb..995bc881059c 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2618,8 +2618,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
  *		prefer to execute it locally.
  */
 static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
-				struct hrtimer_sleeper *timeout,
-				struct task_struct *next)
+				struct hrtimer_sleeper *timeout)
 {
 	/*
 	 * The task state is guaranteed to be set before another task can
@@ -2644,22 +2643,11 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
 		 * flagged for rescheduling. Only call schedule if there
 		 * is no timeout, or if it has yet to expire.
 		 */
-		if (!timeout || timeout->task) {
-			if (next) {
-				wake_up_process_prefer_current_cpu(next);
-				put_task_struct(next);
-				next = NULL;
-			}
+		if (!timeout || timeout->task)
 			freezable_schedule();
-		}
 	}
-	__set_current_state(TASK_RUNNING);
 
-	if (next) {
-		/* Maybe call wake_up_process_prefer_current_cpu()? */
-		wake_up_process(next);
-		put_task_struct(next);
-	}
+	__set_current_state(TASK_RUNNING);
 }
 
 /**
@@ -2739,7 +2727,7 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
 }
 
 static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
-		      ktime_t *abs_time, u32 bitset, struct task_struct *next)
+		      ktime_t *abs_time, u32 bitset)
 {
 	struct hrtimer_sleeper timeout, *to;
 	struct restart_block *restart;
@@ -2763,8 +2751,7 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 		goto out;
 
 	/* queue_me and wait for wakeup, timeout, or a signal. */
-	futex_wait_queue_me(hb, &q, to, next);
-	next = NULL;
+	futex_wait_queue_me(hb, &q, to);
 
 	/* If we were woken (and unqueued), we succeeded, whatever. */
 	ret = 0;
@@ -2797,10 +2784,6 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 	ret = -ERESTART_RESTARTBLOCK;
 
 out:
-	if (next) {
-		wake_up_process(next);
-		put_task_struct(next);
-	}
 	if (to) {
 		hrtimer_cancel(&to->timer);
 		destroy_hrtimer_on_stack(&to->timer);
@@ -2820,7 +2803,7 @@ static long futex_wait_restart(struct restart_block *restart)
 	restart->fn = do_no_restart_syscall;
 
 	return (long)futex_wait(uaddr, restart->futex.flags, restart->futex.val,
-				tp, restart->futex.bitset, NULL);
+				tp, restart->futex.bitset);
 }
 
 static int futex_swap(u32 __user *uaddr, unsigned int flags, u32 val,
@@ -2835,18 +2818,14 @@ static int futex_swap(u32 __user *uaddr, unsigned int flags, u32 val,
 	if (!wake_q_empty(&wake_q)) {
 		/* Pull the first wakee out of the queue to swap into. */
 		next = container_of(wake_q.first, struct task_struct, wake_q);
-		wake_q.first = wake_q.first->next;
 		next->wake_q.next = NULL;
-		/*
-		 * Note that wake_up_q does not touch wake_q.last, so we
-		 * do not bother with it here.
-		 */
-		wake_up_q(&wake_q);
+		wake_up_process_prefer_current_cpu(next);
+		put_task_struct(next);
 	}
 	if (ret < 0)
 		return ret;
 
-	return futex_wait(uaddr, flags, val, abs_time, bitset, next);
+	return futex_wait(uaddr, flags, val, abs_time, bitset);
 }
 
 /*
@@ -3333,7 +3312,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 	}
 
 	/* Queue the futex_q, drop the hb lock, wait for wakeup. */
-	futex_wait_queue_me(hb, &q, to, NULL);
+	futex_wait_queue_me(hb, &q, to);
 
 	spin_lock(&hb->lock);
 	ret = handle_early_requeue_pi_wakeup(hb, &q, &key2, to);
@@ -3863,7 +3842,7 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 		val3 = FUTEX_BITSET_MATCH_ANY;
 		/* fall through */
 	case FUTEX_WAIT_BITSET:
-		return futex_wait(uaddr, flags, val, timeout, val3, NULL);
+		return futex_wait(uaddr, flags, val, timeout, val3);
 	case FUTEX_WAKE:
 		val3 = FUTEX_BITSET_MATCH_ANY;
 		/* fall through */
-- 
2.26.2