[v2,02/25] mm: Optimise find_subpage for !THP
diff mbox series

Message ID 20200212041845.25879-3-willy@infradead.org
State New
Headers show
Series
  • Large pages in the page cache
Related show

Commit Message

Matthew Wilcox Feb. 12, 2020, 4:18 a.m. UTC
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>
---
 include/linux/pagemap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Feb. 12, 2020, 7:41 a.m. UTC | #1
On Tue, Feb 11, 2020 at 08:18:22PM -0800, Matthew Wilcox 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>
> ---
>  include/linux/pagemap.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 75bdfec49710..0842622cca90 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -340,7 +340,7 @@ static inline struct page *find_subpage(struct page *page, pgoff_t offset)
>  
>  	VM_BUG_ON_PAGE(PageTail(page), page);
>  
> -	return page + (offset & (compound_nr(page) - 1));
> +	return page + (offset & (hpage_nr_pages(page) - 1));
>  }

So just above the quoted code is a

	if (PageHuge(page))
		return page;

So how can we get into a compound page that is not a huge page, but
only if THP is enabled?  (Yes, maybe I'm a little rusty on VM
internals).

Can you add comments describing the use case of this function and why
it does all these checks?  It looks like black magic to me.
Matthew Wilcox Feb. 12, 2020, 1:02 p.m. UTC | #2
On Tue, Feb 11, 2020 at 11:41:05PM -0800, Christoph Hellwig wrote:
> On Tue, Feb 11, 2020 at 08:18:22PM -0800, Matthew Wilcox 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>
> > ---
> >  include/linux/pagemap.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index 75bdfec49710..0842622cca90 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -340,7 +340,7 @@ static inline struct page *find_subpage(struct page *page, pgoff_t offset)
> >  
> >  	VM_BUG_ON_PAGE(PageTail(page), page);
> >  
> > -	return page + (offset & (compound_nr(page) - 1));
> > +	return page + (offset & (hpage_nr_pages(page) - 1));
> >  }
> 
> So just above the quoted code is a
> 
> 	if (PageHuge(page))
> 		return page;
> 
> So how can we get into a compound page that is not a huge page, but
> only if THP is enabled?  (Yes, maybe I'm a little rusty on VM
> internals).

That's for hugetlbfs:

        if (!PageCompound(page))
                return 0;

        page = compound_head(page);
        return page[1].compound_dtor == HUGETLB_PAGE_DTOR;

> Can you add comments describing the use case of this function and why
> it does all these checks?  It looks like black magic to me.

Would this help?

-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);
 
-       return page + (offset & (hpage_nr_pages(page) - 1));
+       return head + (offset & (hpage_nr_pages(head) - 1));
 }
Christoph Hellwig Feb. 12, 2020, 5:52 p.m. UTC | #3
On Wed, Feb 12, 2020 at 05:02:00AM -0800, Matthew Wilcox wrote:
> > Can you add comments describing the use case of this function and why
> > it does all these checks?  It looks like black magic to me.
> 
> Would this help?
> 
> -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);
>  
> -       return page + (offset & (hpage_nr_pages(page) - 1));
> +       return head + (offset & (hpage_nr_pages(head) - 1));

Much better.
Kirill A. Shutemov Feb. 13, 2020, 1:50 p.m. UTC | #4
On Wed, Feb 12, 2020 at 05:02:00AM -0800, Matthew Wilcox wrote:
> Would this help?

LGTM:

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Patch
diff mbox series

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 75bdfec49710..0842622cca90 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -340,7 +340,7 @@  static inline struct page *find_subpage(struct page *page, pgoff_t offset)
 
 	VM_BUG_ON_PAGE(PageTail(page), page);
 
-	return page + (offset & (compound_nr(page) - 1));
+	return page + (offset & (hpage_nr_pages(page) - 1));
 }
 
 struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);