linux-kernel - Re: [PATCH 1/3] lib/min_heap: Introduce non-inline versions of min heap API functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241017095520.GV16066@noisy.programming.kicks-ass.net>
Date: Thu, 17 Oct 2024 11:55:20 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: Kuan-Wei Chiu <visitorckw@...il.com>, colyli@...e.de, msakai@...hat.com,
	corbet@....net, mingo@...hat.com, acme@...nel.org,
	namhyung@...nel.org, akpm@...ux-foundation.org,
	mark.rutland@....com, alexander.shishkin@...ux.intel.com,
	jolsa@...nel.org, irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com, jserv@...s.ncku.edu.tw,
	linux-kernel@...r.kernel.org, linux-bcache@...r.kernel.org,
	dm-devel@...ts.linux.dev, linux-bcachefs@...r.kernel.org,
	linux-perf-users@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/3] lib/min_heap: Introduce non-inline versions of min
 heap API functions

On Wed, Oct 16, 2024 at 11:26:30PM -0400, Kent Overstreet wrote:

> yeah, I think we would prefer smaller codesize, by default.
> 
> it'd be well worth checking the code size difference on inlined vs. not,
> and then the really think to do would be to provide optional _inlined()
> helpers that we can switch to if/when a particular codepath shows up in
> a profile

Make sure to build with kCFI and IBT enabled when you do this and enjoy
seeing ec_stripes_heap_cmp turn into this:

00000000000027c0 <__cfi_ec_stripes_heap_cmp>:
    27c0:       b8 01 88 88 07          mov    $0x7888801,%eax
    27c5:       90                      nop
    27c6:       90                      nop
    27c7:       90                      nop
    27c8:       90                      nop
    27c9:       90                      nop
    27ca:       90                      nop
    27cb:       90                      nop
    27cc:       90                      nop
    27cd:       90                      nop
    27ce:       90                      nop
    27cf:       90                      nop

00000000000027d0 <ec_stripes_heap_cmp>:
    27d0:       f3 0f 1e fa             endbr64
    27d4:       e8 00 00 00 00          call   27d9 <ec_stripes_heap_cmp+0x9>   27d5: R_X86_64_PLT32    __fentry__-0x4
    27d9:       8b 47 08                mov    0x8(%rdi),%eax
    27dc:       3b 46 08                cmp    0x8(%rsi),%eax
    27df:       0f 92 c0                setb   %al
    27e2:       2e e9 00 00 00 00       cs jmp 27e8 <ec_stripes_heap_cmp+0x18>  27e4: R_X86_64_PLT32    __x86_return_thunk-0x4
    27e8:       0f 1f 84 00 00 00 00 00         nopl   0x0(%rax,%rax,1)
    27f0:       90                      nop
    27f1:       90                      nop
    27f2:       90                      nop
    27f3:       90                      nop
    27f4:       90                      nop


While I was there, you might want to do the below. It makes no sense
duplicating that thing.

---

diff --git a/fs/bcachefs/ec.c b/fs/bcachefs/ec.c
index 1587c6e1866a..3a6c34cf8a15 100644
--- a/fs/bcachefs/ec.c
+++ b/fs/bcachefs/ec.c
@@ -1048,6 +1048,11 @@ static inline void ec_stripes_heap_swap(void *l, void *r, void *h)
 	ec_stripes_heap_set_backpointer(_h, j);
 }
 
+static const struct min_heap_callbacks ec_stripes_callbacks = {
+	.less = ec_stripes_heap_cmp,
+	.swp = ec_stripes_heap_swap,
+};
+
 static void heap_verify_backpointer(struct bch_fs *c, size_t idx)
 {
 	ec_stripes_heap *h = &c->ec_stripes_heap;
@@ -1060,26 +1065,16 @@ static void heap_verify_backpointer(struct bch_fs *c, size_t idx)
 void bch2_stripes_heap_del(struct bch_fs *c,
 			   struct stripe *m, size_t idx)
 {
-	const struct min_heap_callbacks callbacks = {
-		.less = ec_stripes_heap_cmp,
-		.swp = ec_stripes_heap_swap,
-	};
-
 	mutex_lock(&c->ec_stripes_heap_lock);
 	heap_verify_backpointer(c, idx);
 
-	min_heap_del(&c->ec_stripes_heap, m->heap_idx, &callbacks, &c->ec_stripes_heap);
+	min_heap_del(&c->ec_stripes_heap, m->heap_idx, &ec_strips_callbacks, &c->ec_stripes_heap);
 	mutex_unlock(&c->ec_stripes_heap_lock);
 }
 
 void bch2_stripes_heap_insert(struct bch_fs *c,
 			      struct stripe *m, size_t idx)
 {
-	const struct min_heap_callbacks callbacks = {
-		.less = ec_stripes_heap_cmp,
-		.swp = ec_stripes_heap_swap,
-	};
-
 	mutex_lock(&c->ec_stripes_heap_lock);
 	BUG_ON(min_heap_full(&c->ec_stripes_heap));
 
@@ -1088,7 +1083,7 @@ void bch2_stripes_heap_insert(struct bch_fs *c,
 			.idx = idx,
 			.blocks_nonempty = m->blocks_nonempty,
 		}),
-		&callbacks,
+		&ec_stripes_callbacks,
 		&c->ec_stripes_heap);
 
 	heap_verify_backpointer(c, idx);
@@ -1098,10 +1093,6 @@ void bch2_stripes_heap_insert(struct bch_fs *c,
 void bch2_stripes_heap_update(struct bch_fs *c,
 			      struct stripe *m, size_t idx)
 {
-	const struct min_heap_callbacks callbacks = {
-		.less = ec_stripes_heap_cmp,
-		.swp = ec_stripes_heap_swap,
-	};
 	ec_stripes_heap *h = &c->ec_stripes_heap;
 	bool do_deletes;
 	size_t i;
@@ -1112,8 +1103,8 @@ void bch2_stripes_heap_update(struct bch_fs *c,
 	h->data[m->heap_idx].blocks_nonempty = m->blocks_nonempty;
 
 	i = m->heap_idx;
-	min_heap_sift_up(h,	i, &callbacks, &c->ec_stripes_heap);
-	min_heap_sift_down(h, i, &callbacks, &c->ec_stripes_heap);
+	min_heap_sift_up(  h, i, &ec_stripes_callbacks, &c->ec_stripes_heap);
+	min_heap_sift_down(h, i, &ec_stripes_callbacks, &c->ec_stripes_heap);
 
 	heap_verify_backpointer(c, idx);