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