Messages in this thread | | | Date | Tue, 14 Nov 2023 15:43:06 +0100 | Subject | Re: [PATCH] ksm: delay the check of splitting compound pages | From | David Hildenbrand <> |
| |
On 14.11.23 13:36, yang.yang29@zte.com.cn wrote: > From: xu xin <xu.xin16@zte.com.cn> > > Background > ========== > When trying to merge two pages, it may fail because the two pages > belongs to the same compound page and split_huge_page fails due to > the incorrect reference to the page. To solve the problem, the commit > 77da2ba0648a4 ("mm/ksm: fix interaction with THP") tries to split the > compound page after try_to_merge_two_pages() fails and put_page in > that case. However it is too early to calculate of the variable 'split' which > indicates whether the two pages belongs to the same compound page. > > What to do > ========== > If try_to_merge_two_pages() succeeds, there is no need to check whether > to splitting compound pages. So we delay the check of splitting compound > pages until try_to_merge_two_pages() fails, which can improve the > processing efficiency of cmp_and_merge_page() a little. > > Signed-off-by: xu xin <xu.xin16@zte.com.cn> > Reviewed-by: Yang Yang <yang.yang29@zte.com.cn> > --- > mm/ksm.c | 36 ++++++++++++++++++++---------------- > 1 file changed, 20 insertions(+), 16 deletions(-) > > diff --git a/mm/ksm.c b/mm/ksm.c > index 7efcc68ccc6e..c952fe5d9e43 100644 > --- a/mm/ksm.c > +++ b/mm/ksm.c > @@ -2229,24 +2229,10 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite > tree_rmap_item = > unstable_tree_search_insert(rmap_item, page, &tree_page); > if (tree_rmap_item) { > - bool split; > - > kpage = try_to_merge_two_pages(rmap_item, page, > tree_rmap_item, tree_page); > - /* > - * If both pages we tried to merge belong to the same compound > - * page, then we actually ended up increasing the reference > - * count of the same compound page twice, and split_huge_page > - * failed. > - * Here we set a flag if that happened, and we use it later to > - * try split_huge_page again. Since we call put_page right > - * afterwards, the reference count will be correct and > - * split_huge_page should succeed. > - */
I'm curious, why can't we detect that ahead of time and keep only a single reference? Why do we need the backup code? Anything I am missing?
> - split = PageTransCompound(page) > - && compound_head(page) == compound_head(tree_page); > - put_page(tree_page); > if (kpage) { > + put_page(tree_page); > /* > * The pages were successfully merged: insert new > * node in the stable tree and add both rmap_items. > @@ -2271,7 +2257,25 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite > break_cow(tree_rmap_item); > break_cow(rmap_item); > } > - } else if (split) { > + } else { > + bool split; > + /* > + * If both pages we tried to merge belong to the same compound > + * page, then we actually ended up increasing the reference > + * count of the same compound page twice, and split_huge_page > + * failed. > + * Here we set a flag if that happened, and we use it later to > + * try split_huge_page again. Since we call put_page right > + * afterwards, the reference count will be correct and > + * split_huge_page should succeed. > + */ > + > + split = PageTransCompound(page) > + && compound_head(page) == compound_head(tree_page);
Would
split = page_folio(page) == page_folio(tree_page);
do the trick? No need to mess with compound pages.
-- Cheers,
David / dhildenb
| |