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

[Christoph Hellwig <hch@infradead.org>]

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 is sing the following compiler:
gcc version 10.2.1 20210110 (Debian 10.2.1-6) 

---
diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index 8260f8bb3ff0..a20954f06ec8 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -73,7 +73,7 @@ u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
 	}								\
 	token->kaddr = page_address(token->eb->pages[idx]);		\
 	token->offset = idx << PAGE_SHIFT;				\
-	if (oip + size <= PAGE_SIZE)					\
+	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oip + size <= PAGE_SIZE)	\
 		return get_unaligned_le##bits(token->kaddr + oip);	\
 									\
 	memcpy(lebytes, token->kaddr + oip, part);			\
@@ -94,7 +94,7 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
 	u8 lebytes[sizeof(u##bits)];					\
 									\
 	ASSERT(check_setget_bounds(eb, ptr, off, size));		\
-	if (oip + size <= PAGE_SIZE)					\
+	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oip + size <= PAGE_SIZE)	\
 		return get_unaligned_le##bits(kaddr + oip);		\
 									\
 	memcpy(lebytes, kaddr + oip, part);				\
@@ -124,7 +124,7 @@ void btrfs_set_token_##bits(struct btrfs_map_token *token,		\
 	}								\
 	token->kaddr = page_address(token->eb->pages[idx]);		\
 	token->offset = idx << PAGE_SHIFT;				\
-	if (oip + size <= PAGE_SIZE) {					\
+	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oip + size <= PAGE_SIZE) { \
 		put_unaligned_le##bits(val, token->kaddr + oip);	\
 		return;							\
 	}								\
@@ -146,7 +146,7 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
 	u8 lebytes[sizeof(u##bits)];					\
 									\
 	ASSERT(check_setget_bounds(eb, ptr, off, size));		\
-	if (oip + size <= PAGE_SIZE) {					\
+	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oip + size <= PAGE_SIZE) { \
 		put_unaligned_le##bits(val, kaddr + oip);		\
 		return;							\
 	}								\