| Message ID | 20230712004705.316157-6-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add io_uring futex/futexv support | expand |
On Tue, Jul 11, 2023 at 06:47:03PM -0600, Jens Axboe wrote: > Since we now provide a way to pass in a wake handler and data, ensure we > use __futex_queue() to avoid having futex_queue() overwrite our wait > data. > diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c > index 3471af87cb7d..dfd02ca5ecfa 100644 > --- a/kernel/futex/waitwake.c > +++ b/kernel/futex/waitwake.c > @@ -446,7 +446,8 @@ static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *wo > * next futex. Queue each futex at this moment so hb can > * be unlocked. > */ > - futex_queue(q, hb); > + __futex_queue(q, hb); > + spin_unlock(&hb->lock); > continue; > } I'm not following; I even applied all your patches up to this point, but futex_queue() still reads: static inline void futex_queue(struct futex_q *q, struct futex_hash_bucket *hb) __releases(&hb->lock) { __futex_queue(q, hb); spin_unlock(&hb->lock); } How would it be different and overwrite anything ?!?
On 7/12/23 3:25?AM, Peter Zijlstra wrote: > On Tue, Jul 11, 2023 at 06:47:03PM -0600, Jens Axboe wrote: > >> Since we now provide a way to pass in a wake handler and data, ensure we >> use __futex_queue() to avoid having futex_queue() overwrite our wait >> data. > >> diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c >> index 3471af87cb7d..dfd02ca5ecfa 100644 >> --- a/kernel/futex/waitwake.c >> +++ b/kernel/futex/waitwake.c >> @@ -446,7 +446,8 @@ static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *wo >> * next futex. Queue each futex at this moment so hb can >> * be unlocked. >> */ >> - futex_queue(q, hb); >> + __futex_queue(q, hb); >> + spin_unlock(&hb->lock); >> continue; >> } > > I'm not following; I even applied all your patches up to this point, but > futex_queue() still reads: > > static inline void futex_queue(struct futex_q *q, struct futex_hash_bucket *hb) > __releases(&hb->lock) > { > __futex_queue(q, hb); > spin_unlock(&hb->lock); > } > > How would it be different and overwrite anything ?!? Good catch, this is a leftover from storing the task/wakeup data separately. But I got rid of that, so it's stale comment at this point and we can certainly use futex_queue() here again and drop this hunk. Will make that edit.
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h index 75dec2ec7469..ed5a7ccd2e99 100644 --- a/kernel/futex/futex.h +++ b/kernel/futex/futex.h @@ -284,6 +284,11 @@ struct futex_vector { struct futex_q q; }; +extern int futex_parse_waitv(struct futex_vector *futexv, + struct futex_waitv __user *uwaitv, + unsigned int nr_futexes, futex_wake_fn *wake, + void *wake_data); + extern int futex_wait_multiple(struct futex_vector *vs, unsigned int count, struct hrtimer_sleeper *to); diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c index 75ca8c41cc94..8ac70bfb89fc 100644 --- a/kernel/futex/syscalls.c +++ b/kernel/futex/syscalls.c @@ -184,12 +184,15 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val, * @futexv: Kernel side list of waiters to be filled * @uwaitv: Userspace list to be parsed * @nr_futexes: Length of futexv + * @wake: Wake to call when futex is woken + * @wake_data: Data for the wake handler * * Return: Error code on failure, 0 on success */ -static int futex_parse_waitv(struct futex_vector *futexv, - struct futex_waitv __user *uwaitv, - unsigned int nr_futexes) +int futex_parse_waitv(struct futex_vector *futexv, + struct futex_waitv __user *uwaitv, + unsigned int nr_futexes, futex_wake_fn *wake, + void *wake_data) { struct futex_waitv aux; unsigned int i; @@ -208,6 +211,8 @@ static int futex_parse_waitv(struct futex_vector *futexv, futexv[i].w.val = aux.val; futexv[i].w.uaddr = aux.uaddr; futexv[i].q = futex_q_init; + futexv[i].q.wake = wake; + futexv[i].q.wake_data = wake_data; } return 0; @@ -284,7 +289,8 @@ SYSCALL_DEFINE5(futex_waitv, struct futex_waitv __user *, waiters, goto destroy_timer; } - ret = futex_parse_waitv(futexv, waiters, nr_futexes); + ret = futex_parse_waitv(futexv, waiters, nr_futexes, futex_wake_mark, + NULL); if (!ret) ret = futex_wait_multiple(futexv, nr_futexes, timeout ? &to : NULL); diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c index 3471af87cb7d..dfd02ca5ecfa 100644 --- a/kernel/futex/waitwake.c +++ b/kernel/futex/waitwake.c @@ -446,7 +446,8 @@ static int futex_wait_multiple_setup(struct futex_vector *vs, int count, int *wo * next futex. Queue each futex at this moment so hb can * be unlocked. */ - futex_queue(q, hb); + __futex_queue(q, hb); + spin_unlock(&hb->lock); continue; }
To make it more generically useful, augment it with allowing the caller to pass in the wake handler and wake data. Convert the futex_waitv() syscall, passing in the default handlers. Since we now provide a way to pass in a wake handler and data, ensure we use __futex_queue() to avoid having futex_queue() overwrite our wait data. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- kernel/futex/futex.h | 5 +++++ kernel/futex/syscalls.c | 14 ++++++++++---- kernel/futex/waitwake.c | 3 ++- 3 files changed, 17 insertions(+), 5 deletions(-)