Skip to contentSkip to conversation

block: optimise bvec_iter_advance()

PATCH Applied Pavel Begunkov 3 participants 4 messages Nov 30, 2019 – Dec 3, 2019 1 message not in the archive Block layer

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Begunkov

author
#1 lore raw

PATCH v2 1/1 block: optimise bvec_iter_advance()

bvec_iter_advance() is quite popular, but compilers fail to do proper alias analysis and optimise it good enough. The assembly is checked for gcc 9.2, x86-64.

- remove @iter->bi_size from min(...), as it's always less than @bytes. Modify at the beginning and forget about it.

- the compiler isn't able to collapse memory dependencies and remove writes in the loop. Help it by explicitely using local vars.

Signed-off-by Pavel Begunkov <[email protected]>

---

v2: simplify code (Arvind Sankar)

 include/linux/bvec.h | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
include/linux/bvec.h +10 -12
index a032f01e928c..679a42253170 100644
@@ -87,26 +87,24 @@ struct bvec_iter_all {
 static inline bool bvec_iter_advance(const struct bio_vec *bv,
 		struct bvec_iter *iter, unsigned bytes)
 {
+	unsigned int idx = iter->bi_idx;
+
 	if (WARN_ONCE(bytes > iter->bi_size,
 		     "Attempted to advance past end of bvec iter\n")) {
 		iter->bi_size = 0;
 		return false;
 	}
 
-	while (bytes) {
-		const struct bio_vec *cur = bv + iter->bi_idx;
-		unsigned len = min3(bytes, iter->bi_size,
-				    cur->bv_len - iter->bi_bvec_done);
-
-		bytes -= len;
-		iter->bi_size -= len;
-		iter->bi_bvec_done += len;
+	iter->bi_size -= bytes;
+	bytes += iter->bi_bvec_done;
 
-		if (iter->bi_bvec_done == cur->bv_len) {
-			iter->bi_bvec_done = 0;
-			iter->bi_idx++;
-		}
+	while (bytes && bytes >= bv[idx].bv_len) {
+		bytes -= bv[idx].bv_len;
+		idx++;
 	}
+
+	iter->bi_idx = idx;
+	iter->bi_bvec_done = bytes;
 	return true;
 }
Signature · 1 line
2.24.0
#2 lore raw
Quoted 7 lines · Pavel Begunkov

bvec_iter_advance() is quite popular, but compilers fail to do proper alias analysis and optimise it good enough. The assembly is checked for gcc 9.2, x86-64.

- remove @iter->bi_size from min(...), as it's always less than @bytes. Modify at the beginning and forget about it.

Quoted 8 lines

- the compiler isn't able to collapse memory dependencies and remove writes in the loop. Help it by explicitely using local vars.

Signed-off-by Pavel Begunkov <[email protected]>

---

v2: simplify code (Arvind Sankar)

Thanks :)

Signed-off-by Arvind Sankar <[email protected]>

Btw, I discovered that gcc 9.2 doesn't optimize away the second comparison in something like

	m = min(a,b);
	return m>a;

So the WARN_ONCE bit doesn't get optimized away even in cases like bio_for_each_bvec where it's guaranteed at compile-time to not trigger.

author
#3 lore raw
Quoted 17 lines · Arvind Sankar
Quoted 7 lines · Pavel Begunkov

bvec_iter_advance() is quite popular, but compilers fail to do proper alias analysis and optimise it good enough. The assembly is checked for gcc 9.2, x86-64.

- remove @iter->bi_size from min(...), as it's always less than @bytes. Modify at the beginning and forget about it.

Quoted 8 lines

- the compiler isn't able to collapse memory dependencies and remove writes in the loop. Help it by explicitly using local vars.

Signed-off-by Pavel Begunkov <[email protected]>

---

v2: simplify code (Arvind Sankar)

Quoted 6 lines

Thanks :)

Signed-off-by Arvind Sankar <[email protected]>

Btw, I discovered that gcc 9.2 doesn't optimize away the second comparison in something like

That's unfortunate. IDK, whether it could be easily done in gcc, but maybe relay it to compiler guys?

Quoted 7 lines
	m = min(a,b);
	return m>a;

So the WARN_ONCE bit doesn't get optimized away even in cases like bio_for_each_bvec where it's guaranteed at compile-time to not trigger.

That's more like a starting point. The idea is to revise and tune/rewrite iteration helpers including *for_each_bvec(). I assume, all those are really poorly optimisable (especially with -fno-strict-aliasing). So, noted to consider

Signature · 1 line
Pavel Begunkov
maintainer Applied
#4 lore raw
In reply to Pavel Begunkov
Quoted 9 lines · Pavel Begunkov

bvec_iter_advance() is quite popular, but compilers fail to do proper alias analysis and optimise it good enough. The assembly is checked for gcc 9.2, x86-64.

- remove @iter->bi_size from min(...), as it's always less than @bytes. Modify at the beginning and forget about it.

- the compiler isn't able to collapse memory dependencies and remove writes in the loop. Help it by explicitely using local vars.

Applied, thanks.

Signature · 1 line
Jens Axboe