Re: [PATCH 7/7] btrfs: accessors: factor out split memcpy with two sources

[David Sterba <dsterba@suse.cz>]

On Wed, Jul 02, 2025 at 11:53:37AM -0700, Boris Burkov wrote:
> On Tue, Jul 01, 2025 at 07:23:54PM +0200, David Sterba wrote:
> > The case of a reading the bytes from 2 folios needs two memcpy()s, the
> > compiler does not emit calls but two inline loops.
> > 
> > Factoring out the code makes some improvement (stack, code) and in the
> > future will provide an optimized implementation as well. (The analogical
> > version with two destinations is not done as it increases stack usage
> > but can be done if needed.)
> 
> Is there some fundamental reason for this, or does it just happen to be
> so? Sort of interesting either way. Does it make you worry that this
> stuff will regress randomly in the future?

My explanation for that is that it's in the compiler optimization black
box, the function is inline and when evaluated in the context of the
caller it just came out worse.

This is the patch:

--- a/fs/btrfs/accessors.c
+++ b/fs/btrfs/accessors.c
@@ -29,6 +29,14 @@ static __always_inline void memcpy_split_src(char *dest, const char *src1,
        memcpy(dest + len1, src2, total - len1);
 }
 
+static __always_inline void memcpy_split_dest(char *dest1, const char *src,
+                                             char *dest2, const size_t len1,
+                                             const size_t total)
+{
+       memcpy(dest1, src, len1);
+       memcpy(dest2, src + len1, total - len1);
+}
+
 /*
  * Macro templates that define helpers to read/write extent buffer data of a
  * given size, that are also used via ctree.h for access to item members by
@@ -105,9 +113,9 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,    \
                kaddr = folio_address(eb->folios[idx + 1]);             \
                *kaddr = lebytes[1];                                    \
        } else {                                                        \
-               memcpy(kaddr, lebytes, part);                           \
-               kaddr = folio_address(eb->folios[idx + 1]);             \
-               memcpy(kaddr, lebytes + part, sizeof(u##bits) - part);  \
+               memcpy_split_dest(kaddr, lebytes,                       \
+                                 folio_address(eb->folios[idx + 1]),   \
+                                 part, sizeof(u##bits));               \
        }                                                               \
 }
 
---

And the evaluation:

  btrfs_set_64                                           +8 (24 -> 32)
  btrfs_set_32                                           +8 (24 -> 32)

     text    data     bss     dec     hex filename
  1454157  115665   16088 1585910  1832f6 pre/btrfs.ko
  1454173  115665   16088 1585926  183306 post/btrfs.ko

  DELTA: +16

I excluded the patch because of the worse stack and code diff, because
the other patches were all an improvement. This one was preparatory for
the fancy optimization I have so it can be placed to a helper instead of
the macro. Given the remaining time before code freeze it would not
leave enough time for testing and it might also be a generic helper in
the end.