On 10/9/24 20:01, Jens Axboe wrote:
On 10/9/24 12:51 PM, Pavel Begunkov wrote:
On 10/9/24 19:28, Jens Axboe wrote:
diff --git a/io_uring/net.c b/io_uring/net.c
index d08abcca89cc..482e138d2994 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -1193,6 +1201,76 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
return ret;
}
+int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+ struct io_recvzc *zc = io_kiocb_to_cmd(req, struct io_recvzc);
+ unsigned ifq_idx;
+
+ if (unlikely(sqe->file_index || sqe->addr2 || sqe->addr ||
+ sqe->len || sqe->addr3))
+ return -EINVAL;
+
+ ifq_idx = READ_ONCE(sqe->zcrx_ifq_idx);
+ if (ifq_idx != 0)
+ return -EINVAL;
+ zc->ifq = req->ctx->ifq;
+ if (!zc->ifq)
+ return -EINVAL;
This is read and assigned to 'zc' here, but then the issue handler does
it again? I'm assuming that at some point we'll have ifq selection here,
and then the issue handler will just use zc->ifq. So this part should
probably remain, and the issue side just use zc->ifq?
Yep, fairly overlooked. It's not a real problem, but should
only be fetched and checked here.
Right
+ /* All data completions are posted as aux CQEs. */
+ req->flags |= REQ_F_APOLL_MULTISHOT;
This puzzles me a bit...
Well, it's a multishot request. And that flag protects from cq
locking rules violations, i.e. avoiding multishot reqs from
posting from io-wq.
Maybe make it more like the others and require that
IORING_RECV_MULTISHOT is set then, and set it based on that?
if (IORING_RECV_MULTISHOT)
return -EINVAL;
req->flags |= REQ_F_APOLL_MULTISHOT;
It can be this if that's the preference. It's a bit more consistent,
but might be harder to use. Though I can just hide the flag behind
liburing helpers, would spare from neverending GH issues asking
why it's -EINVAL'ed
+ zc->flags = READ_ONCE(sqe->ioprio);
+ zc->msg_flags = READ_ONCE(sqe->msg_flags);
+ if (zc->msg_flags)
+ return -EINVAL;
Maybe allow MSG_DONTWAIT at least? You already pass that in anyway.
What would the semantics be? The io_uring nowait has always
been a pure mess because it's not even clear what it supposed
to mean for async requests.
Yeah can't disagree with that. Not a big deal, doesn't really matter,
can stay as-is.
I went through the MSG_* flags before looking which ones might
even make sense here and be useful... Let's better enable if
it'd be needed.
+ if (zc->flags & ~(IORING_RECVSEND_POLL_FIRST | IORING_RECV_MULTISHOT))
+ return -EINVAL;
+
+
+#ifdef CONFIG_COMPAT
+ if (req->ctx->compat)
+ zc->msg_flags |= MSG_CMSG_COMPAT;
+#endif
+ return 0;
+}
Heh, we could probably just return -EINVAL for that case, but since this
is all we need, fine.
Well, there is no msghdr, cmsg nor iovec there, so doesn't even
make sense to set it. Can fail as well, I don't anyone would care.
Then let's please just kill it, should not need a check for that then.
--
Pavel Begunkov
