Re: [PATCH 2/6] mm: Optimise find_subpage for !THP

[Alexander Duyck <alexander.duyck@gmail.com>]

On Mon, Mar 2, 2020 at 8:11 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
>
> If THP is disabled, find_subpage can become a no-op.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  include/linux/pagemap.h | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index b4ea3a5d00e5..8785f60b05f8 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -333,14 +333,19 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
>                         mapping_gfp_mask(mapping));
>  }
>
> -static inline struct page *find_subpage(struct page *page, pgoff_t offset)
> +/*
> + * Given the page we found in the page cache, return the page corresponding
> + * to this offset in the file
> + */
> +static inline struct page *find_subpage(struct page *head, pgoff_t offset)
>  {
> -       if (PageHuge(page))
> -               return page;
> +       /* HugeTLBfs wants the head page regardless */
> +       if (PageHuge(head))
> +               return head;
>
> -       VM_BUG_ON_PAGE(PageTail(page), page);
> +       VM_BUG_ON_PAGE(PageTail(head), head);

Is there any specific reason for renaming page to head? Just wondering
since it adds some noise to the patch that wasn't really called out in
the patch description. From what I can tell none of the changes above
this point have any explanation to them in the patch description, and
until I noticed the change below I thought maybe you had the wrong
patch description for this patch.

> -       return page + (offset & (compound_nr(page) - 1));
> +       return head + (offset & (hpage_nr_pages(head) - 1));
>  }

So the patch description refers to this line here, correct?

One thing I am noticing is that it looks like the VM_BUG_ON_PAGE is
now redundant. If I am not not mistaken hpage_nr_pages will call
PageTransHuge which also performs the same check. So do you still need
the VM_BUG_ON_PAGE call in this function?