optimise iov_iter

The problem here is that iov_iter_is_*() helpers check types for equality, but all iterate_* helpers do bitwise ands. This confuses a compiler, so even if some cases were handled separately with iov_iter_is_*(), it can't eliminate and skip unreachable branches in following iterate*().

Quoted 5 lines · Pavel Begunkov

The problem here is that iov_iter_is_*() helpers check types for equality, but all iterate_* helpers do bitwise ands. This confuses a compiler, so even if some cases were handled separately with iov_iter_is_*(), it can't eliminate and skip unreachable branches in following iterate*().

I think we need to kill the iov_iter_is_* helpers, renumber to not do the pointless bitmask and just check for equality (might turn into a bunch of nice switch statements actually).

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Pavel Begunkov

The problem here is that iov_iter_is_*() helpers check types for equality, but all iterate_* helpers do bitwise ands. This confuses a compiler, so even if some cases were handled separately with iov_iter_is_*(), it can't eliminate and skip unreachable branches in following iterate*().

I think we need to kill the iov_iter_is_* helpers, renumber to not do the pointless bitmask and just check for equality (might turn into a bunch of nice switch statements actually).

There are uses like below though, and that would also add some overhead on iov_iter_type(), so it's not apparent to me which version would be cleaner/faster in the end. But yeah, we can experiment after landing this patch.

if (type & (ITER_BVEC|ITER_KVEC))

Quoted 12 lines · Pavel Begunkov

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Pavel Begunkov

The problem here is that iov_iter_is_*() helpers check types for equality, but all iterate_* helpers do bitwise ands. This confuses a compiler, so even if some cases were handled separately with iov_iter_is_*(), it can't eliminate and skip unreachable branches in following iterate*().

I think we need to kill the iov_iter_is_* helpers, renumber to not do the pointless bitmask and just check for equality (might turn into a bunch of nice switch statements actually).

Quoted 6 lines

There are uses like below though, and that would also add some overhead on iov_iter_type(), so it's not apparent to me which version would be cleaner/faster in the end. But yeah, we can experiment after landing this patch.

if (type & (ITER_BVEC|ITER_KVEC))

There are exactly 3 such places, and all of them would've been just as well with case ITER_BVEC: case ITER_KVEC: ... in a switch.

Hmm... I wonder which would work better:

enum iter_type {
        ITER_IOVEC = 0,
        ITER_KVEC = 2,
        ITER_BVEC = 4,
        ITER_PIPE = 6,
        ITER_DISCARD = 8,
};

iov_iter_type(iter) (((iter)->type) & ~1)
iov_iter_rw(iter) (((iter)->type) & 1)

or

enum iter_type {
ITER_IOVEC,
ITER_KVEC,
ITER_BVEC,
ITER_PIPE,
ITER_DISCARD,
};
iov_iter_type(iter) (((iter)->type) & (~0U>>1))
// callers of iov_iter_rw() are almost all comparing with explicit READ or WRITE
iov_iter_rw(iter) (((iter)->type) & ~(~0U>>1) ? WRITE : READ)
with places like iov_iter_kvec() doing

	i->type = ITER_KVEC | ((direction == WRITE) ? BIT(31) : 0);

Preferences?

Sent: 13 December 2020 22:33

Quoted 48 lines · Al Viro

Quoted 12 lines · Pavel Begunkov

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Pavel Begunkov

The problem here is that iov_iter_is_*() helpers check types for equality, but all iterate_* helpers do bitwise ands. This confuses a compiler, so even if some cases were handled separately with iov_iter_is_*(), it can't eliminate and skip unreachable branches in following iterate*().

I think we need to kill the iov_iter_is_* helpers, renumber to not do the pointless bitmask and just check for equality (might turn into a bunch of nice switch statements actually).

Quoted 6 lines

There are uses like below though, and that would also add some overhead on iov_iter_type(), so it's not apparent to me which version would be cleaner/faster in the end. But yeah, we can experiment after landing this patch.

if (type & (ITER_BVEC|ITER_KVEC))

There are exactly 3 such places, and all of them would've been just as well with case ITER_BVEC: case ITER_KVEC: ... in a switch.

Hmm... I wonder which would work better:

enum iter_type {
        ITER_IOVEC = 0,
        ITER_KVEC = 2,
        ITER_BVEC = 4,
        ITER_PIPE = 6,
        ITER_DISCARD = 8,
};

iov_iter_type(iter) (((iter)->type) & ~1)
iov_iter_rw(iter) (((iter)->type) & 1)

or

enum iter_type {
ITER_IOVEC,
ITER_KVEC,
ITER_BVEC,
ITER_PIPE,
ITER_DISCARD,
};
iov_iter_type(iter) (((iter)->type) & (~0U>>1))
// callers of iov_iter_rw() are almost all comparing with explicit READ or WRITE
iov_iter_rw(iter) (((iter)->type) & ~(~0U>>1) ? WRITE : READ)
with places like iov_iter_kvec() doing

Quoted 3 lines

	i->type = ITER_KVEC | ((direction == WRITE) ? BIT(31) : 0);

Preferences?

For the bitmask version (with this patch) we have most of iov_iter_type() completely optimised out. E.g. identical

iov_iter_type(i) & ITER_IOVEC <=> iter->type & ITER_IOVEC

It's also nice to have iov_iter_rw() to be just
(type & 1), operations with which can be optimised in a handful of ways.

Unless the compiler would be able to heavily optimise switches, e.g. to out-of-memory/calculation-based jump tables, that I doubt, I'd personally leave it be. Though, not like it should matter much.

From: Pavel Begunkov

Quoted 63 lines

Sent: 13 December 2020 22:33

Quoted 48 lines · Al Viro

Quoted 12 lines · Pavel Begunkov

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Pavel Begunkov

The problem here is that iov_iter_is_*() helpers check types for equality, but all iterate_* helpers do bitwise ands. This confuses a compiler, so even if some cases were handled separately with iov_iter_is_*(), it can't eliminate and skip unreachable branches in following iterate*().

I think we need to kill the iov_iter_is_* helpers, renumber to not do the pointless bitmask and just check for equality (might turn into a bunch of nice switch statements actually).

Quoted 6 lines

There are uses like below though, and that would also add some overhead on iov_iter_type(), so it's not apparent to me which version would be cleaner/faster in the end. But yeah, we can experiment after landing this patch.

if (type & (ITER_BVEC|ITER_KVEC))

There are exactly 3 such places, and all of them would've been just as well with case ITER_BVEC: case ITER_KVEC: ... in a switch.

Hmm... I wonder which would work better:

enum iter_type {
        ITER_IOVEC = 0,
        ITER_KVEC = 2,
        ITER_BVEC = 4,
        ITER_PIPE = 6,
        ITER_DISCARD = 8,
};

iov_iter_type(iter) (((iter)->type) & ~1)
iov_iter_rw(iter) (((iter)->type) & 1)

or

enum iter_type {
ITER_IOVEC,
ITER_KVEC,
ITER_BVEC,
ITER_PIPE,
ITER_DISCARD,
};
iov_iter_type(iter) (((iter)->type) & (~0U>>1))
// callers of iov_iter_rw() are almost all comparing with explicit READ or WRITE
iov_iter_rw(iter) (((iter)->type) & ~(~0U>>1) ? WRITE : READ)
with places like iov_iter_kvec() doing

Quoted 3 lines

	i->type = ITER_KVEC | ((direction == WRITE) ? BIT(31) : 0);

Preferences?

For the bitmask version (with this patch) we have most of iov_iter_type() completely optimised out. E.g. identical

iov_iter_type(i) & ITER_IOVEC <=> iter->type & ITER_IOVEC

It's also nice to have iov_iter_rw() to be just
(type & 1), operations with which can be optimised in a handful of ways.

Quoted 3 lines

Unless the compiler would be able to heavily optimise switches, e.g. to out-of-memory/calculation-based jump tables, that I doubt, I'd personally leave it be. Though, not like it should matter much.

The advantage of the bit-masks is that the 'usual' options can be tested for together. So the code can be (for example):

	if (likely(iter->type & (ITER_IOVEC | ITER_PIPE) {
		if (likely((iter->type & ITER_IOVEC)) {
			... code for iovec
		} else [
			... code for pipe
		}
	} else if (iter->type & ITER_BVEC) {
		... code for bvec
	} else if (iter->type & ITER_KVEC) {
		.. code for kvec
	} else {
		.. must be discard
	}

The first patch optimises iov_iter_npages() for the bvec case, and the second helps code generation to kill unreachable code.

Pavel Begunkov (2):
iov_iter: optimise iov_iter_npages for bvec
iov_iter: optimise iter type checking

 include/linux/uio.h | 10 +++++-----
 lib/iov_iter.c      | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

Quoted 7 lines · Pavel Begunkov

The first patch optimises iov_iter_npages() for the bvec case, and the second helps code generation to kill unreachable code.

Pavel Begunkov (2):
iov_iter: optimise iov_iter_npages for bvec
iov_iter: optimise iter type checking

Quoted 3 lines

 include/linux/uio.h | 10 +++++-----
 lib/iov_iter.c      | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

Nice! Tested this and confirmed both the better code generation, and reduction in overhead in iov_iter_npages().

Reviewed-by Jens Axboe <[email protected]>

Quoted 8 lines · Jens Axboe

Quoted 7 lines · Pavel Begunkov

The first patch optimises iov_iter_npages() for the bvec case, and the second helps code generation to kill unreachable code.

Pavel Begunkov (2):
iov_iter: optimise iov_iter_npages for bvec
iov_iter: optimise iter type checking

Quoted 6 lines

Quoted 3 lines

 include/linux/uio.h | 10 +++++-----
 lib/iov_iter.c      | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

Nice! Tested this and confirmed both the better code generation, and reduction in overhead in iov_iter_npages().

Thanks! Did you find t-put/etc. boost with your setup?

Quoted 13 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Pavel Begunkov

The first patch optimises iov_iter_npages() for the bvec case, and the second helps code generation to kill unreachable code.

Pavel Begunkov (2):
iov_iter: optimise iov_iter_npages for bvec
iov_iter: optimise iter type checking

 include/linux/uio.h | 10 +++++-----
 lib/iov_iter.c      | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

Quoted 4 lines

Quoted 2 lines

Nice! Tested this and confirmed both the better code generation, and reduction in overhead in iov_iter_npages().

Thanks! Did you find t-put/etc. boost with your setup?

Yeah, for this kind of test, if we shave 1% off the stack overhead, that directly yields an increase in peak IOPS. My numbers were close to yours, dropped about 1% of system overhead.