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

From: Amy Parker
Date: Tue Jan 17 2023 - 00:07:35 EST


On Mon, Jan 16, 2023 at 6:41 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
>
> 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...