| Message ID | 5c9c22dbeecad883ca29b31896c262a8d2a77132.1607976425.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | no-copy bvec | expand |
From: Pavel Begunkov > Sent: 15 December 2020 00:20 > > iov_iter_advance() is heavily used, but implemented through generic > iteration. As bvecs have a specifically crafted advance() function, i.e. > bvec_iter_advance(), which is faster and slimmer, use it instead. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > lib/iov_iter.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/lib/iov_iter.c b/lib/iov_iter.c > index 1635111c5bd2..5b186dc2c9ea 100644 > --- a/lib/iov_iter.c > +++ b/lib/iov_iter.c > @@ -1067,6 +1067,21 @@ static void pipe_advance(struct iov_iter *i, size_t size) > pipe_truncate(i); > } > > +static void iov_iter_bvec_advance(struct iov_iter *i, size_t size) > +{ > + struct bvec_iter bi; > + > + bi.bi_size = i->count; > + bi.bi_bvec_done = i->iov_offset; > + bi.bi_idx = 0; > + bvec_iter_advance(i->bvec, &bi, size); > + > + i->bvec += bi.bi_idx; > + i->nr_segs -= bi.bi_idx; > + i->count = bi.bi_size; > + i->iov_offset = bi.bi_bvec_done; > +} > + > void iov_iter_advance(struct iov_iter *i, size_t size) > { > if (unlikely(iov_iter_is_pipe(i))) { > @@ -1077,6 +1092,10 @@ void iov_iter_advance(struct iov_iter *i, size_t size) > i->count -= size; > return; > } > + if (iov_iter_is_bvec(i)) { > + iov_iter_bvec_advance(i, size); > + return; > + } > iterate_and_advance(i, size, v, 0, 0, 0) > } This seems to add yet another comparison before what is probably the common case on an IOVEC (ie normal userspace buffer). Can't the call to bver_iter_advance be dropped into the 'advance' path for BVEC's inside iterate_and_advance? iterate_and_advance itself has three 'unlikely' conditional tests that may be mis-predicted taken before the 'likely' path. One is for DISCARD which is checked twice on the object I just looked at - the test in iov_iter_advance() is pointless. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 15/12/2020 09:37, David Laight wrote: > From: Pavel Begunkov >> Sent: 15 December 2020 00:20 >> >> iov_iter_advance() is heavily used, but implemented through generic >> iteration. As bvecs have a specifically crafted advance() function, i.e. >> bvec_iter_advance(), which is faster and slimmer, use it instead. >> >> lib/iov_iter.c | 19 +++++++++++++++++++ [...] >> void iov_iter_advance(struct iov_iter *i, size_t size) >> { >> if (unlikely(iov_iter_is_pipe(i))) { >> @@ -1077,6 +1092,10 @@ void iov_iter_advance(struct iov_iter *i, size_t size) >> i->count -= size; >> return; >> } >> + if (iov_iter_is_bvec(i)) { >> + iov_iter_bvec_advance(i, size); >> + return; >> + } >> iterate_and_advance(i, size, v, 0, 0, 0) >> } > > This seems to add yet another comparison before what is probably > the common case on an IOVEC (ie normal userspace buffer). If Al finally takes the patch for iov_iter_is_*() helpers it would be completely optimised out. > > Can't the call to bver_iter_advance be dropped into the 'advance' > path for BVEC's inside iterate_and_advance? It iterates by page/segment/etc., why would you want to do bver_iter_advance(i->count) there? > > iterate_and_advance itself has three 'unlikely' conditional tests > that may be mis-predicted taken before the 'likely' path. > One is for DISCARD which is checked twice on the object I just > looked at - the test in iov_iter_advance() is pointless. And again, both second checks, including for discards, would be optimised out by the iov_iter_is_* patch.
From: Pavel Begunkov > Sent: 15 December 2020 11:24 > > On 15/12/2020 09:37, David Laight wrote: > > From: Pavel Begunkov > >> Sent: 15 December 2020 00:20 > >> > >> iov_iter_advance() is heavily used, but implemented through generic > >> iteration. As bvecs have a specifically crafted advance() function, i.e. > >> bvec_iter_advance(), which is faster and slimmer, use it instead. > >> > >> lib/iov_iter.c | 19 +++++++++++++++++++ > [...] > >> void iov_iter_advance(struct iov_iter *i, size_t size) > >> { > >> if (unlikely(iov_iter_is_pipe(i))) { > >> @@ -1077,6 +1092,10 @@ void iov_iter_advance(struct iov_iter *i, size_t size) > >> i->count -= size; > >> return; > >> } > >> + if (iov_iter_is_bvec(i)) { > >> + iov_iter_bvec_advance(i, size); > >> + return; > >> + } > >> iterate_and_advance(i, size, v, 0, 0, 0) > >> } > > > > This seems to add yet another comparison before what is probably > > the common case on an IOVEC (ie normal userspace buffer). > > If Al finally takes the patch for iov_iter_is_*() helpers it would > be completely optimised out. I knew I didn't have that path - the sources I looked at aren't that new. Didn't know its state. In any case that just stops the same test being done twice. In still changes the order of the tests. The three 'unlikely' cases should really be inside a single 'unlikely' test for all three bits. Then there is only one mis-predictable jump prior to the usual path. By adding the test before iterate_and_advance() you are (effectively) optimising for the bvec (and discard) cases. Adding 'unlikely()' won't make any difference on some architectures. IIRC recent intel x86 don't have a 'static prediction' for unknown branches - they just use whatever in is the branch predictor tables. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 15/12/2020 13:54, David Laight wrote: > From: Pavel Begunkov >> Sent: 15 December 2020 11:24 >> >> On 15/12/2020 09:37, David Laight wrote: >>> From: Pavel Begunkov >>>> Sent: 15 December 2020 00:20 >>>> >>>> iov_iter_advance() is heavily used, but implemented through generic >>>> iteration. As bvecs have a specifically crafted advance() function, i.e. >>>> bvec_iter_advance(), which is faster and slimmer, use it instead. >>>> >>>> lib/iov_iter.c | 19 +++++++++++++++++++ >> [...] >>>> void iov_iter_advance(struct iov_iter *i, size_t size) >>>> { >>>> if (unlikely(iov_iter_is_pipe(i))) { >>>> @@ -1077,6 +1092,10 @@ void iov_iter_advance(struct iov_iter *i, size_t size) >>>> i->count -= size; >>>> return; >>>> } >>>> + if (iov_iter_is_bvec(i)) { >>>> + iov_iter_bvec_advance(i, size); >>>> + return; >>>> + } >>>> iterate_and_advance(i, size, v, 0, 0, 0) >>>> } >>> >>> This seems to add yet another comparison before what is probably >>> the common case on an IOVEC (ie normal userspace buffer). >> >> If Al finally takes the patch for iov_iter_is_*() helpers it would >> be completely optimised out. > > I knew I didn't have that path - the sources I looked at aren't that new. > Didn't know its state. > > In any case that just stops the same test being done twice. > In still changes the order of the tests. > > The three 'unlikely' cases should really be inside a single > 'unlikely' test for all three bits. > Then there is only one mis-predictable jump prior to the usual path. > > By adding the test before iterate_and_advance() you are (effectively) > optimising for the bvec (and discard) cases. Take a closer look, bvec check is already first in iterate_and_advance(). Anyway, that all is an unrelated story. > Adding 'unlikely()' won't make any difference on some architectures. > IIRC recent intel x86 don't have a 'static prediction' for unknown > branches - they just use whatever in is the branch predictor tables.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 1635111c5bd2..5b186dc2c9ea 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1067,6 +1067,21 @@ static void pipe_advance(struct iov_iter *i, size_t size) pipe_truncate(i); } +static void iov_iter_bvec_advance(struct iov_iter *i, size_t size) +{ + struct bvec_iter bi; + + bi.bi_size = i->count; + bi.bi_bvec_done = i->iov_offset; + bi.bi_idx = 0; + bvec_iter_advance(i->bvec, &bi, size); + + i->bvec += bi.bi_idx; + i->nr_segs -= bi.bi_idx; + i->count = bi.bi_size; + i->iov_offset = bi.bi_bvec_done; +} + void iov_iter_advance(struct iov_iter *i, size_t size) { if (unlikely(iov_iter_is_pipe(i))) { @@ -1077,6 +1092,10 @@ void iov_iter_advance(struct iov_iter *i, size_t size) i->count -= size; return; } + if (iov_iter_is_bvec(i)) { + iov_iter_bvec_advance(i, size); + return; + } iterate_and_advance(i, size, v, 0, 0, 0) } EXPORT_SYMBOL(iov_iter_advance);
iov_iter_advance() is heavily used, but implemented through generic iteration. As bvecs have a specifically crafted advance() function, i.e. bvec_iter_advance(), which is faster and slimmer, use it instead. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- lib/iov_iter.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)