* [PATCH] btrfs: make btrfs_node_key static inline
@ 2021-09-14 10:53 Su Yue
2021-09-14 13:12 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Su Yue @ 2021-09-14 10:53 UTC (permalink / raw)
To: linux-btrfs; +Cc: l
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make btrfs_node_key static inline
2021-09-14 10:53 [PATCH] btrfs: make btrfs_node_key static inline Su Yue
@ 2021-09-14 13:12 ` David Sterba
2021-09-14 14:08 ` Su Yue
0 siblings, 1 reply; 5+ messages in thread
From: David Sterba @ 2021-09-14 13:12 UTC (permalink / raw)
To: Su Yue; +Cc: linux-btrfs
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).
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make btrfs_node_key static inline
2021-09-14 13:12 ` David Sterba
@ 2021-09-14 14:08 ` Su Yue
2021-09-14 14:36 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Su Yue @ 2021-09-14 14:08 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make btrfs_node_key static inline
2021-09-14 14:08 ` Su Yue
@ 2021-09-14 14:36 ` David Sterba
2021-09-14 14:55 ` Su Yue
0 siblings, 1 reply; 5+ messages in thread
From: David Sterba @ 2021-09-14 14:36 UTC (permalink / raw)
To: Su Yue; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: make btrfs_node_key static inline
2021-09-14 14:36 ` David Sterba
@ 2021-09-14 14:55 ` Su Yue
0 siblings, 0 replies; 5+ messages in thread
From: Su Yue @ 2021-09-14 14:55 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-14 15:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 10:53 [PATCH] btrfs: make btrfs_node_key static inline Su Yue
2021-09-14 13:12 ` David Sterba
2021-09-14 14:08 ` Su Yue
2021-09-14 14:36 ` David Sterba
2021-09-14 14:55 ` Su Yue
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.