| Message ID | 20231107135216.415926-2-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: cleanup and use more folio in page fault | expand |
On Tue, Nov 07, 2023 at 09:52:11PM +0800, Kefeng Wang wrote: > struct page *ksm_might_need_to_copy(struct page *page, > - struct vm_area_struct *vma, unsigned long address) > + struct vm_area_struct *vma, unsigned long addr) > { > struct folio *folio = page_folio(page); > struct anon_vma *anon_vma = folio_anon_vma(folio); > - struct page *new_page; > + struct folio *new_folio; > > - if (PageKsm(page)) { > - if (page_stable_node(page) && > + if (folio_test_ksm(folio)) { > + if (folio_stable_node(folio) && > !(ksm_run & KSM_RUN_UNMERGE)) > return page; /* no need to copy it */ > } else if (!anon_vma) { > return page; /* no need to copy it */ > - } else if (page->index == linear_page_index(vma, address) && > + } else if (page->index == linear_page_index(vma, addr) && Hmm. page->index is going away. What should we do here instead? The rest of this looks good.
On 2023/11/7 22:24, Matthew Wilcox wrote: > On Tue, Nov 07, 2023 at 09:52:11PM +0800, Kefeng Wang wrote: >> struct page *ksm_might_need_to_copy(struct page *page, >> - struct vm_area_struct *vma, unsigned long address) >> + struct vm_area_struct *vma, unsigned long addr) >> { >> struct folio *folio = page_folio(page); >> struct anon_vma *anon_vma = folio_anon_vma(folio); >> - struct page *new_page; >> + struct folio *new_folio; >> >> - if (PageKsm(page)) { >> - if (page_stable_node(page) && >> + if (folio_test_ksm(folio)) { >> + if (folio_stable_node(folio) && >> !(ksm_run & KSM_RUN_UNMERGE)) >> return page; /* no need to copy it */ >> } else if (!anon_vma) { >> return page; /* no need to copy it */ >> - } else if (page->index == linear_page_index(vma, address) && >> + } else if (page->index == linear_page_index(vma, addr) && > > Hmm. page->index is going away. What should we do here instead? Do you mean to replace page->index to folio->index, or kill index from struct page? > > The rest of this looks good. >
On Wed, Nov 08, 2023 at 09:40:09AM +0800, Kefeng Wang wrote: > > > On 2023/11/7 22:24, Matthew Wilcox wrote: > > On Tue, Nov 07, 2023 at 09:52:11PM +0800, Kefeng Wang wrote: > > > struct page *ksm_might_need_to_copy(struct page *page, > > > - struct vm_area_struct *vma, unsigned long address) > > > + struct vm_area_struct *vma, unsigned long addr) > > > { > > > struct folio *folio = page_folio(page); > > > struct anon_vma *anon_vma = folio_anon_vma(folio); > > > - struct page *new_page; > > > + struct folio *new_folio; > > > - if (PageKsm(page)) { > > > - if (page_stable_node(page) && > > > + if (folio_test_ksm(folio)) { > > > + if (folio_stable_node(folio) && > > > !(ksm_run & KSM_RUN_UNMERGE)) > > > return page; /* no need to copy it */ > > > } else if (!anon_vma) { > > > return page; /* no need to copy it */ > > > - } else if (page->index == linear_page_index(vma, address) && > > > + } else if (page->index == linear_page_index(vma, addr) && > > > > Hmm. page->index is going away. What should we do here instead? > > Do you mean to replace page->index to folio->index, or kill index from > struct page? I'm asking you what we should do. Tail pages already don't have a valid ->index (or ->mapping). So presumably we can't see a tail page here today. But will we in future? Just to remind you, the goal here is: struct page { unsigned long memdesc; }; so folios will be the only thing that have a ->index. I haven't looked at this code; I know nothing about it. But you're changing it, so you must have some understanding of it.
On 2023/11/8 21:59, Matthew Wilcox wrote: > On Wed, Nov 08, 2023 at 09:40:09AM +0800, Kefeng Wang wrote: >> >> >> On 2023/11/7 22:24, Matthew Wilcox wrote: >>> On Tue, Nov 07, 2023 at 09:52:11PM +0800, Kefeng Wang wrote: >>>> struct page *ksm_might_need_to_copy(struct page *page, >>>> - struct vm_area_struct *vma, unsigned long address) >>>> + struct vm_area_struct *vma, unsigned long addr) >>>> { >>>> struct folio *folio = page_folio(page); >>>> struct anon_vma *anon_vma = folio_anon_vma(folio); >>>> - struct page *new_page; >>>> + struct folio *new_folio; >>>> - if (PageKsm(page)) { >>>> - if (page_stable_node(page) && >>>> + if (folio_test_ksm(folio)) { >>>> + if (folio_stable_node(folio) && >>>> !(ksm_run & KSM_RUN_UNMERGE)) >>>> return page; /* no need to copy it */ >>>> } else if (!anon_vma) { >>>> return page; /* no need to copy it */ >>>> - } else if (page->index == linear_page_index(vma, address) && >>>> + } else if (page->index == linear_page_index(vma, addr) && >>> >>> Hmm. page->index is going away. What should we do here instead? >> >> Do you mean to replace page->index to folio->index, or kill index from >> struct page? > > I'm asking you what we should do. > > Tail pages already don't have a valid ->index (or ->mapping). > So presumably we can't see a tail page here today. But will we in future? I think we could replace page->index to page_to_pgoff(page). > > Just to remind you, the goal here is: > > struct page { > unsigned long memdesc; > }; > Get your point, that will be great. > so folios will be the only thing that have a ->index. I haven't looked > at this code; I know nothing about it. But you're changing it, so you > must have some understanding of it. >
On 09.11.23 08:09, Kefeng Wang wrote: > > > On 2023/11/8 21:59, Matthew Wilcox wrote: >> On Wed, Nov 08, 2023 at 09:40:09AM +0800, Kefeng Wang wrote: >>> >>> >>> On 2023/11/7 22:24, Matthew Wilcox wrote: >>>> On Tue, Nov 07, 2023 at 09:52:11PM +0800, Kefeng Wang wrote: >>>>> struct page *ksm_might_need_to_copy(struct page *page, >>>>> - struct vm_area_struct *vma, unsigned long address) >>>>> + struct vm_area_struct *vma, unsigned long addr) >>>>> { >>>>> struct folio *folio = page_folio(page); >>>>> struct anon_vma *anon_vma = folio_anon_vma(folio); >>>>> - struct page *new_page; >>>>> + struct folio *new_folio; >>>>> - if (PageKsm(page)) { >>>>> - if (page_stable_node(page) && >>>>> + if (folio_test_ksm(folio)) { >>>>> + if (folio_stable_node(folio) && >>>>> !(ksm_run & KSM_RUN_UNMERGE)) >>>>> return page; /* no need to copy it */ >>>>> } else if (!anon_vma) { >>>>> return page; /* no need to copy it */ >>>>> - } else if (page->index == linear_page_index(vma, address) && >>>>> + } else if (page->index == linear_page_index(vma, addr) && >>>> >>>> Hmm. page->index is going away. What should we do here instead? >>> >>> Do you mean to replace page->index to folio->index, or kill index from >>> struct page? >> >> I'm asking you what we should do. >> >> Tail pages already don't have a valid ->index (or ->mapping). >> So presumably we can't see a tail page here today. But will we in future? > > I think we could replace page->index to page_to_pgoff(page). What the second part of that code does is check whether a page might have been a KSM page before swapout. Once a KSM page is swapped out, we lose the KSM marker. To recover, we have to check whether the new page logically "fits" into the VMA. Large folios are never KSM folios, and we only swap in small folios (and in the future, once we would swap in large folios, they couldn't have been KSM folios before). So you could return early in the function if we have a large folio and make all operations based on the (small) folio.
On 2023/11/13 16:32, David Hildenbrand wrote: > On 09.11.23 08:09, Kefeng Wang wrote: >> >> >> On 2023/11/8 21:59, Matthew Wilcox wrote: >>> On Wed, Nov 08, 2023 at 09:40:09AM +0800, Kefeng Wang wrote: >>>> >>>> >>>> On 2023/11/7 22:24, Matthew Wilcox wrote: >>>>> On Tue, Nov 07, 2023 at 09:52:11PM +0800, Kefeng Wang wrote: >>>>>> struct page *ksm_might_need_to_copy(struct page *page, >>>>>> - struct vm_area_struct *vma, unsigned long address) >>>>>> + struct vm_area_struct *vma, unsigned long addr) >>>>>> { >>>>>> struct folio *folio = page_folio(page); >>>>>> struct anon_vma *anon_vma = folio_anon_vma(folio); >>>>>> - struct page *new_page; >>>>>> + struct folio *new_folio; >>>>>> - if (PageKsm(page)) { >>>>>> - if (page_stable_node(page) && >>>>>> + if (folio_test_ksm(folio)) { >>>>>> + if (folio_stable_node(folio) && >>>>>> !(ksm_run & KSM_RUN_UNMERGE)) >>>>>> return page; /* no need to copy it */ >>>>>> } else if (!anon_vma) { >>>>>> return page; /* no need to copy it */ >>>>>> - } else if (page->index == linear_page_index(vma, address) && >>>>>> + } else if (page->index == linear_page_index(vma, addr) && >>>>> >>>>> Hmm. page->index is going away. What should we do here instead? >>>> >>>> Do you mean to replace page->index to folio->index, or kill index from >>>> struct page? >>> >>> I'm asking you what we should do. >>> >>> Tail pages already don't have a valid ->index (or ->mapping). >>> So presumably we can't see a tail page here today. But will we in >>> future? >> >> I think we could replace page->index to page_to_pgoff(page). > > What the second part of that code does is check whether a page might > have been a KSM page before swapout. > > Once a KSM page is swapped out, we lose the KSM marker. To recover, we > have to check whether the new page logically "fits" into the VMA. > > Large folios are never KSM folios, and we only swap in small folios (and > in the future, once we would swap in large folios, they couldn't have > been KSM folios before). > > So you could return early in the function if we have a large folio and > make all operations based on the (small) folio. Sure, I will add folio_test_large check ahead and convert page->index to folio->index, and adjust the logical if ksm and swapin support large folio, thanks.
diff --git a/include/linux/ksm.h b/include/linux/ksm.h index c2dd786a30e1..4643d5244e77 100644 --- a/include/linux/ksm.h +++ b/include/linux/ksm.h @@ -77,7 +77,7 @@ static inline void ksm_exit(struct mm_struct *mm) * but what if the vma was unmerged while the page was swapped out? */ struct page *ksm_might_need_to_copy(struct page *page, - struct vm_area_struct *vma, unsigned long address); + struct vm_area_struct *vma, unsigned long addr); void rmap_walk_ksm(struct folio *folio, struct rmap_walk_control *rwc); void folio_migrate_ksm(struct folio *newfolio, struct folio *folio); @@ -130,7 +130,7 @@ static inline int ksm_madvise(struct vm_area_struct *vma, unsigned long start, } static inline struct page *ksm_might_need_to_copy(struct page *page, - struct vm_area_struct *vma, unsigned long address) + struct vm_area_struct *vma, unsigned long addr) { return page; } diff --git a/mm/ksm.c b/mm/ksm.c index 7efcc68ccc6e..e5b8b677e2de 100644 --- a/mm/ksm.c +++ b/mm/ksm.c @@ -2876,48 +2876,48 @@ void __ksm_exit(struct mm_struct *mm) } struct page *ksm_might_need_to_copy(struct page *page, - struct vm_area_struct *vma, unsigned long address) + struct vm_area_struct *vma, unsigned long addr) { struct folio *folio = page_folio(page); struct anon_vma *anon_vma = folio_anon_vma(folio); - struct page *new_page; + struct folio *new_folio; - if (PageKsm(page)) { - if (page_stable_node(page) && + if (folio_test_ksm(folio)) { + if (folio_stable_node(folio) && !(ksm_run & KSM_RUN_UNMERGE)) return page; /* no need to copy it */ } else if (!anon_vma) { return page; /* no need to copy it */ - } else if (page->index == linear_page_index(vma, address) && + } else if (page->index == linear_page_index(vma, addr) && anon_vma->root == vma->anon_vma->root) { return page; /* still no need to copy it */ } if (PageHWPoison(page)) return ERR_PTR(-EHWPOISON); - if (!PageUptodate(page)) + if (!folio_test_uptodate(folio)) return page; /* let do_swap_page report the error */ - new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address); - if (new_page && - mem_cgroup_charge(page_folio(new_page), vma->vm_mm, GFP_KERNEL)) { - put_page(new_page); - new_page = NULL; + new_folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, addr, false); + if (new_folio && + mem_cgroup_charge(new_folio, vma->vm_mm, GFP_KERNEL)) { + folio_put(new_folio); + new_folio = NULL; } - if (new_page) { - if (copy_mc_user_highpage(new_page, page, address, vma)) { - put_page(new_page); + if (new_folio) { + if (copy_mc_user_highpage(&new_folio->page, page, addr, vma)) { + folio_put(new_folio); memory_failure_queue(page_to_pfn(page), 0); return ERR_PTR(-EHWPOISON); } - SetPageDirty(new_page); - __SetPageUptodate(new_page); - __SetPageLocked(new_page); + folio_set_dirty(new_folio); + __folio_mark_uptodate(new_folio); + __folio_set_locked(new_folio); #ifdef CONFIG_SWAP count_vm_event(KSM_SWPIN_COPY); #endif } - return new_page; + return new_folio ? &new_folio->page : NULL; } void rmap_walk_ksm(struct folio *folio, struct rmap_walk_control *rwc)
Convert ksm_might_need_to_copy() to use more folio api to save nine compound_head() calls, short 'address' to reduce max-line-length. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- include/linux/ksm.h | 4 ++-- mm/ksm.c | 36 ++++++++++++++++++------------------ 2 files changed, 20 insertions(+), 20 deletions(-)