Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Jul 2023 13:15:13 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Jens Axboe <axboe@kernel.dk>
Cc:     io-uring@vger.kernel.org, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, mingo@redhat.com, andres@anarazel.de
Subject: Re: [PATCH 4/8] io_uring: add support for futex wake and wait
Message-ID: <20230713111513.GH3138667@hirez.programming.kicks-ass.net>
References: <20230712162017.391843-1-axboe@kernel.dk>
 <20230712162017.391843-5-axboe@kernel.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230712162017.391843-5-axboe@kernel.dk>
Precedence: bulk

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.

> +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?

> +		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.