Messages in this thread | | | Subject | Re: [RFC PATCH 1/3 v2] futex: introduce FUTEX_SWAP operation | From | Peter Oskolkov <> | Date | Tue, 23 Jun 2020 11:30:30 -0700 |
| |
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 */
| |