[PATCHSET v3 0/4] Turn single segment imports into ITER_UBUF

[PATCHSET v3 0/4] Turn single segment imports into ITER_UBUF

[Jens Axboe]

Hi,

Rather than repeat the same blurb again, see the v2 posting here:

https://lore.kernel.org/linux-fsdevel/20230327180449.87382-1-axboe@kernel.dk/

tldr - turn single segment iovecs into ITER_UBUF rather than ITER_IOVEC,
because they are more efficient.

Since v2:

- Add prep patch based on Al's suggestion on just making iov_iter_iovec()
  deal with ITER_UBUF.
- Drop previous two prep patches, as they are no longer needed.

-- 
Jens Axboe

[PATCH 1/3] iov_iter: teach iov_iter_iovec() to deal with ITER_UBUF

[Jens Axboe]

Even if we're returning an iovec, we can trivially fill it in with the
details from an ITER_UBUF as well. This enables loops that assume
ITER_IOVEC to deal with ITER_UBUF transparently.

This is done in preparation for automatically importing single segment
iovecs as ITER_UBUF.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/uio.h | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 27e3fd942960..3b4403efcce1 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -143,13 +143,29 @@ static inline size_t iov_length(const struct iovec *iov, unsigned long nr_segs)
 	return ret;
 }
 
+/*
+ * Don't assume we're called with ITER_IOVEC, enable usage of ITER_UBUF
+ * as well by simply filling in the iovec.
+ */
 static inline struct iovec iov_iter_iovec(const struct iov_iter *iter)
 {
-	return (struct iovec) {
-		.iov_base = iter->iov->iov_base + iter->iov_offset,
-		.iov_len = min(iter->count,
-			       iter->iov->iov_len - iter->iov_offset),
-	};
+	if (WARN_ON_ONCE(!iter->user_backed)) {
+		return (struct iovec) {
+			.iov_base = NULL,
+			.iov_len = 0
+		};
+	} else if (iter_is_ubuf(iter)) {
+		return (struct iovec) {
+			.iov_base = iter->ubuf + iter->iov_offset,
+			.iov_len = iter->count
+		};
+	} else {
+		return (struct iovec) {
+			.iov_base = iter->iov->iov_base + iter->iov_offset,
+			.iov_len = min(iter->count,
+				       iter->iov->iov_len - iter->iov_offset),
+		};
+	}
 }
 
 size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
-- 
2.39.2

[PATCH 2/3] iov_iter: convert import_single_range() to ITER_UBUF

[Jens Axboe]

Since we're just importing a single vector, we don't have to turn it
into an ITER_IOVEC. Instead turn it into an ITER_UBUF, which is cheaper
to iterate.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 lib/iov_iter.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 274014e4eafe..fc82cc42ffe6 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1866,9 +1866,7 @@ int import_single_range(int rw, void __user *buf, size_t len,
 	if (unlikely(!access_ok(buf, len)))
 		return -EFAULT;
 
-	iov->iov_base = buf;
-	iov->iov_len = len;
-	iov_iter_init(i, rw, iov, 1, len);
+	iov_iter_ubuf(i, rw, buf, len);
 	return 0;
 }
 EXPORT_SYMBOL(import_single_range);
-- 
2.39.2

[PATCH 3/3] iov_iter: import single vector iovecs as ITER_UBUF

[Jens Axboe]

Add a special case to __import_iovec(), which imports a single segment
iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
to iterate than ITER_IOVEC, and for a single segment iovec, there's no
point in using a segmented iterator.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 lib/iov_iter.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index fc82cc42ffe6..63cf9997bd50 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1780,6 +1780,30 @@ struct iovec *iovec_from_user(const struct iovec __user *uvec,
 	return iov;
 }
 
+/*
+ * Single segment iovec supplied by the user, import it as ITER_UBUF.
+ */
+static ssize_t __import_iovec_ubuf(int type, const struct iovec __user *uvec,
+				   struct iovec **iovp, struct iov_iter *i,
+				   bool compat)
+{
+	struct iovec *iov = *iovp;
+	ssize_t ret;
+
+	if (compat)
+		ret = copy_compat_iovec_from_user(iov, uvec, 1);
+	else
+		ret = copy_iovec_from_user(iov, uvec, 1);
+	if (unlikely(ret))
+		return ret;
+
+	ret = import_ubuf(type, iov->iov_base, iov->iov_len, i);
+	if (unlikely(ret))
+		return ret;
+	*iovp = NULL;
+	return i->count;
+}
+
 ssize_t __import_iovec(int type, const struct iovec __user *uvec,
 		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
 		 struct iov_iter *i, bool compat)
@@ -1788,6 +1812,9 @@ ssize_t __import_iovec(int type, const struct iovec __user *uvec,
 	unsigned long seg;
 	struct iovec *iov;
 
+	if (nr_segs == 1)
+		return __import_iovec_ubuf(type, uvec, iovp, i, compat);
+
 	iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
 	if (IS_ERR(iov)) {
 		*iovp = NULL;
-- 
2.39.2

Re: [PATCH 3/3] iov_iter: import single vector iovecs as ITER_UBUF

[Al Viro]

On Mon, Mar 27, 2023 at 05:27:13PM -0600, Jens Axboe wrote:
> Add a special case to __import_iovec(), which imports a single segment
> iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
> to iterate than ITER_IOVEC, and for a single segment iovec, there's no
> point in using a segmented iterator.

Won't that enforce the "single-segment readv() is always identical to
read()"?  We'd been through that before - some of infinibarf drvivers
have two different command sets, one reached via read(), another - via
readv().  It's a userland ABI.  Misdesigned one, but that's infinibarf
for you.

Does anyone really need this particular microoptimization?

Re: [PATCH 3/3] iov_iter: import single vector iovecs as ITER_UBUF

[Al Viro]

On Tue, Mar 28, 2023 at 01:08:11AM +0100, Al Viro wrote:
> On Mon, Mar 27, 2023 at 05:27:13PM -0600, Jens Axboe wrote:
> > Add a special case to __import_iovec(), which imports a single segment
> > iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
> > to iterate than ITER_IOVEC, and for a single segment iovec, there's no
> > point in using a segmented iterator.
> 
> Won't that enforce the "single-segment readv() is always identical to
> read()"?  We'd been through that before - some of infinibarf drvivers
> have two different command sets, one reached via read(), another - via
> readv().  It's a userland ABI.  Misdesigned one, but that's infinibarf
> for you.
> 
> Does anyone really need this particular microoptimization?

static ssize_t qib_write_iter(struct kiocb *iocb, struct iov_iter *from)
{
        struct qib_filedata *fp = iocb->ki_filp->private_data;
	struct qib_ctxtdata *rcd = ctxt_fp(iocb->ki_filp);
	struct qib_user_sdma_queue *pq = fp->pq;

	if (!iter_is_iovec(from) || !from->nr_segs || !pq)
		return -EINVAL;

	return qib_user_sdma_writev(rcd, pq, from->iov, from->nr_segs);
}

Hit this with single-segment writev() and you've got yourself -EINVAL.
Sure, that could be adjusted for (check for user_backed_iter(), then
if it's ubuf form an iovec and pass that to qib_user_sdma_writev()),
but that's a clear regression.

Found by simple grepping for iter_is_iovec()...

Re: [PATCH 3/3] iov_iter: import single vector iovecs as ITER_UBUF

[Jens Axboe]

On 3/27/23 6:25 PM, Al Viro wrote:
> On Tue, Mar 28, 2023 at 01:08:11AM +0100, Al Viro wrote:
>> On Mon, Mar 27, 2023 at 05:27:13PM -0600, Jens Axboe wrote:
>>> Add a special case to __import_iovec(), which imports a single segment
>>> iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
>>> to iterate than ITER_IOVEC, and for a single segment iovec, there's no
>>> point in using a segmented iterator.
>>
>> Won't that enforce the "single-segment readv() is always identical to
>> read()"?  We'd been through that before - some of infinibarf drvivers
>> have two different command sets, one reached via read(), another - via
>> readv().  It's a userland ABI.  Misdesigned one, but that's infinibarf
>> for you.
>>
>> Does anyone really need this particular microoptimization?
> 
> static ssize_t qib_write_iter(struct kiocb *iocb, struct iov_iter *from)
> {
>         struct qib_filedata *fp = iocb->ki_filp->private_data;
> 	struct qib_ctxtdata *rcd = ctxt_fp(iocb->ki_filp);
> 	struct qib_user_sdma_queue *pq = fp->pq;
> 
> 	if (!iter_is_iovec(from) || !from->nr_segs || !pq)
> 		return -EINVAL;
> 
> 	return qib_user_sdma_writev(rcd, pq, from->iov, from->nr_segs);
> }
> 
> Hit this with single-segment writev() and you've got yourself -EINVAL.
> Sure, that could be adjusted for (check for user_backed_iter(), then
> if it's ubuf form an iovec and pass that to qib_user_sdma_writev()),
> but that's a clear regression.
> 
> Found by simple grepping for iter_is_iovec()...

Looks like there are a few of those, in fact. Guess we need to do
an iter->user_backed check for these.

-- 
Jens Axboe

Re: [PATCH 3/3] iov_iter: import single vector iovecs as ITER_UBUF

[Jens Axboe]

On 3/27/23 6:08 PM, Al Viro wrote:
> On Mon, Mar 27, 2023 at 05:27:13PM -0600, Jens Axboe wrote:
>> Add a special case to __import_iovec(), which imports a single segment
>> iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
>> to iterate than ITER_IOVEC, and for a single segment iovec, there's no
>> point in using a segmented iterator.
> 
> Won't that enforce the "single-segment readv() is always identical to
> read()"?  We'd been through that before - some of infinibarf drvivers
> have two different command sets, one reached via read(), another - via
> readv().  It's a userland ABI.  Misdesigned one, but that's infinibarf
> for you.

How so? We're not changing how we pick ->read vs ->read_iter, just
how the iterator is setup. Either method should be happy with either
iterator type.

-- 
Jens Axboe