Re: [PATCH 4/8] io_uring: add support for futex wake and wait

[Jens Axboe <axboe@kernel.dk>]

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