* [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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox