On 7/13/23 5:15?AM, Peter Zijlstra wrote:
> On Wed, Jul 12, 2023 at 10:20:13AM -0600, Jens Axboe wrote:
>
>> +int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>> +{
>> + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>> +
>> + if (unlikely(sqe->addr2 || sqe->buf_index || sqe->addr3))
>> + return -EINVAL;
>> +
>> + iof->futex_op = READ_ONCE(sqe->fd);
>> + iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>> + iof->futex_val = READ_ONCE(sqe->len);
>> + iof->futex_mask = READ_ONCE(sqe->file_index);
>> + iof->futex_flags = READ_ONCE(sqe->futex_flags);
>> + if (iof->futex_flags & FUTEX_CMD_MASK)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>
> I'm a little confused on the purpose of iof->futex_op, it doesn't appear
> to be used. Instead iof->futex_flags is used as the ~FUTEX_CMD_MASK part
> of ops.
>
> The latter actually makes sense since you encode the actual op in the
> IOURING_OP_ space.
Yep, I think this is also a leftover from when I had it multiplexed a
bit more. The liburing side got fixed for that, but neglected this bit.
Good catch. I'll fold the below in.
>
>> +int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
>> +{
>> + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
>> + struct io_ring_ctx *ctx = req->ctx;
>> + struct io_futex_data *ifd = NULL;
>> + struct futex_hash_bucket *hb;
>> + unsigned int flags;
>> + int ret;
>> +
>> + if (!iof->futex_mask) {
>> + ret = -EINVAL;
>> + goto done;
>> + }
>> + if (!futex_op_to_flags(FUTEX_WAIT, iof->futex_flags, &flags)) {
>
> A little confusing since you then implement FUTEX_WAIT_BITSET, but using
> FUTEX_WAIT ensures this goes -ENOSYS when setting FUTEX_CLOCK_REALTIME,
> since you handle timeouts through the iouring thing.
>
> Perhaps a comment?
OK, will add a comment on that.
>> + ret = -ENOSYS;
>> + goto done;
>> + }
>> +
>> + io_ring_submit_lock(ctx, issue_flags);
>> + ifd = io_alloc_ifd(ctx);
>> + if (!ifd) {
>> + ret = -ENOMEM;
>> + goto done_unlock;
>> + }
>> +
>> + req->async_data = ifd;
>> + ifd->q = futex_q_init;
>> + ifd->q.bitset = iof->futex_mask;
>> + ifd->q.wake = io_futex_wake_fn;
>> + ifd->req = req;
>> +
>> + ret = futex_wait_setup(iof->uaddr, iof->futex_val, flags, &ifd->q, &hb);
>> + if (!ret) {
>> + hlist_add_head(&req->hash_node, &ctx->futex_list);
>> + io_ring_submit_unlock(ctx, issue_flags);
>> +
>> + futex_queue(&ifd->q, hb);
>> + return IOU_ISSUE_SKIP_COMPLETE;
>> + }
>> +
>> +done_unlock:
>> + io_ring_submit_unlock(ctx, issue_flags);
>> +done:
>> + if (ret < 0)
>> + req_set_fail(req);
>> + io_req_set_res(req, ret, 0);
>> + kfree(ifd);
>> + return IOU_OK;
>> +}
>
> Other than that, I think these things are indeed transparant wrt the
> existing futex interface. If we add a flag this shouldn't care.
Not sure I follow, what kind of flag do you want/need?
diff --git a/io_uring/futex.c b/io_uring/futex.c
index df65b8f3593f..bced11c87896 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -18,7 +18,6 @@ struct io_futex {
u32 __user *uaddr;
struct futex_waitv __user *uwaitv;
};
- int futex_op;
unsigned int futex_val;
unsigned int futex_flags;
unsigned int futex_mask;
@@ -173,10 +172,9 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
{
struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex);
- if (unlikely(sqe->buf_index || sqe->addr3))
+ if (unlikely(sqe->fd || sqe->buf_index || sqe->addr3))
return -EINVAL;
- iof->futex_op = READ_ONCE(sqe->fd);
iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
iof->futex_val = READ_ONCE(sqe->len);
iof->futex_mask = READ_ONCE(sqe->file_index);
--
Jens Axboe
