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(-)
--
2.24.0
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*().
E.g. iov_iter_npages() first handles bvecs and discards, but
iterate_all_kinds() still generate branches for them.
Making checks consistent solves that.
size lib/iov_iter.o
before:
text data bss dec hex filename
24409 805 0 25214 627e lib/iov_iter.o
after:
text data bss dec hex filename
23785 793 0 24578 6002 lib/iov_iter.o
Signed-off-by: Pavel Begunkov <[email protected]>
---
include/linux/uio.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 72d88566694e..c5970b2d3307 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -57,27 +57,27 @@ static inline enum iter_type iov_iter_type(const struct iov_iter *i)
static inline bool iter_is_iovec(const struct iov_iter *i)
{
- return iov_iter_type(i) == ITER_IOVEC;
+ return iov_iter_type(i) & ITER_IOVEC;
}
static inline bool iov_iter_is_kvec(const struct iov_iter *i)
{
- return iov_iter_type(i) == ITER_KVEC;
+ return iov_iter_type(i) & ITER_KVEC;
}
static inline bool iov_iter_is_bvec(const struct iov_iter *i)
{
- return iov_iter_type(i) == ITER_BVEC;
+ return iov_iter_type(i) & ITER_BVEC;
}
static inline bool iov_iter_is_pipe(const struct iov_iter *i)
{
- return iov_iter_type(i) == ITER_PIPE;
+ return iov_iter_type(i) & ITER_PIPE;
}
static inline bool iov_iter_is_discard(const struct iov_iter *i)
{
- return iov_iter_type(i) == ITER_DISCARD;
+ return iov_iter_type(i) & ITER_DISCARD;
}
static inline unsigned char iov_iter_rw(const struct iov_iter *i)
--
2.24.0
Block layer spend quite a while in iov_iter_npages(), but for bvecs
number of pages is already known and stored in iter->nr_segs, so it can
be returned directly as an optimisation
Running an io_uring benchmark with registered buffers (i.e. bvec) perf
showed ~1.5-2.0% total cycle was spent there, and that dropped to ~0.2%
after applying this patch.
Signed-off-by: Pavel Begunkov <[email protected]>
---
lib/iov_iter.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 1635111c5bd2..0fa7ac330acf 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1594,6 +1594,8 @@ int iov_iter_npages(const struct iov_iter *i, int maxpages)
return 0;
if (unlikely(iov_iter_is_discard(i)))
return 0;
+ if (unlikely(iov_iter_is_bvec(i)))
+ return min_t(int, i->nr_segs, maxpages);
if (unlikely(iov_iter_is_pipe(i))) {
struct pipe_inode_info *pipe = i->pipe;
@@ -1614,11 +1616,9 @@ int iov_iter_npages(const struct iov_iter *i, int maxpages)
- p / PAGE_SIZE;
if (npages >= maxpages)
return maxpages;
- 0;}),({
- npages++;
- if (npages >= maxpages)
- return maxpages;
- }),({
+ 0;}),
+ 0 /* bvecs are handled above */
+ ,({
unsigned long p = (unsigned long)v.iov_base;
npages += DIV_ROUND_UP(p + v.iov_len, PAGE_SIZE)
- p / PAGE_SIZE;
--
2.24.0
On 11/19/20 8:29 AM, Pavel Begunkov wrote:
> 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(-)
Nice! Tested this and confirmed both the better code generation,
and reduction in overhead in iov_iter_npages().
Reviewed-by: Jens Axboe <[email protected]>
--
Jens Axboe
On Thu, Nov 19, 2020 at 03:29:43PM +0000, Pavel Begunkov wrote:
> 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).
On 19/11/2020 17:03, Christoph Hellwig wrote:
> On Thu, Nov 19, 2020 at 03:29:43PM +0000, Pavel Begunkov wrote:
>> 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))
--
Pavel Begunkov
On 19/11/2020 16:46, Jens Axboe wrote:
> On 11/19/20 8:29 AM, Pavel Begunkov wrote:
>> 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(-)
>
> 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?
>
> Reviewed-by: Jens Axboe <[email protected]>
--
Pavel Begunkov
On 11/19/20 10:14 AM, Pavel Begunkov wrote:
> On 19/11/2020 16:46, Jens Axboe wrote:
>> On 11/19/20 8:29 AM, Pavel Begunkov wrote:
>>> 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(-)
>>
>> 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.
--
Jens Axboe
On 19/11/2020 17:20, Jens Axboe wrote:
> On 11/19/20 10:14 AM, Pavel Begunkov wrote:
>> On 19/11/2020 16:46, Jens Axboe wrote:
>>> On 11/19/20 8:29 AM, Pavel Begunkov wrote:
>>>> 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(-)
>>>
>>> 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.
That's great. I was guessing how much of it can be due
to not cached bvec and was just tossed to somewhere else.
--
Pavel Begunkov
On Thu, Nov 19, 2020 at 05:12:44PM +0000, Pavel Begunkov wrote:
> On 19/11/2020 17:03, Christoph Hellwig wrote:
> > On Thu, Nov 19, 2020 at 03:29:43PM +0000, Pavel Begunkov wrote:
> >> 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))
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?
From: Pavel Begunkov
> Sent: 13 December 2020 22:33
>
> On 11/12/2020 02:01, Al Viro wrote:
> > On Thu, Nov 19, 2020 at 05:12:44PM +0000, Pavel Begunkov wrote:
> >> On 19/11/2020 17:03, Christoph Hellwig wrote:
> >>> On Thu, Nov 19, 2020 at 03:29:43PM +0000, Pavel Begunkov wrote:
> >>>> 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))
> >
> > 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?
>
> 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.
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
}
I'm not sure of the best order though.
You might want 3 bits in the first test.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 14/12/2020 10:28, David Laight wrote:
> From: Pavel Begunkov
>> Sent: 13 December 2020 22:33
>>
>> On 11/12/2020 02:01, Al Viro wrote:
>>> On Thu, Nov 19, 2020 at 05:12:44PM +0000, Pavel Begunkov wrote:
>>>> On 19/11/2020 17:03, Christoph Hellwig wrote:
>>>>> On Thu, Nov 19, 2020 at 03:29:43PM +0000, Pavel Begunkov wrote:
>>>>>> 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))
>>>
>>> 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?
>>
>> 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.
>
> The advantage of the bit-masks is that the 'usual' options can
> be tested for together. So the code can be (for example):
Well, you can do that for the non-bitwise case as well.
In a simpler form but should be enough.
enum { ITER_IOVEC = 1, ITER_BVEC = 2, ... }
if (type <= ITER_BVEC) {
if (iovec) ...
if (bvec) ...
} else { ... }
> 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
> }
--
Pavel Begunkov
On 11/12/2020 02:01, Al Viro wrote:
> On Thu, Nov 19, 2020 at 05:12:44PM +0000, Pavel Begunkov wrote:
>> On 19/11/2020 17:03, Christoph Hellwig wrote:
>>> On Thu, Nov 19, 2020 at 03:29:43PM +0000, Pavel Begunkov wrote:
>>>> 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))
>
> 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?
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.
--
Pavel Begunkov