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;
Message-ID: <d29a2ba031ecd96f785a602e703d97e24980ef15.camel@posk.io>
Subject: Re: [RFC PATCH 1/3 v2] futex: introduce FUTEX_SWAP operation
From:   Peter Oskolkov <posk@posk.io>
To:     Aaron Lu <aaron.lwe@gmail.com>
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>
Date:   Tue, 23 Jun 2020 11:30:30 -0700
In-Reply-To: <20200623132507.GA875@aaronlu-desktop>
References: <cf7c83d0b58aa4912b465392ce8e40a974f250bb.camel@posk.io>
         <20200623132507.GA875@aaronlu-desktop>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.2-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Aaron,

thanks a lot for your tests and feedback! My comments below.

Thanks,
Peter

On Tue, 2020-06-23 at 21:25 +0800, Aaron Lu wrote:
> 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.

I chose to do it the way I did so that
wake_up_process_prefer_current_cpu() is called only
when we are reasonably sure that the current task is
going to block/wait, as otherwise it makes no sense
to migrate the wakee/next to the current CPU. In addition, future
optimizations may include actually doing a context switch into
the wakee/next if the current task is going to block instead of
just calling schedule(), which also points to benefits of
having the waking/context-switching code
reside here rather than in futex_swap.

These reasons are somewhat hand-wavy, though, and I agree
that logically it seems that waking in futex_swap() is
easier to think through. Let's see what the maintainers say.

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

The futex at uaddr2 may have more than one waiter, so we cannot assume
that wake_q will be empty when we remove the first element.

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