[PATCH] dax: use switch statement over chained ifs

[PATCH] dax: use switch statement over chained ifs

[Amy Parker]

This patch uses a switch statement for pe_order, which improves
readability and on some platforms may minorly improve performance. It
also, to improve readability, recognizes that `PAGE_SHIFT - PAGE_SHIFT' is
a constant, and uses 0 in its place instead.

Signed-off-by: Amy Parker <apark0006@student.cerritos.edu>
---
 fs/dax.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index c48a3a93ab29..e8beed601384 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -32,13 +32,16 @@

 static inline unsigned int pe_order(enum page_entry_size pe_size)
 {
-    if (pe_size == PE_SIZE_PTE)
-        return PAGE_SHIFT - PAGE_SHIFT;
-    if (pe_size == PE_SIZE_PMD)
+    switch (pe_size) {
+    case PE_SIZE_PTE:
+        return 0;
+    case PE_SIZE_PMD:
         return PMD_SHIFT - PAGE_SHIFT;
-    if (pe_size == PE_SIZE_PUD)
+    case PE_SIZE_PUD:
         return PUD_SHIFT - PAGE_SHIFT;
-    return ~0;
+    default:
+        return ~0;
+    }
 }

 /* We choose 4096 entries - same as per-zone page wait tables */
-- 
2.39.0

Re: [PATCH] dax: use switch statement over chained ifs

[Matthew Wilcox]

On Mon, Jan 16, 2023 at 06:11:00PM -0800, Amy Parker wrote:
> This patch uses a switch statement for pe_order, which improves
> readability and on some platforms may minorly improve performance. It
> also, to improve readability, recognizes that `PAGE_SHIFT - PAGE_SHIFT' is
> a constant, and uses 0 in its place instead.
> 
> Signed-off-by: Amy Parker <apark0006@student.cerritos.edu>

Hi Amy,

Thanks for the patch!  Two problems.  First, your mailer seems to have
mangled the patch; in my tree these are tab indents, and the patch has
arrived with four-space indents, so it can't be applied.

The second problem is that this function should simply not exist.
I forget how we ended up with enum page_entry_size, but elsewhere
we simply pass 'order' around.  So what I'd really like to see is
a patch series that eliminates page_entry_size everywhere.

I can outline a way to do that in individual patches if that would be
helpful.

>  fs/dax.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index c48a3a93ab29..e8beed601384 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -32,13 +32,16 @@
> 
>  static inline unsigned int pe_order(enum page_entry_size pe_size)
>  {
> -    if (pe_size == PE_SIZE_PTE)
> -        return PAGE_SHIFT - PAGE_SHIFT;
> -    if (pe_size == PE_SIZE_PMD)
> +    switch (pe_size) {
> +    case PE_SIZE_PTE:
> +        return 0;
> +    case PE_SIZE_PMD:
>          return PMD_SHIFT - PAGE_SHIFT;
> -    if (pe_size == PE_SIZE_PUD)
> +    case PE_SIZE_PUD:
>          return PUD_SHIFT - PAGE_SHIFT;
> -    return ~0;
> +    default:
> +        return ~0;
> +    }
>  }
> 
>  /* We choose 4096 entries - same as per-zone page wait tables */
> -- 
> 2.39.0

Re: [PATCH] dax: use switch statement over chained ifs

[Amy Parker]

On Mon, Jan 16, 2023 at 6:41 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> CAUTION: This email originated from outside your organization. Exercise caution when opening attachments or clicking links, especially from unknown senders.
>
> On Mon, Jan 16, 2023 at 06:11:00PM -0800, Amy Parker wrote:
> > This patch uses a switch statement for pe_order, which improves
> > readability and on some platforms may minorly improve performance. It
> > also, to improve readability, recognizes that `PAGE_SHIFT - PAGE_SHIFT' is
> > a constant, and uses 0 in its place instead.
> >
> > Signed-off-by: Amy Parker <apark0006@student.cerritos.edu>
>
> Hi Amy,
>
> Thanks for the patch!  Two problems.  First, your mailer seems to have
> mangled the patch; in my tree these are tab indents, and the patch has
> arrived with four-space indents, so it can't be applied.

Ah, gotcha. Next time I'll just use git send-email, was hoping this
time I'd be able to use my normal mailing system directly. (Also
hoping my mail server isn't applying anything outgoing that messes it
up... should probably check on that)

> The second problem is that this function should simply not exist.
> I forget how we ended up with enum page_entry_size, but elsewhere
> we simply pass 'order' around.  So what I'd really like to see is
> a patch series that eliminates page_entry_size everywhere.

Hmm, alright... I'm not really familiar with the enum/how it's used, I
pretty much just added this as a cleanup. If you've got any
information on it so I know how to actually work with it, that'd be
great!

> I can outline a way to do that in individual patches if that would be
> helpful.

Alright - although, would it actually need to be individual patches?
I'm not 100% sure whether the page_entry_size used across the kernel
is the same enum or different enums, my guess looking at the grep
context summary is that they are the same, but the number of usages (I
count 18) should fit in a single patch just fine...

Re: [PATCH] dax: use switch statement over chained ifs

[Matthew Wilcox]

On Mon, Jan 16, 2023 at 09:07:23PM -0800, Amy Parker wrote:
> On Mon, Jan 16, 2023 at 6:41 PM Matthew Wilcox <willy@infradead.org> wrote:
> > CAUTION: This email originated from outside your organization. Exercise caution when opening attachments or clicking links, especially from unknown senders.

Muahaha.  I am evil.

> > Thanks for the patch!  Two problems.  First, your mailer seems to have
> > mangled the patch; in my tree these are tab indents, and the patch has
> > arrived with four-space indents, so it can't be applied.
> 
> Ah, gotcha. Next time I'll just use git send-email, was hoping this
> time I'd be able to use my normal mailing system directly. (Also
> hoping my mail server isn't applying anything outgoing that messes it
> up... should probably check on that)

Feel free to send me the patch again, off-list, and I can check if it
arrived correctly.

> > The second problem is that this function should simply not exist.
> > I forget how we ended up with enum page_entry_size, but elsewhere
> > we simply pass 'order' around.  So what I'd really like to see is
> > a patch series that eliminates page_entry_size everywhere.
> 
> Hmm, alright... I'm not really familiar with the enum/how it's used, I
> pretty much just added this as a cleanup. If you've got any
> information on it so I know how to actually work with it, that'd be
> great!

The intent is to describe which "layer" of the page tables we're trying
to hadle a fault for -- PTE, PMD or PUD.  But as you can see by this
pe_order() function, the rest of the kernel tends to use the order
to communicate this information, so pass in 0, PMD_ORDER or PUD_ORDER.
Also PMD_ORDER and PUD_ORDER should exist in mm.h ;-)

> > I can outline a way to do that in individual patches if that would be
> > helpful.
> 
> Alright - although, would it actually need to be individual patches?
> I'm not 100% sure whether the page_entry_size used across the kernel
> is the same enum or different enums, my guess looking at the grep
> context summary is that they are the same, but the number of usages (I
> count 18) should fit in a single patch just fine...

I'd take it step by step.  First, I'd lift pe_order() to mm.h.
Second patch, convert dax_finish_sync_fault() to take an order instead
of a pe_size, making each caller call pe_order().  And do it at
the start of each function, eg the very first line of
__xfs_filemap_fault() should be

	unsigned int order = pe_order(pe_size);

Third, convert dax_iomap_fault() to take an order instead of a pe_size.
Fourth, convert huge_fault() to take an order.  Fifth, remove the
enum and pe_order.

This makes it easier to review, as well as looking good for your
contribution stats ;-)

RE: [PATCH] dax: use switch statement over chained ifs

[David Laight]

From: Amy Parker
> Sent: 17 January 2023 02:11
> 
> This patch uses a switch statement for pe_order, which improves
> readability and on some platforms may minorly improve performance. It
> also, to improve readability, recognizes that `PAGE_SHIFT - PAGE_SHIFT' is
> a constant, and uses 0 in its place instead.

The compiler is pretty much guaranteed to do that anyway.
The 'chained ifs' can generate better code because the
common case can be put first.
The compiler will base its 'chained ifs' (jump tables
can't be used due to cpu 'features' - and would be slower
anyway with only a few labels) on minimising the number
of conditionals.

(Never mind any of the other problems.)

	David

> 
> Signed-off-by: Amy Parker <apark0006@student.cerritos.edu>
> ---
>  fs/dax.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index c48a3a93ab29..e8beed601384 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -32,13 +32,16 @@
> 
>  static inline unsigned int pe_order(enum page_entry_size pe_size)
>  {
> -    if (pe_size == PE_SIZE_PTE)
> -        return PAGE_SHIFT - PAGE_SHIFT;
> -    if (pe_size == PE_SIZE_PMD)
> +    switch (pe_size) {
> +    case PE_SIZE_PTE:
> +        return 0;
> +    case PE_SIZE_PMD:
>          return PMD_SHIFT - PAGE_SHIFT;
> -    if (pe_size == PE_SIZE_PUD)
> +    case PE_SIZE_PUD:
>          return PUD_SHIFT - PAGE_SHIFT;
> -    return ~0;
> +    default:
> +        return ~0;
> +    }
>  }
> 
>  /* We choose 4096 entries - same as per-zone page wait tables */
> --
> 2.39.0

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)