Re: [PATCH] btrfs: add special case to setget helpers for 64k pages

[David Sterba <dsterba@suse.cz>]

On Mon, Jul 05, 2021 at 09:33:34AM +0100, Christoph Hellwig wrote:
> On Fri, Jul 02, 2021 at 01:06:30PM +0200, David Sterba wrote:
> > On Fri, Jul 02, 2021 at 08:10:50AM +0100, Christoph Hellwig wrote:
> > > > +	if (INLINE_EXTENT_BUFFER_PAGES == 1) {				\
> > > >  		return get_unaligned_le##bits(token->kaddr + oip);	\
> > > > +	} else {							\
> > > 
> > > No need for an else after the return and thus no need for all the
> > > reformatting.
> > 
> > That leads to worse code, compiler does not eliminate the block that
> > would otherwise be in the else block. Measured on x86_64 with
> > instrumented code to force INLINE_EXTENT_BUFFER_PAGES = 1 this adds
> > +1100 bytes of code and has impact on stack consumption.
> > 
> > That the code that is in two branches that do not share any code is
> > maybe not pretty but the compiler did what I expected.  The set/get
> > helpers get called a lot and are performance sensitive.
> > 
> > This patch pre (original version), post (with dropped else):
> > 
> > 1156210   19305   14912 1190427  122a1b pre/btrfs.ko
> > 1157386   19305   14912 1191603  122eb3 post/btrfs.ko
> 
> For the obvious trivial patch (see below) I see the following
> difference, which actually makes the simple change smaller:
> 
>    text	   data	    bss	    dec	    hex	filename
> 1322580	 112183	  27600	1462363	 16505b	fs/btrfs/btrfs.o.hch
> 1322832	 112183	  27600	1462615	 165157	fs/btrfs/btrfs.o.dave

This was on x86_64 and without any further changes to the
extent_buffer::pages, right?

I've tested your version with the following diff emulating the single
page that would be on ppc:

--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -94,7 +94,8 @@ struct extent_buffer {

        struct rw_semaphore lock;

-       struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
+       struct page *pages[1];
+       /* struct page *pages[INLINE_EXTENT_BUFFER_PAGES]; */
        struct list_head release_list;
 #ifdef CONFIG_BTRFS_DEBUG
        struct list_head leak_list;
diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index 8260f8bb3ff0..4f8e8f7b29d1 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -52,6 +52,8 @@ static bool check_setget_bounds(const struct extent_buffer *eb,
  * from 0 to metadata node size.
  */

+#define _INLINE_EXTENT_BUFFER_PAGES 1
...
---

And replacing _INLINE_EXTENT_BUFFER_PAGES in the checks. This leads to
the same result as in my original version with the copied blocks:

   text    data     bss     dec     hex filename
1161350   19305   14912 1195567  123e2f pre/btrfs.ko
1156090   19305   14912 1190307  1229a3 post/btrfs.ko

DELTA: -5260

ie. compiler properly removed the dead code after evaluating the
conditions. As your change is simpler code I'll take it, tahnks for the
suggestion.