[PATCH] btrfs: make btrfs_node_key static inline

[PATCH] btrfs: make btrfs_node_key static inline

[Su Yue]

It looks strange that btrfs_node_key is in struct-funcs.c.
So move it to ctree.h and make it static inline.

Signed-off-by: Su Yue <l@damenly.su>
---
 fs/btrfs/ctree.h        | 10 ++++++++--
 fs/btrfs/struct-funcs.c |  8 --------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 452e410adbf5..67340a74f9ed 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1939,8 +1939,14 @@ static inline unsigned long btrfs_node_key_ptr_offset(int nr)
 		sizeof(struct btrfs_key_ptr) * nr;
 }
 
-void btrfs_node_key(const struct extent_buffer *eb,
-		    struct btrfs_disk_key *disk_key, int nr);
+static inline void btrfs_node_key(const struct extent_buffer *eb,
+				  struct btrfs_disk_key *disk_key, int nr)
+{
+	unsigned long ptr = btrfs_node_key_ptr_offset(nr);
+	read_eb_member(eb, (struct btrfs_key_ptr *)ptr,
+		       struct btrfs_key_ptr, key, disk_key);
+
+}
 
 static inline void btrfs_set_node_key(const struct extent_buffer *eb,
 				      struct btrfs_disk_key *disk_key, int nr)
diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index f429256f56db..7526005525cb 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -161,11 +161,3 @@ DEFINE_BTRFS_SETGET_BITS(8)
 DEFINE_BTRFS_SETGET_BITS(16)
 DEFINE_BTRFS_SETGET_BITS(32)
 DEFINE_BTRFS_SETGET_BITS(64)
-
-void btrfs_node_key(const struct extent_buffer *eb,
-		    struct btrfs_disk_key *disk_key, int nr)
-{
-	unsigned long ptr = btrfs_node_key_ptr_offset(nr);
-	read_eb_member(eb, (struct btrfs_key_ptr *)ptr,
-		       struct btrfs_key_ptr, key, disk_key);
-}
-- 
2.30.1

Re: [PATCH] btrfs: make btrfs_node_key static inline

[David Sterba]

On Tue, Sep 14, 2021 at 06:53:35PM +0800, Su Yue wrote:
> It looks strange that btrfs_node_key is in struct-funcs.c.
> So move it to ctree.h and make it static inline.

"looks strange" is not a sufficient reason. Inlining a function means
that the body will be expanded at each call site, bloating the binary
code. Have you measured the impact of that?

There's some performance cost of an non-inline function due to the call
overhead but it does not make sense to inline a function that's called
rarely and not in a tight loop. If you grep for the function you'd see
that it's called eg. once per function or in a loop that's not
performance critical on first sight (eg. in reada_for_search).

Re: [PATCH] btrfs: make btrfs_node_key static inline

[Su Yue]

On Tue 14 Sep 2021 at 15:12, David Sterba <dsterba@suse.cz> wrote:

> On Tue, Sep 14, 2021 at 06:53:35PM +0800, Su Yue wrote:
>> It looks strange that btrfs_node_key is in struct-funcs.c.
>> So move it to ctree.h and make it static inline.
>
> "looks strange" is not a sufficient reason. Inlining a function 
> means
> that the body will be expanded at each call site, bloating the 
> binary
> code. Have you measured the impact of that?
>
Fair enough.

Before:
   text    data     bss     dec     hex filename
1202418  123105   19384 1344907  14858b fs/btrfs/btrfs.ko
After:
   text    data     bss     dec     hex filename
1202620  123105   19384 1345109  148655 fs/btrfs/btrfs.ko

+202

> There's some performance cost of an non-inline function due to 
> the call
> overhead but it does not make sense to inline a function that's 
> called
> rarely and not in a tight loop. If you grep for the function 
> you'd see
> that it's called eg. once per function or in a loop that's not
> performance critical on first sight (eg. in reada_for_search).

Right, the patch won't impact performance obviously at the cost of
+202 binary size. So I'd drop the patch.

Sorry for the noise.

--
Su

Re: [PATCH] btrfs: make btrfs_node_key static inline

[David Sterba]

On Tue, Sep 14, 2021 at 10:08:36PM +0800, Su Yue wrote:
> 
> On Tue 14 Sep 2021 at 15:12, David Sterba <dsterba@suse.cz> wrote:
> 
> > On Tue, Sep 14, 2021 at 06:53:35PM +0800, Su Yue wrote:
> >> It looks strange that btrfs_node_key is in struct-funcs.c.
> >> So move it to ctree.h and make it static inline.
> >
> > "looks strange" is not a sufficient reason. Inlining a function 
> > means
> > that the body will be expanded at each call site, bloating the 
> > binary
> > code. Have you measured the impact of that?
> >
> Fair enough.
> 
> Before:
>    text    data     bss     dec     hex filename
> 1202418  123105   19384 1344907  14858b fs/btrfs/btrfs.ko
> After:
>    text    data     bss     dec     hex filename
> 1202620  123105   19384 1345109  148655 fs/btrfs/btrfs.ko
> 
> +202
> 
> > There's some performance cost of an non-inline function due to 
> > the call
> > overhead but it does not make sense to inline a function that's 
> > called
> > rarely and not in a tight loop. If you grep for the function 
> > you'd see
> > that it's called eg. once per function or in a loop that's not
> > performance critical on first sight (eg. in reada_for_search).
> 
> Right, the patch won't impact performance obviously at the cost of
> +202 binary size. So I'd drop the patch.

It does increase the size a bit but from what I've seen in the assembly
it's not that bad and still probably worth doing the inline. There's one
more extra call to read_extent_buffer (hidden behind read_eb_member
macro).

Cleaning up the node key helpers would be useful too, adding some
more helpers and not calling read_eb_member in the end. I have a WIP
patchset for that but had to leave it as there were bugs I did not find.
I can bounce it to you if you're interested.

Re: [PATCH] btrfs: make btrfs_node_key static inline

[Su Yue]

On Tue 14 Sep 2021 at 16:36, David Sterba <dsterba@suse.cz> wrote:

> On Tue, Sep 14, 2021 at 10:08:36PM +0800, Su Yue wrote:
>>
>> On Tue 14 Sep 2021 at 15:12, David Sterba <dsterba@suse.cz> 
>> wrote:
>>
>> > On Tue, Sep 14, 2021 at 06:53:35PM +0800, Su Yue wrote:
>> >> It looks strange that btrfs_node_key is in struct-funcs.c.
>> >> So move it to ctree.h and make it static inline.
>> >
>> > "looks strange" is not a sufficient reason. Inlining a 
>> > function
>> > means
>> > that the body will be expanded at each call site, bloating 
>> > the
>> > binary
>> > code. Have you measured the impact of that?
>> >
>> Fair enough.
>>
>> Before:
>>    text    data     bss     dec     hex filename
>> 1202418  123105   19384 1344907  14858b fs/btrfs/btrfs.ko
>> After:
>>    text    data     bss     dec     hex filename
>> 1202620  123105   19384 1345109  148655 fs/btrfs/btrfs.ko
>>
>> +202
>>
>> > There's some performance cost of an non-inline function due 
>> > to
>> > the call
>> > overhead but it does not make sense to inline a function 
>> > that's
>> > called
>> > rarely and not in a tight loop. If you grep for the function
>> > you'd see
>> > that it's called eg. once per function or in a loop that's 
>> > not
>> > performance critical on first sight (eg. in 
>> > reada_for_search).
>>
>> Right, the patch won't impact performance obviously at the cost 
>> of
>> +202 binary size. So I'd drop the patch.
>
> It does increase the size a bit but from what I've seen in the 
> assembly
> it's not that bad and still probably worth doing the inline. 
> There's one
> more extra call to read_extent_buffer (hidden behind 
> read_eb_member
> macro).
>
Thanks for taking a look. I just noticed the lonely function then 
post
the patch without deeper thinking.

> Cleaning up the node key helpers would be useful too, adding 
> some
> more helpers and not calling read_eb_member in the end. I have a 
> WIP
> patchset for that but had to leave it as there were bugs I did 
> not find.
> I can bounce it to you if you're interested.

Thanks a lot. But I can't take it due to some reasons. Since you 
have
a better WIP patchset, I don't mind forgoing the patch.

--
Su