* [PATCH for-next 0/3] sendmsg/recvmsg cleanup
@ 2021-02-05 0:57 Pavel Begunkov
2021-02-05 0:57 ` [PATCH 1/3] io_uring: set msg_name on msg fixup Pavel Begunkov
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 0:57 UTC (permalink / raw)
To: Jens Axboe, io-uring
Reincarnated cleanups of sendmsg/recvmsg managing and copying of iov.
Pavel Begunkov (3):
io_uring: set msg_name on msg fixup
io_uring: clean iov usage for recvmsg buf select
io_uring: refactor sendmsg/recvmsg iov managing
fs/io_uring.c | 68 +++++++++++++++++++++++----------------------------
1 file changed, 30 insertions(+), 38 deletions(-)
--
2.24.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] io_uring: set msg_name on msg fixup
2021-02-05 0:57 [PATCH for-next 0/3] sendmsg/recvmsg cleanup Pavel Begunkov
@ 2021-02-05 0:57 ` Pavel Begunkov
2021-02-05 0:57 ` [PATCH 2/3] io_uring: clean iov usage for recvmsg buf select Pavel Begunkov
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 0:57 UTC (permalink / raw)
To: Jens Axboe, io-uring
io_setup_async_msg() should fully prepare io_async_msghdr, let it also
handle assigning msg_name and don't hand code it in [send,recv]msg().
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/io_uring.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index b740a39110d6..39bc1df9bb64 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4558,6 +4558,7 @@ static int io_setup_async_msg(struct io_kiocb *req,
async_msg = req->async_data;
req->flags |= REQ_F_NEED_CLEANUP;
memcpy(async_msg, kmsg, sizeof(*kmsg));
+ async_msg->msg.msg_name = &async_msg->addr;
return -EAGAIN;
}
@@ -4610,7 +4611,6 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock,
if (req->async_data) {
kmsg = req->async_data;
- kmsg->msg.msg_name = &kmsg->addr;
/* if iov is set, it's allocated already */
if (!kmsg->iov)
kmsg->iov = kmsg->fast_iov;
@@ -4839,7 +4839,6 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
if (req->async_data) {
kmsg = req->async_data;
- kmsg->msg.msg_name = &kmsg->addr;
/* if iov is set, it's allocated already */
if (!kmsg->iov)
kmsg->iov = kmsg->fast_iov;
--
2.24.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] io_uring: clean iov usage for recvmsg buf select
2021-02-05 0:57 [PATCH for-next 0/3] sendmsg/recvmsg cleanup Pavel Begunkov
2021-02-05 0:57 ` [PATCH 1/3] io_uring: set msg_name on msg fixup Pavel Begunkov
@ 2021-02-05 0:57 ` Pavel Begunkov
2021-02-05 0:58 ` [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing Pavel Begunkov
2021-02-05 14:46 ` [PATCH for-next 0/3] sendmsg/recvmsg cleanup Jens Axboe
3 siblings, 0 replies; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 0:57 UTC (permalink / raw)
To: Jens Axboe, io-uring
Don't pretend we don't know that REQ_F_BUFFER_SELECT for recvmsg always
uses fast_iov -- clean up confusing intermixing kmsg->iov and
kmsg->fast_iov for buffer select.
Also don't init iter with garbage in __io_recvmsg_copy_hdr() only for it
to be set shortly after in io_recvmsg().
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/io_uring.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 39bc1df9bb64..e07a7fa15cfa 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4701,11 +4701,9 @@ static int __io_recvmsg_copy_hdr(struct io_kiocb *req,
if (req->flags & REQ_F_BUFFER_SELECT) {
if (iov_len > 1)
return -EINVAL;
- if (copy_from_user(iomsg->iov, uiov, sizeof(*uiov)))
+ if (copy_from_user(iomsg->fast_iov, uiov, sizeof(*uiov)))
return -EFAULT;
- sr->len = iomsg->iov[0].iov_len;
- iov_iter_init(&iomsg->msg.msg_iter, READ, iomsg->iov, 1,
- sr->len);
+ sr->len = iomsg->fast_iov[0].iov_len;
iomsg->iov = NULL;
} else {
ret = __import_iovec(READ, uiov, iov_len, UIO_FASTIOV,
@@ -4748,7 +4746,6 @@ static int __io_compat_recvmsg_copy_hdr(struct io_kiocb *req,
if (clen < 0)
return -EINVAL;
sr->len = clen;
- iomsg->iov[0].iov_len = clen;
iomsg->iov = NULL;
} else {
ret = __import_iovec(READ, (struct iovec __user *)uiov, len,
@@ -4855,7 +4852,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
if (IS_ERR(kbuf))
return PTR_ERR(kbuf);
kmsg->fast_iov[0].iov_base = u64_to_user_ptr(kbuf->addr);
- iov_iter_init(&kmsg->msg.msg_iter, READ, kmsg->iov,
+ kmsg->fast_iov[0].iov_len = req->sr_msg.len;
+ iov_iter_init(&kmsg->msg.msg_iter, READ, kmsg->fast_iov,
1, req->sr_msg.len);
}
--
2.24.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 0:57 [PATCH for-next 0/3] sendmsg/recvmsg cleanup Pavel Begunkov
2021-02-05 0:57 ` [PATCH 1/3] io_uring: set msg_name on msg fixup Pavel Begunkov
2021-02-05 0:57 ` [PATCH 2/3] io_uring: clean iov usage for recvmsg buf select Pavel Begunkov
@ 2021-02-05 0:58 ` Pavel Begunkov
2021-02-05 7:17 ` Stefan Metzmacher
2021-02-05 14:46 ` [PATCH for-next 0/3] sendmsg/recvmsg cleanup Jens Axboe
3 siblings, 1 reply; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 0:58 UTC (permalink / raw)
To: Jens Axboe, io-uring
Current iov handling with recvmsg/sendmsg may be confusing. First make a
rule for msg->iov: either it points to an allocated iov that have to be
kfree()'d later, or it's NULL and we use fast_iov. That's much better
than current 3-state (also can point to fast_iov). And rename it into
free_iov for uniformity with read/write.
Also, instead of after struct io_async_msghdr copy fixing up of
msg.msg_iter.iov has been happening in io_recvmsg()/io_sendmsg(). Move
it into io_setup_async_msg(), that's the right place.
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
fs/io_uring.c | 55 +++++++++++++++++++++++----------------------------
1 file changed, 25 insertions(+), 30 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index e07a7fa15cfa..7008e0c7e05d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -594,7 +594,8 @@ struct io_async_connect {
struct io_async_msghdr {
struct iovec fast_iov[UIO_FASTIOV];
- struct iovec *iov;
+ /* points to an allocated iov, if NULL we use fast_iov instead */
+ struct iovec *free_iov;
struct sockaddr __user *uaddr;
struct msghdr msg;
struct sockaddr_storage addr;
@@ -4551,24 +4552,27 @@ static int io_setup_async_msg(struct io_kiocb *req,
if (async_msg)
return -EAGAIN;
if (io_alloc_async_data(req)) {
- if (kmsg->iov != kmsg->fast_iov)
- kfree(kmsg->iov);
+ kfree(kmsg->free_iov);
return -ENOMEM;
}
async_msg = req->async_data;
req->flags |= REQ_F_NEED_CLEANUP;
memcpy(async_msg, kmsg, sizeof(*kmsg));
async_msg->msg.msg_name = &async_msg->addr;
+ /* if were using fast_iov, set it to the new one */
+ if (!async_msg->free_iov)
+ async_msg->msg.msg_iter.iov = async_msg->fast_iov;
+
return -EAGAIN;
}
static int io_sendmsg_copy_hdr(struct io_kiocb *req,
struct io_async_msghdr *iomsg)
{
- iomsg->iov = iomsg->fast_iov;
iomsg->msg.msg_name = &iomsg->addr;
+ iomsg->free_iov = iomsg->fast_iov;
return sendmsg_copy_msghdr(&iomsg->msg, req->sr_msg.umsg,
- req->sr_msg.msg_flags, &iomsg->iov);
+ req->sr_msg.msg_flags, &iomsg->free_iov);
}
static int io_sendmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
@@ -4609,13 +4613,8 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock,
if (unlikely(!sock))
return -ENOTSOCK;
- if (req->async_data) {
- kmsg = req->async_data;
- /* if iov is set, it's allocated already */
- if (!kmsg->iov)
- kmsg->iov = kmsg->fast_iov;
- kmsg->msg.msg_iter.iov = kmsg->iov;
- } else {
+ kmsg = req->async_data;
+ if (!kmsg) {
ret = io_sendmsg_copy_hdr(req, &iomsg);
if (ret)
return ret;
@@ -4634,8 +4633,8 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock,
if (ret == -ERESTARTSYS)
ret = -EINTR;
- if (kmsg->iov != kmsg->fast_iov)
- kfree(kmsg->iov);
+ if (kmsg->free_iov)
+ kfree(kmsg->free_iov);
req->flags &= ~REQ_F_NEED_CLEANUP;
if (ret < 0)
req_set_fail_links(req);
@@ -4704,10 +4703,11 @@ static int __io_recvmsg_copy_hdr(struct io_kiocb *req,
if (copy_from_user(iomsg->fast_iov, uiov, sizeof(*uiov)))
return -EFAULT;
sr->len = iomsg->fast_iov[0].iov_len;
- iomsg->iov = NULL;
+ iomsg->free_iov = NULL;
} else {
+ iomsg->free_iov = iomsg->fast_iov;
ret = __import_iovec(READ, uiov, iov_len, UIO_FASTIOV,
- &iomsg->iov, &iomsg->msg.msg_iter,
+ &iomsg->free_iov, &iomsg->msg.msg_iter,
false);
if (ret > 0)
ret = 0;
@@ -4746,10 +4746,11 @@ static int __io_compat_recvmsg_copy_hdr(struct io_kiocb *req,
if (clen < 0)
return -EINVAL;
sr->len = clen;
- iomsg->iov = NULL;
+ iomsg->free_iov = NULL;
} else {
+ iomsg->free_iov = iomsg->fast_iov;
ret = __import_iovec(READ, (struct iovec __user *)uiov, len,
- UIO_FASTIOV, &iomsg->iov,
+ UIO_FASTIOV, &iomsg->free_iov,
&iomsg->msg.msg_iter, true);
if (ret < 0)
return ret;
@@ -4763,7 +4764,6 @@ static int io_recvmsg_copy_hdr(struct io_kiocb *req,
struct io_async_msghdr *iomsg)
{
iomsg->msg.msg_name = &iomsg->addr;
- iomsg->iov = iomsg->fast_iov;
#ifdef CONFIG_COMPAT
if (req->ctx->compat)
@@ -4834,13 +4834,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
if (unlikely(!sock))
return -ENOTSOCK;
- if (req->async_data) {
- kmsg = req->async_data;
- /* if iov is set, it's allocated already */
- if (!kmsg->iov)
- kmsg->iov = kmsg->fast_iov;
- kmsg->msg.msg_iter.iov = kmsg->iov;
- } else {
+ kmsg = req->async_data;
+ if (!kmsg) {
ret = io_recvmsg_copy_hdr(req, &iomsg);
if (ret)
return ret;
@@ -4872,8 +4867,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
if (req->flags & REQ_F_BUFFER_SELECTED)
cflags = io_put_recv_kbuf(req);
- if (kmsg->iov != kmsg->fast_iov)
- kfree(kmsg->iov);
+ if (kmsg->free_iov)
+ kfree(kmsg->free_iov);
req->flags &= ~REQ_F_NEED_CLEANUP;
if (ret < 0)
req_set_fail_links(req);
@@ -6166,8 +6161,8 @@ static void __io_clean_op(struct io_kiocb *req)
case IORING_OP_RECVMSG:
case IORING_OP_SENDMSG: {
struct io_async_msghdr *io = req->async_data;
- if (io->iov != io->fast_iov)
- kfree(io->iov);
+
+ kfree(io->free_iov);
break;
}
case IORING_OP_SPLICE:
--
2.24.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 0:58 ` [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing Pavel Begunkov
@ 2021-02-05 7:17 ` Stefan Metzmacher
2021-02-05 9:48 ` Pavel Begunkov
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Metzmacher @ 2021-02-05 7:17 UTC (permalink / raw)
To: Pavel Begunkov, Jens Axboe, io-uring
[-- Attachment #1.1: Type: text/plain, Size: 1781 bytes --]
Hi Pavel,
> static int io_sendmsg_copy_hdr(struct io_kiocb *req,
> struct io_async_msghdr *iomsg)
> {
> - iomsg->iov = iomsg->fast_iov;
> iomsg->msg.msg_name = &iomsg->addr;
> + iomsg->free_iov = iomsg->fast_iov;
Why this? Isn't the idea of this patch that free_iov is never == fast_iov?
> @@ -4704,10 +4703,11 @@ static int __io_recvmsg_copy_hdr(struct io_kiocb *req,
> if (copy_from_user(iomsg->fast_iov, uiov, sizeof(*uiov)))
> return -EFAULT;
> sr->len = iomsg->fast_iov[0].iov_len;
> - iomsg->iov = NULL;
> + iomsg->free_iov = NULL;
> } else {
> + iomsg->free_iov = iomsg->fast_iov;
The same here...
> ret = __import_iovec(READ, uiov, iov_len, UIO_FASTIOV,
> - &iomsg->iov, &iomsg->msg.msg_iter,
> + &iomsg->free_iov, &iomsg->msg.msg_iter,
> false);
> if (ret > 0)
> ret = 0;
> @@ -4746,10 +4746,11 @@ static int __io_compat_recvmsg_copy_hdr(struct io_kiocb *req,
> if (clen < 0)
> return -EINVAL;
> sr->len = clen;
> - iomsg->iov = NULL;
> + iomsg->free_iov = NULL;
> } else {
> + iomsg->free_iov = iomsg->fast_iov;
And here...
> ret = __import_iovec(READ, (struct iovec __user *)uiov, len,
> - UIO_FASTIOV, &iomsg->iov,
> + UIO_FASTIOV, &iomsg->free_iov,
> &iomsg->msg.msg_iter, true);
> if (ret < 0)
> return ret;
> @@ -4872,8 +4867,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
>
> if (req->flags & REQ_F_BUFFER_SELECTED)
> cflags = io_put_recv_kbuf(req);
> - if (kmsg->iov != kmsg->fast_iov)
> - kfree(kmsg->iov);
> + if (kmsg->free_iov)
> + kfree(kmsg->free_iov);
kfree() handles NULL, or is this a hot path and we want to avoid a function call?
metze
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 7:17 ` Stefan Metzmacher
@ 2021-02-05 9:48 ` Pavel Begunkov
2021-02-05 9:58 ` Stefan Metzmacher
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 9:48 UTC (permalink / raw)
To: Stefan Metzmacher, Jens Axboe, io-uring
On 05/02/2021 07:17, Stefan Metzmacher wrote:
> Hi Pavel,
>
>> static int io_sendmsg_copy_hdr(struct io_kiocb *req,
>> struct io_async_msghdr *iomsg)
>> {
>> - iomsg->iov = iomsg->fast_iov;
>> iomsg->msg.msg_name = &iomsg->addr;
>> + iomsg->free_iov = iomsg->fast_iov;
>
> Why this? Isn't the idea of this patch that free_iov is never == fast_iov?
That's a part of __import_iovec() and sendmsg_copy_msghdr() API, you pass
fast_iov as such and get back NULL or a newly allocated one in it.
>
>
>> @@ -4704,10 +4703,11 @@ static int __io_recvmsg_copy_hdr(struct io_kiocb *req,
>> if (copy_from_user(iomsg->fast_iov, uiov, sizeof(*uiov)))
>> return -EFAULT;
>> sr->len = iomsg->fast_iov[0].iov_len;
>> - iomsg->iov = NULL;
>> + iomsg->free_iov = NULL;
>> } else {
>> + iomsg->free_iov = iomsg->fast_iov;
>
> The same here...
>
>> ret = __import_iovec(READ, uiov, iov_len, UIO_FASTIOV,
>> - &iomsg->iov, &iomsg->msg.msg_iter,
>> + &iomsg->free_iov, &iomsg->msg.msg_iter,
>> false);
>> if (ret > 0)
>> ret = 0;
>> @@ -4746,10 +4746,11 @@ static int __io_compat_recvmsg_copy_hdr(struct io_kiocb *req,
>> if (clen < 0)
>> return -EINVAL;
>> sr->len = clen;
>> - iomsg->iov = NULL;
>> + iomsg->free_iov = NULL;
>> } else {
>> + iomsg->free_iov = iomsg->fast_iov;
>
> And here...
>
>> ret = __import_iovec(READ, (struct iovec __user *)uiov, len,
>> - UIO_FASTIOV, &iomsg->iov,
>> + UIO_FASTIOV, &iomsg->free_iov,
>> &iomsg->msg.msg_iter, true);
>> if (ret < 0)
>> return ret;
>
>> @@ -4872,8 +4867,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
>>
>> if (req->flags & REQ_F_BUFFER_SELECTED)
>> cflags = io_put_recv_kbuf(req);
>> - if (kmsg->iov != kmsg->fast_iov)
>> - kfree(kmsg->iov);
>> + if (kmsg->free_iov)
>> + kfree(kmsg->free_iov);
>
> kfree() handles NULL, or is this a hot path and we want to avoid a function call?
Yes, the hot path here is not having iov allocated, and Jens told before
that he had observed overhead for a similar place in io_[read,write].
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 9:48 ` Pavel Begunkov
@ 2021-02-05 9:58 ` Stefan Metzmacher
2021-02-05 10:06 ` Pavel Begunkov
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Metzmacher @ 2021-02-05 9:58 UTC (permalink / raw)
To: Pavel Begunkov, Jens Axboe, io-uring
[-- Attachment #1.1: Type: text/plain, Size: 1293 bytes --]
Hi Pavel,
>>> static int io_sendmsg_copy_hdr(struct io_kiocb *req,
>>> struct io_async_msghdr *iomsg)
>>> {
>>> - iomsg->iov = iomsg->fast_iov;
>>> iomsg->msg.msg_name = &iomsg->addr;
>>> + iomsg->free_iov = iomsg->fast_iov;
>>
>> Why this? Isn't the idea of this patch that free_iov is never == fast_iov?
>
> That's a part of __import_iovec() and sendmsg_copy_msghdr() API, you pass
> fast_iov as such and get back NULL or a newly allocated one in it.
I think that should at least get a comment to make this clear and
maybe a temporary variable like this:
tmp_iov = iomsg->fast_iov;
__import_iovec(..., &tmp_iov, ...);
iomsg->free_iov = tmp_iov;
>>> @@ -4872,8 +4867,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
>>>
>>> if (req->flags & REQ_F_BUFFER_SELECTED)
>>> cflags = io_put_recv_kbuf(req);
>>> - if (kmsg->iov != kmsg->fast_iov)
>>> - kfree(kmsg->iov);
>>> + if (kmsg->free_iov)
>>> + kfree(kmsg->free_iov);
>>
>> kfree() handles NULL, or is this a hot path and we want to avoid a function call?
>
> Yes, the hot path here is not having iov allocated, and Jens told before
> that he had observed overhead for a similar place in io_[read,write].
Ok, a comment would also help here...
metze
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 9:58 ` Stefan Metzmacher
@ 2021-02-05 10:06 ` Pavel Begunkov
2021-02-05 14:45 ` Jens Axboe
0 siblings, 1 reply; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 10:06 UTC (permalink / raw)
To: Stefan Metzmacher, Jens Axboe, io-uring
On 05/02/2021 09:58, Stefan Metzmacher wrote:
> Hi Pavel,
>
>>>> static int io_sendmsg_copy_hdr(struct io_kiocb *req,
>>>> struct io_async_msghdr *iomsg)
>>>> {
>>>> - iomsg->iov = iomsg->fast_iov;
>>>> iomsg->msg.msg_name = &iomsg->addr;
>>>> + iomsg->free_iov = iomsg->fast_iov;
>>>
>>> Why this? Isn't the idea of this patch that free_iov is never == fast_iov?
>>
>> That's a part of __import_iovec() and sendmsg_copy_msghdr() API, you pass
>> fast_iov as such and get back NULL or a newly allocated one in it.
> I think that should at least get a comment to make this clear and
> maybe a temporary variable like this:
>
> tmp_iov = iomsg->fast_iov;
> __import_iovec(..., &tmp_iov, ...);
> iomsg->free_iov = tmp_iov;
I'd rather disagree. It's a well known (ish) API, and I deliberately
placed such assignments right before import_iovec/etc.
We have been using the same pattern has been used for reads/writes and
io_import_iovec() for ages, but seems it haven't got its attention.
>
>>>> @@ -4872,8 +4867,8 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
>>>>
>>>> if (req->flags & REQ_F_BUFFER_SELECTED)
>>>> cflags = io_put_recv_kbuf(req);
>>>> - if (kmsg->iov != kmsg->fast_iov)
>>>> - kfree(kmsg->iov);
>>>> + if (kmsg->free_iov)
>>>> + kfree(kmsg->free_iov);
>>>
>>> kfree() handles NULL, or is this a hot path and we want to avoid a function call?
>>
>> Yes, the hot path here is not having iov allocated, and Jens told before
>> that he had observed overhead for a similar place in io_[read,write].
>
> Ok, a comment would also help here...
>
> metze
>
>
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 10:06 ` Pavel Begunkov
@ 2021-02-05 14:45 ` Jens Axboe
2021-02-05 14:57 ` Pavel Begunkov
0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2021-02-05 14:45 UTC (permalink / raw)
To: Pavel Begunkov, Stefan Metzmacher, io-uring
On 2/5/21 3:06 AM, Pavel Begunkov wrote:
> On 05/02/2021 09:58, Stefan Metzmacher wrote:
>> Hi Pavel,
>>
>>>>> static int io_sendmsg_copy_hdr(struct io_kiocb *req,
>>>>> struct io_async_msghdr *iomsg)
>>>>> {
>>>>> - iomsg->iov = iomsg->fast_iov;
>>>>> iomsg->msg.msg_name = &iomsg->addr;
>>>>> + iomsg->free_iov = iomsg->fast_iov;
>>>>
>>>> Why this? Isn't the idea of this patch that free_iov is never == fast_iov?
>>>
>>> That's a part of __import_iovec() and sendmsg_copy_msghdr() API, you pass
>>> fast_iov as such and get back NULL or a newly allocated one in it.
>> I think that should at least get a comment to make this clear and
>> maybe a temporary variable like this:
>>
>> tmp_iov = iomsg->fast_iov;
>> __import_iovec(..., &tmp_iov, ...);
>> iomsg->free_iov = tmp_iov;
>
> I'd rather disagree. It's a well known (ish) API, and I deliberately
> placed such assignments right before import_iovec/etc.
Agree on that, it's kind of a weird idiom, but it's used throughout the
kernel. However:
>>>> kfree() handles NULL, or is this a hot path and we want to avoid a function call?
>>>
>>> Yes, the hot path here is not having iov allocated, and Jens told before
>>> that he had observed overhead for a similar place in io_[read,write].
>>
>> Ok, a comment would also help here...
I do agree on that one, since otherwise we get patches for it as has
been proven by the few other spots... I'll add then when queueing this
up.
--
Jens Axboe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-next 0/3] sendmsg/recvmsg cleanup
2021-02-05 0:57 [PATCH for-next 0/3] sendmsg/recvmsg cleanup Pavel Begunkov
` (2 preceding siblings ...)
2021-02-05 0:58 ` [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing Pavel Begunkov
@ 2021-02-05 14:46 ` Jens Axboe
3 siblings, 0 replies; 11+ messages in thread
From: Jens Axboe @ 2021-02-05 14:46 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
On 2/4/21 5:57 PM, Pavel Begunkov wrote:
> Reincarnated cleanups of sendmsg/recvmsg managing and copying of iov.
>
> Pavel Begunkov (3):
> io_uring: set msg_name on msg fixup
> io_uring: clean iov usage for recvmsg buf select
> io_uring: refactor sendmsg/recvmsg iov managing
>
> fs/io_uring.c | 68 +++++++++++++++++++++++----------------------------
> 1 file changed, 30 insertions(+), 38 deletions(-)
Applied, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing
2021-02-05 14:45 ` Jens Axboe
@ 2021-02-05 14:57 ` Pavel Begunkov
0 siblings, 0 replies; 11+ messages in thread
From: Pavel Begunkov @ 2021-02-05 14:57 UTC (permalink / raw)
To: Jens Axboe, Stefan Metzmacher, io-uring
On 05/02/2021 14:45, Jens Axboe wrote:
> On 2/5/21 3:06 AM, Pavel Begunkov wrote:
>> On 05/02/2021 09:58, Stefan Metzmacher wrote:
>>> Hi Pavel,
>>>
>>>>>> static int io_sendmsg_copy_hdr(struct io_kiocb *req,
>>>>>> struct io_async_msghdr *iomsg)
>>>>>> {
>>>>>> - iomsg->iov = iomsg->fast_iov;
>>>>>> iomsg->msg.msg_name = &iomsg->addr;
>>>>>> + iomsg->free_iov = iomsg->fast_iov;
>>>>>
>>>>> Why this? Isn't the idea of this patch that free_iov is never == fast_iov?
>>>>
>>>> That's a part of __import_iovec() and sendmsg_copy_msghdr() API, you pass
>>>> fast_iov as such and get back NULL or a newly allocated one in it.
>>> I think that should at least get a comment to make this clear and
>>> maybe a temporary variable like this:
>>>
>>> tmp_iov = iomsg->fast_iov;
>>> __import_iovec(..., &tmp_iov, ...);
>>> iomsg->free_iov = tmp_iov;
>>
>> I'd rather disagree. It's a well known (ish) API, and I deliberately
>> placed such assignments right before import_iovec/etc.
>
> Agree on that, it's kind of a weird idiom, but it's used throughout the
> kernel. However:
Maybe someday someone will refactor it and make it accepting inline_vecs
directly...
>
>>>>> kfree() handles NULL, or is this a hot path and we want to avoid a function call?
>>>>
>>>> Yes, the hot path here is not having iov allocated, and Jens told before
>>>> that he had observed overhead for a similar place in io_[read,write].
>>>
>>> Ok, a comment would also help here...
>
> I do agree on that one, since otherwise we get patches for it as has
> been proven by the few other spots... I'll add then when queueing this
> up.
I didn't disagree with that, just forgot to throw one to protect ourselves
from static analysis tools. Thanks for fixing up
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-05 23:23 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 0:57 [PATCH for-next 0/3] sendmsg/recvmsg cleanup Pavel Begunkov
2021-02-05 0:57 ` [PATCH 1/3] io_uring: set msg_name on msg fixup Pavel Begunkov
2021-02-05 0:57 ` [PATCH 2/3] io_uring: clean iov usage for recvmsg buf select Pavel Begunkov
2021-02-05 0:58 ` [PATCH 3/3] io_uring: refactor sendmsg/recvmsg iov managing Pavel Begunkov
2021-02-05 7:17 ` Stefan Metzmacher
2021-02-05 9:48 ` Pavel Begunkov
2021-02-05 9:58 ` Stefan Metzmacher
2021-02-05 10:06 ` Pavel Begunkov
2021-02-05 14:45 ` Jens Axboe
2021-02-05 14:57 ` Pavel Begunkov
2021-02-05 14:46 ` [PATCH for-next 0/3] sendmsg/recvmsg cleanup Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.