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@xxxxxxxxxxxxx> 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 ;-)
