* [PATCH 0/2] Better support for THP in page cache @ 2018-11-22 21:32 Matthew Wilcox 2018-11-22 21:32 ` [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig Matthew Wilcox 2018-11-22 21:32 ` [PATCH 2/2] page cache: Store only head pages in i_pages Matthew Wilcox 0 siblings, 2 replies; 8+ messages in thread From: Matthew Wilcox @ 2018-11-22 21:32 UTC (permalink / raw) To: linux-mm, linux-fsdevel Cc: linux-kernel, Kirill A. Shutemov, Hugh Dickins, Matthew Wilcox This is the first step towards representing THPs more efficiently in the page cache. The next step is to insert one head page instead of HPAGE_PMD_NR. This passes a Trinity run, but I'm sure there's a better test-case out there for THP. Matthew Wilcox (2): mm: Remove redundant test from find_get_pages_contig page cache: Store only head pages in i_pages include/linux/pagemap.h | 9 ++++ mm/filemap.c | 106 +++++++++++----------------------------- mm/khugepaged.c | 4 +- mm/shmem.c | 2 +- mm/swap_state.c | 2 +- 5 files changed, 42 insertions(+), 81 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig 2018-11-22 21:32 [PATCH 0/2] Better support for THP in page cache Matthew Wilcox @ 2018-11-22 21:32 ` Matthew Wilcox 2018-11-23 10:47 ` Kirill A. Shutemov 2018-11-22 21:32 ` [PATCH 2/2] page cache: Store only head pages in i_pages Matthew Wilcox 1 sibling, 1 reply; 8+ messages in thread From: Matthew Wilcox @ 2018-11-22 21:32 UTC (permalink / raw) To: linux-mm, linux-fsdevel Cc: linux-kernel, Kirill A. Shutemov, Hugh Dickins, Matthew Wilcox After we establish a reference on the page, we check the pointer continues to be in the correct position in i_pages. There's no need to check the page->mapping or page->index afterwards; if those can change after we've got the reference, they can change after we return the page to the caller. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- mm/filemap.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/mm/filemap.c b/mm/filemap.c index 81adec8ee02cc..538531590ef2d 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1776,16 +1776,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index, if (unlikely(page != xas_reload(&xas))) goto put_page; - /* - * must check mapping and index after taking the ref. - * otherwise we can get both false positives and false - * negatives, which is just confusing to the caller. - */ - if (!page->mapping || page_to_pgoff(page) != xas.xa_index) { - put_page(page); - break; - } - pages[ret] = page; if (++ret == nr_pages) break; -- 2.19.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig 2018-11-22 21:32 ` [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig Matthew Wilcox @ 2018-11-23 10:47 ` Kirill A. Shutemov 2018-11-23 17:59 ` Matthew Wilcox 0 siblings, 1 reply; 8+ messages in thread From: Kirill A. Shutemov @ 2018-11-23 10:47 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-mm, linux-fsdevel, linux-kernel, Kirill A. Shutemov, Hugh Dickins On Thu, Nov 22, 2018 at 01:32:23PM -0800, Matthew Wilcox wrote: > After we establish a reference on the page, we check the pointer continues > to be in the correct position in i_pages. There's no need to check the > page->mapping or page->index afterwards; if those can change after we've > got the reference, they can change after we return the page to the caller. Hm. IIRC, page->mapping can be set to NULL due truncation, but what about index? When it can be changed? Truncation doesn't touch it. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig 2018-11-23 10:47 ` Kirill A. Shutemov @ 2018-11-23 17:59 ` Matthew Wilcox 0 siblings, 0 replies; 8+ messages in thread From: Matthew Wilcox @ 2018-11-23 17:59 UTC (permalink / raw) To: Kirill A. Shutemov Cc: linux-mm, linux-fsdevel, linux-kernel, Kirill A. Shutemov, Hugh Dickins On Fri, Nov 23, 2018 at 01:47:32PM +0300, Kirill A. Shutemov wrote: > On Thu, Nov 22, 2018 at 01:32:23PM -0800, Matthew Wilcox wrote: > > After we establish a reference on the page, we check the pointer continues > > to be in the correct position in i_pages. There's no need to check the > > page->mapping or page->index afterwards; if those can change after we've > > got the reference, they can change after we return the page to the caller. > > Hm. IIRC, page->mapping can be set to NULL due truncation, but what about > index? When it can be changed? Truncation doesn't touch it. I think index can only be changed after the refcount has hit zero and the page is safely out of the pagecache. I agree that page->mapping can be set to NULL after the call to xas_reload() ... but then it can also happen after the check, so the check isn't really buying us anything that the xas_reload() call doesn't already check. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] page cache: Store only head pages in i_pages 2018-11-22 21:32 [PATCH 0/2] Better support for THP in page cache Matthew Wilcox 2018-11-22 21:32 ` [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig Matthew Wilcox @ 2018-11-22 21:32 ` Matthew Wilcox 2018-11-23 10:56 ` Kirill A. Shutemov 1 sibling, 1 reply; 8+ messages in thread From: Matthew Wilcox @ 2018-11-22 21:32 UTC (permalink / raw) To: linux-mm, linux-fsdevel Cc: linux-kernel, Kirill A. Shutemov, Hugh Dickins, Matthew Wilcox Transparent Huge Pages are currently stored in i_pages as pointers to consecutive subpages. This patch changes that to storing consecutive pointers to the head page in preparation for storing huge pages more efficiently in i_pages. Large parts of this are "inspired" by Kirill's patch https://lore.kernel.org/lkml/20170126115819.58875-2-kirill.shutemov@linux.intel.com/ Signed-off-by: Matthew Wilcox <willy@infradead.org> --- include/linux/pagemap.h | 9 ++++ mm/filemap.c | 96 +++++++++++++---------------------------- mm/khugepaged.c | 4 +- mm/shmem.c | 2 +- mm/swap_state.c | 2 +- 5 files changed, 42 insertions(+), 71 deletions(-) diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index 226f96f0dee06..41bf976574e74 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -345,6 +345,15 @@ 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) +{ + VM_BUG_ON_PAGE(PageTail(page), page); + VM_BUG_ON_PAGE(page->index > offset, page); + VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) <= offset, + page); + return page - page->index + offset; +} + struct page *find_get_entry(struct address_space *mapping, pgoff_t offset); struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset); unsigned find_get_entries(struct address_space *mapping, pgoff_t start, diff --git a/mm/filemap.c b/mm/filemap.c index 538531590ef2d..d7274591381ac 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1407,7 +1407,7 @@ EXPORT_SYMBOL(page_cache_prev_miss); struct page *find_get_entry(struct address_space *mapping, pgoff_t offset) { XA_STATE(xas, &mapping->i_pages, offset); - struct page *head, *page; + struct page *page; rcu_read_lock(); repeat: @@ -1422,25 +1422,19 @@ struct page *find_get_entry(struct address_space *mapping, pgoff_t offset) if (!page || xa_is_value(page)) goto out; - head = compound_head(page); - if (!page_cache_get_speculative(head)) - goto repeat; - - /* The page was split under us? */ - if (compound_head(page) != head) { - put_page(head); + if (!page_cache_get_speculative(page)) goto repeat; - } /* - * Has the page moved? + * Has the page moved or been split? * This is part of the lockless pagecache protocol. See * include/linux/pagemap.h for details. */ if (unlikely(page != xas_reload(&xas))) { - put_page(head); + put_page(page); goto repeat; } + page = find_subpage(page, offset); out: rcu_read_unlock(); @@ -1611,7 +1605,6 @@ unsigned find_get_entries(struct address_space *mapping, rcu_read_lock(); xas_for_each(&xas, page, ULONG_MAX) { - struct page *head; if (xas_retry(&xas, page)) continue; /* @@ -1622,17 +1615,13 @@ unsigned find_get_entries(struct address_space *mapping, if (xa_is_value(page)) goto export; - head = compound_head(page); - if (!page_cache_get_speculative(head)) + if (!page_cache_get_speculative(page)) goto retry; - /* The page was split under us? */ - if (compound_head(page) != head) - goto put_page; - - /* Has the page moved? */ + /* Has the page moved or been split? */ if (unlikely(page != xas_reload(&xas))) goto put_page; + page = find_subpage(page, xas.xa_index); export: indices[ret] = xas.xa_index; @@ -1641,7 +1630,7 @@ unsigned find_get_entries(struct address_space *mapping, break; continue; put_page: - put_page(head); + put_page(page); retry: xas_reset(&xas); } @@ -1683,33 +1672,27 @@ unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start, rcu_read_lock(); xas_for_each(&xas, page, end) { - struct page *head; if (xas_retry(&xas, page)) continue; /* Skip over shadow, swap and DAX entries */ if (xa_is_value(page)) continue; - head = compound_head(page); - if (!page_cache_get_speculative(head)) + if (!page_cache_get_speculative(page)) goto retry; - /* The page was split under us? */ - if (compound_head(page) != head) - goto put_page; - - /* Has the page moved? */ + /* Has the page moved or been split? */ if (unlikely(page != xas_reload(&xas))) goto put_page; - pages[ret] = page; + pages[ret] = find_subpage(page, xas.xa_index); if (++ret == nr_pages) { *start = page->index + 1; goto out; } continue; put_page: - put_page(head); + put_page(page); retry: xas_reset(&xas); } @@ -1754,7 +1737,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index, rcu_read_lock(); for (page = xas_load(&xas); page; page = xas_next(&xas)) { - struct page *head; if (xas_retry(&xas, page)) continue; /* @@ -1764,24 +1746,19 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index, if (xa_is_value(page)) break; - head = compound_head(page); - if (!page_cache_get_speculative(head)) + if (!page_cache_get_speculative(page)) goto retry; - /* The page was split under us? */ - if (compound_head(page) != head) - goto put_page; - - /* Has the page moved? */ + /* Has the page moved or been split? */ if (unlikely(page != xas_reload(&xas))) goto put_page; - pages[ret] = page; + pages[ret] = find_subpage(page, xas.xa_index); if (++ret == nr_pages) break; continue; put_page: - put_page(head); + put_page(page); retry: xas_reset(&xas); } @@ -1815,7 +1792,6 @@ unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index, rcu_read_lock(); xas_for_each_marked(&xas, page, end, tag) { - struct page *head; if (xas_retry(&xas, page)) continue; /* @@ -1826,26 +1802,21 @@ unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index, if (xa_is_value(page)) continue; - head = compound_head(page); - if (!page_cache_get_speculative(head)) + if (!page_cache_get_speculative(page)) goto retry; - /* The page was split under us? */ - if (compound_head(page) != head) - goto put_page; - - /* Has the page moved? */ + /* Has the page moved or been split? */ if (unlikely(page != xas_reload(&xas))) goto put_page; - pages[ret] = page; + pages[ret] = find_subpage(page, xas.xa_index); if (++ret == nr_pages) { *index = page->index + 1; goto out; } continue; put_page: - put_page(head); + put_page(page); retry: xas_reset(&xas); } @@ -1892,7 +1863,6 @@ unsigned find_get_entries_tag(struct address_space *mapping, pgoff_t start, rcu_read_lock(); xas_for_each_marked(&xas, page, ULONG_MAX, tag) { - struct page *head; if (xas_retry(&xas, page)) continue; /* @@ -1903,17 +1873,13 @@ unsigned find_get_entries_tag(struct address_space *mapping, pgoff_t start, if (xa_is_value(page)) goto export; - head = compound_head(page); - if (!page_cache_get_speculative(head)) + if (!page_cache_get_speculative(page)) goto retry; - /* The page was split under us? */ - if (compound_head(page) != head) - goto put_page; - - /* Has the page moved? */ + /* Has the page moved or been split? */ if (unlikely(page != xas_reload(&xas))) goto put_page; + page = find_subpage(page, xas.xa_index); export: indices[ret] = xas.xa_index; @@ -1922,7 +1888,7 @@ unsigned find_get_entries_tag(struct address_space *mapping, pgoff_t start, break; continue; put_page: - put_page(head); + put_page(page); retry: xas_reset(&xas); } @@ -2533,7 +2499,7 @@ void filemap_map_pages(struct vm_fault *vmf, pgoff_t last_pgoff = start_pgoff; unsigned long max_idx; XA_STATE(xas, &mapping->i_pages, start_pgoff); - struct page *head, *page; + struct page *page; rcu_read_lock(); xas_for_each(&xas, page, end_pgoff) { @@ -2542,17 +2508,13 @@ void filemap_map_pages(struct vm_fault *vmf, if (xa_is_value(page)) goto next; - head = compound_head(page); - if (!page_cache_get_speculative(head)) + if (!page_cache_get_speculative(page)) goto next; - /* The page was split under us? */ - if (compound_head(page) != head) - goto skip; - - /* Has the page moved? */ + /* Has the page moved or been split? */ if (unlikely(page != xas_reload(&xas))) goto skip; + page = find_subpage(page, xas.xa_index); if (!PageUptodate(page) || PageReadahead(page) || diff --git a/mm/khugepaged.c b/mm/khugepaged.c index c13625c1ad5e5..7d6a1319dd42e 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -1363,7 +1363,7 @@ static void collapse_shmem(struct mm_struct *mm, result = SCAN_FAIL; break; } - xas_store(&xas, new_page + (index % HPAGE_PMD_NR)); + xas_store(&xas, new_page); nr_none++; continue; } @@ -1431,7 +1431,7 @@ static void collapse_shmem(struct mm_struct *mm, list_add_tail(&page->lru, &pagelist); /* Finally, replace with the new page. */ - xas_store(&xas, new_page + (index % HPAGE_PMD_NR)); + xas_store(&xas, new_page); continue; out_lru: xas_unlock_irq(&xas); diff --git a/mm/shmem.c b/mm/shmem.c index ea26d7a0342d7..c2ba84cbb0c0e 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -603,7 +603,7 @@ static int shmem_add_to_page_cache(struct page *page, if (xas_error(&xas)) goto unlock; next: - xas_store(&xas, page + i); + xas_store(&xas, page); if (++i < nr) { xas_next(&xas); goto next; diff --git a/mm/swap_state.c b/mm/swap_state.c index fd2f21e1c60ae..dcf9e466d2945 100644 --- a/mm/swap_state.c +++ b/mm/swap_state.c @@ -132,7 +132,7 @@ int add_to_swap_cache(struct page *page, swp_entry_t entry, gfp_t gfp) for (i = 0; i < nr; i++) { VM_BUG_ON_PAGE(xas.xa_index != idx + i, page); set_page_private(page + i, entry.val + i); - xas_store(&xas, page + i); + xas_store(&xas, page); xas_next(&xas); } address_space->nrpages += nr; -- 2.19.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] page cache: Store only head pages in i_pages 2018-11-22 21:32 ` [PATCH 2/2] page cache: Store only head pages in i_pages Matthew Wilcox @ 2018-11-23 10:56 ` Kirill A. Shutemov 2018-11-23 17:19 ` Matthew Wilcox 0 siblings, 1 reply; 8+ messages in thread From: Kirill A. Shutemov @ 2018-11-23 10:56 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-mm, linux-fsdevel, linux-kernel, Kirill A. Shutemov, Hugh Dickins On Thu, Nov 22, 2018 at 01:32:24PM -0800, Matthew Wilcox wrote: > Transparent Huge Pages are currently stored in i_pages as pointers to > consecutive subpages. This patch changes that to storing consecutive > pointers to the head page in preparation for storing huge pages more > efficiently in i_pages. I probably miss something, I don't see how it wouldn't break split_huge_page(). I don't see what would replace head pages in i_pages with formerly-tail-pages? Hm? -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] page cache: Store only head pages in i_pages 2018-11-23 10:56 ` Kirill A. Shutemov @ 2018-11-23 17:19 ` Matthew Wilcox 2018-11-23 17:36 ` Kirill A. Shutemov 0 siblings, 1 reply; 8+ messages in thread From: Matthew Wilcox @ 2018-11-23 17:19 UTC (permalink / raw) To: Kirill A. Shutemov Cc: linux-mm, linux-fsdevel, linux-kernel, Kirill A. Shutemov, Hugh Dickins On Fri, Nov 23, 2018 at 01:56:44PM +0300, Kirill A. Shutemov wrote: > On Thu, Nov 22, 2018 at 01:32:24PM -0800, Matthew Wilcox wrote: > > Transparent Huge Pages are currently stored in i_pages as pointers to > > consecutive subpages. This patch changes that to storing consecutive > > pointers to the head page in preparation for storing huge pages more > > efficiently in i_pages. > > I probably miss something, I don't see how it wouldn't break > split_huge_page(). > > I don't see what would replace head pages in i_pages with > formerly-tail-pages? You're quite right. Where's your test-suite? ;-) I think this should do the job: +++ b/mm/huge_memory.c @@ -2464,6 +2464,9 @@ static void __split_huge_page(struct page *page, struct list_head *list, if (IS_ENABLED(CONFIG_SHMEM) && PageSwapBacked(head)) shmem_uncharge(head->mapping->host, 1); put_page(head + i); + } else if (!PageAnon(page)) { + __xa_store(&head->mapping->i_pages, head[i].index, + head + i, 0); } } Having looked at this area, I think there was actually a bug in the patch you wrote that I'm cribbing from. You inserted the tail pages before calling __split_huge_page_tail(), so a racing lookup would have found a tail page before it got transformed into a non-tail page. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] page cache: Store only head pages in i_pages 2018-11-23 17:19 ` Matthew Wilcox @ 2018-11-23 17:36 ` Kirill A. Shutemov 0 siblings, 0 replies; 8+ messages in thread From: Kirill A. Shutemov @ 2018-11-23 17:36 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-mm, linux-fsdevel, linux-kernel, Kirill A. Shutemov, Hugh Dickins On Fri, Nov 23, 2018 at 09:19:00AM -0800, Matthew Wilcox wrote: > On Fri, Nov 23, 2018 at 01:56:44PM +0300, Kirill A. Shutemov wrote: > > On Thu, Nov 22, 2018 at 01:32:24PM -0800, Matthew Wilcox wrote: > > > Transparent Huge Pages are currently stored in i_pages as pointers to > > > consecutive subpages. This patch changes that to storing consecutive > > > pointers to the head page in preparation for storing huge pages more > > > efficiently in i_pages. > > > > I probably miss something, I don't see how it wouldn't break > > split_huge_page(). > > > > I don't see what would replace head pages in i_pages with > > formerly-tail-pages? > > You're quite right. Where's your test-suite? ;-) Yeah-yeah... > I think this should do the job: > > +++ b/mm/huge_memory.c > @@ -2464,6 +2464,9 @@ static void __split_huge_page(struct page *page, struct list_head *list, > if (IS_ENABLED(CONFIG_SHMEM) && PageSwapBacked(head)) > shmem_uncharge(head->mapping->host, 1); > put_page(head + i); > + } else if (!PageAnon(page)) { > + __xa_store(&head->mapping->i_pages, head[i].index, > + head + i, 0); > } > } Looks good to me. But I still need to look into the rest of the patch. > Having looked at this area, I think there was actually a bug in the patch > you wrote that I'm cribbing from. You inserted the tail pages before > calling __split_huge_page_tail(), so a racing lookup would have found > a tail page before it got transformed into a non-tail page. I don't think so. The page still has refcount==0 and any lookup of the page suppose to fail due to !page_cache_get_speculative() or block on tree lock. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-11-23 17:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-22 21:32 [PATCH 0/2] Better support for THP in page cache Matthew Wilcox 2018-11-22 21:32 ` [PATCH 1/2] mm: Remove redundant test from find_get_pages_contig Matthew Wilcox 2018-11-23 10:47 ` Kirill A. Shutemov 2018-11-23 17:59 ` Matthew Wilcox 2018-11-22 21:32 ` [PATCH 2/2] page cache: Store only head pages in i_pages Matthew Wilcox 2018-11-23 10:56 ` Kirill A. Shutemov 2018-11-23 17:19 ` Matthew Wilcox 2018-11-23 17:36 ` Kirill A. Shutemov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).