Re: [PATCH] iov_iter: optimise iter type checking
From: Al Viro
Date: Sat Jan 16 2021 - 00:19:04 EST
On Sat, Jan 09, 2021 at 10:11:09PM +0000, Pavel Begunkov wrote:
> > Does any code actually look at the fields as a pair?
> > Would it even be better to use separate bytes?
> > Even growing the on-stack structure by a word won't really matter.
>
> u8 type, rw;
>
> That won't bloat the struct. I like the idea. If used together compilers
> can treat it as u16.
Reasonable, and from what I remember from looking through the users,
no readers will bother with looking at both at the same time.
On the write side... it's only set in iov_iter_{kvec,bvec,pipe,discard,init}.
I sincerely doubt anyone would give a fuck, not to mention that something
like
void iov_iter_pipe(struct iov_iter *i, unsigned int direction,
struct pipe_inode_info *pipe,
size_t count)
{
BUG_ON(direction != READ);
WARN_ON(pipe_full(pipe->head, pipe->tail, pipe->ring_size));
*i = (struct iov_iter) {
.iter_type = ITER_PIPE,
.data_source = false,
.pipe = pipe,
.head = pipe->head,
.start_head = pipe->head,
.count = count,
.iov_offset = 0
};
}
would make more sense anyway - we do want to overwrite everything in the
object, and let the compiler do whatever it likes to do.
So... something like (completely untested) variant below, perhaps?
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 72d88566694e..ed8ad2c5d384 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -19,20 +19,16 @@ struct kvec {
enum iter_type {
/* iter types */
- ITER_IOVEC = 4,
- ITER_KVEC = 8,
- ITER_BVEC = 16,
- ITER_PIPE = 32,
- ITER_DISCARD = 64,
+ ITER_IOVEC,
+ ITER_KVEC,
+ ITER_BVEC,
+ ITER_PIPE,
+ ITER_DISCARD
};
struct iov_iter {
- /*
- * Bit 0 is the read/write bit, set if we're writing.
- * Bit 1 is the BVEC_FLAG_NO_REF bit, set if type is a bvec and
- * the caller isn't expecting to drop a page reference when done.
- */
- unsigned int type;
+ u8 iter_type;
+ bool data_source;
size_t iov_offset;
size_t count;
union {
@@ -52,7 +48,7 @@ struct iov_iter {
static inline enum iter_type iov_iter_type(const struct iov_iter *i)
{
- return i->type & ~(READ | WRITE);
+ return i->iter_type;
}
static inline bool iter_is_iovec(const struct iov_iter *i)
@@ -82,7 +78,7 @@ static inline bool iov_iter_is_discard(const struct iov_iter *i)
static inline unsigned char iov_iter_rw(const struct iov_iter *i)
{
- return i->type & (READ | WRITE);
+ return i->data_source ? WRITE : READ;
}
/*
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 1635111c5bd2..133c03b2dcae 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -81,19 +81,18 @@
#define iterate_all_kinds(i, n, v, I, B, K) { \
if (likely(n)) { \
size_t skip = i->iov_offset; \
- if (unlikely(i->type & ITER_BVEC)) { \
+ if (likely(i->iter_type == ITER_IOVEC)) { \
+ const struct iovec *iov; \
+ struct iovec v; \
+ iterate_iovec(i, n, v, iov, skip, (I)) \
+ } else if (i->iter_type == ITER_BVEC) { \
struct bio_vec v; \
struct bvec_iter __bi; \
iterate_bvec(i, n, v, __bi, skip, (B)) \
- } else if (unlikely(i->type & ITER_KVEC)) { \
+ } else if (i->iter_type == ITER_KVEC) { \
const struct kvec *kvec; \
struct kvec v; \
iterate_kvec(i, n, v, kvec, skip, (K)) \
- } else if (unlikely(i->type & ITER_DISCARD)) { \
- } else { \
- const struct iovec *iov; \
- struct iovec v; \
- iterate_iovec(i, n, v, iov, skip, (I)) \
} \
} \
}
@@ -103,7 +102,17 @@
n = i->count; \
if (i->count) { \
size_t skip = i->iov_offset; \
- if (unlikely(i->type & ITER_BVEC)) { \
+ if (likely(i->iter_type == ITER_IOVEC)) { \
+ const struct iovec *iov; \
+ struct iovec v; \
+ iterate_iovec(i, n, v, iov, skip, (I)) \
+ if (skip == iov->iov_len) { \
+ iov++; \
+ skip = 0; \
+ } \
+ i->nr_segs -= iov - i->iov; \
+ i->iov = iov; \
+ } else if (i->iter_type == ITER_BVEC) { \
const struct bio_vec *bvec = i->bvec; \
struct bio_vec v; \
struct bvec_iter __bi; \
@@ -111,7 +120,7 @@
i->bvec = __bvec_iter_bvec(i->bvec, __bi); \
i->nr_segs -= i->bvec - bvec; \
skip = __bi.bi_bvec_done; \
- } else if (unlikely(i->type & ITER_KVEC)) { \
+ } else if (i->iter_type == ITER_KVEC) { \
const struct kvec *kvec; \
struct kvec v; \
iterate_kvec(i, n, v, kvec, skip, (K)) \
@@ -121,18 +130,8 @@
} \
i->nr_segs -= kvec - i->kvec; \
i->kvec = kvec; \
- } else if (unlikely(i->type & ITER_DISCARD)) { \
+ } else if (i->iter_type == ITER_DISCARD) { \
skip += n; \
- } else { \
- const struct iovec *iov; \
- struct iovec v; \
- iterate_iovec(i, n, v, iov, skip, (I)) \
- if (skip == iov->iov_len) { \
- iov++; \
- skip = 0; \
- } \
- i->nr_segs -= iov - i->iov; \
- i->iov = iov; \
} \
i->count -= n; \
i->iov_offset = skip; \
@@ -434,7 +433,7 @@ int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes)
int err;
struct iovec v;
- if (!(i->type & (ITER_BVEC|ITER_KVEC))) {
+ if (i->iter_type == ITER_IOVEC) {
iterate_iovec(i, bytes, v, iov, skip, ({
err = fault_in_pages_readable(v.iov_base, v.iov_len);
if (unlikely(err))
@@ -450,19 +449,26 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction,
size_t count)
{
WARN_ON(direction & ~(READ | WRITE));
- direction &= READ | WRITE;
/* It will get better. Eventually... */
- if (uaccess_kernel()) {
- i->type = ITER_KVEC | direction;
- i->kvec = (struct kvec *)iov;
- } else {
- i->type = ITER_IOVEC | direction;
- i->iov = iov;
- }
- i->nr_segs = nr_segs;
- i->iov_offset = 0;
- i->count = count;
+ if (uaccess_kernel())
+ *i = (struct iov_iter) {
+ .iter_type = ITER_KVEC,
+ .data_source = direction,
+ .kvec = (struct kvec *)iov,
+ .nr_segs = nr_segs,
+ .iov_offset = 0,
+ .count = count
+ };
+ else
+ *i = (struct iov_iter) {
+ .iter_type = ITER_IOVEC,
+ .data_source = direction,
+ .iov = iov,
+ .nr_segs = nr_segs,
+ .iov_offset = 0,
+ .count = count
+ };
}
EXPORT_SYMBOL(iov_iter_init);
@@ -915,17 +921,20 @@ size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
{
if (unlikely(!page_copy_sane(page, offset, bytes)))
return 0;
- if (i->type & (ITER_BVEC|ITER_KVEC)) {
+ if (likely(i->iter_type == ITER_IOVEC))
+ return copy_page_to_iter_iovec(page, offset, bytes, i);
+ if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {
void *kaddr = kmap_atomic(page);
size_t wanted = copy_to_iter(kaddr + offset, bytes, i);
kunmap_atomic(kaddr);
return wanted;
- } else if (unlikely(iov_iter_is_discard(i)))
- return bytes;
- else if (likely(!iov_iter_is_pipe(i)))
- return copy_page_to_iter_iovec(page, offset, bytes, i);
- else
+ }
+ if (i->iter_type == ITER_PIPE)
return copy_page_to_iter_pipe(page, offset, bytes, i);
+ if (i->iter_type == ITER_DISCARD)
+ return bytes;
+ WARN_ON(1);
+ return 0;
}
EXPORT_SYMBOL(copy_page_to_iter);
@@ -934,17 +943,16 @@ size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
{
if (unlikely(!page_copy_sane(page, offset, bytes)))
return 0;
- if (unlikely(iov_iter_is_pipe(i) || iov_iter_is_discard(i))) {
- WARN_ON(1);
- return 0;
- }
- if (i->type & (ITER_BVEC|ITER_KVEC)) {
+ if (likely(i->iter_type == ITER_IOVEC))
+ return copy_page_from_iter_iovec(page, offset, bytes, i);
+ if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {
void *kaddr = kmap_atomic(page);
size_t wanted = _copy_from_iter(kaddr + offset, bytes, i);
kunmap_atomic(kaddr);
return wanted;
- } else
- return copy_page_from_iter_iovec(page, offset, bytes, i);
+ }
+ WARN_ON(1);
+ return 0;
}
EXPORT_SYMBOL(copy_page_from_iter);
@@ -1172,11 +1180,14 @@ void iov_iter_kvec(struct iov_iter *i, unsigned int direction,
size_t count)
{
WARN_ON(direction & ~(READ | WRITE));
- i->type = ITER_KVEC | (direction & (READ | WRITE));
- i->kvec = kvec;
- i->nr_segs = nr_segs;
- i->iov_offset = 0;
- i->count = count;
+ *i = (struct iov_iter) {
+ .iter_type = ITER_KVEC,
+ .data_source = direction,
+ .kvec = kvec,
+ .nr_segs = nr_segs,
+ .iov_offset = 0,
+ .count = count
+ };
}
EXPORT_SYMBOL(iov_iter_kvec);
@@ -1185,11 +1196,14 @@ void iov_iter_bvec(struct iov_iter *i, unsigned int direction,
size_t count)
{
WARN_ON(direction & ~(READ | WRITE));
- i->type = ITER_BVEC | (direction & (READ | WRITE));
- i->bvec = bvec;
- i->nr_segs = nr_segs;
- i->iov_offset = 0;
- i->count = count;
+ *i = (struct iov_iter) {
+ .iter_type = ITER_BVEC,
+ .data_source = direction,
+ .bvec = bvec,
+ .nr_segs = nr_segs,
+ .iov_offset = 0,
+ .count = count
+ };
}
EXPORT_SYMBOL(iov_iter_bvec);
@@ -1199,12 +1213,15 @@ void iov_iter_pipe(struct iov_iter *i, unsigned int direction,
{
BUG_ON(direction != READ);
WARN_ON(pipe_full(pipe->head, pipe->tail, pipe->ring_size));
- i->type = ITER_PIPE | READ;
- i->pipe = pipe;
- i->head = pipe->head;
- i->iov_offset = 0;
- i->count = count;
- i->start_head = i->head;
+ *i = (struct iov_iter) {
+ .iter_type = ITER_PIPE,
+ .data_source = false,
+ .pipe = pipe,
+ .head = pipe->head,
+ .start_head = pipe->head,
+ .count = count,
+ .iov_offset = 0
+ };
}
EXPORT_SYMBOL(iov_iter_pipe);
@@ -1220,9 +1237,11 @@ EXPORT_SYMBOL(iov_iter_pipe);
void iov_iter_discard(struct iov_iter *i, unsigned int direction, size_t count)
{
BUG_ON(direction != READ);
- i->type = ITER_DISCARD | READ;
- i->count = count;
- i->iov_offset = 0;
+ *i = (struct iov_iter) {
+ .iter_type = ITER_DISCARD,
+ .data_source = false,
+ .count = count,
+ };
}
EXPORT_SYMBOL(iov_iter_discard);