Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 15.11.23 04:11, xu wrote:
> > > 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?
>
> I don't know the original reason, better ask Claudio Imbrenda <imbrenda@xxxxxxxxxxxxxxxxxx>.
> Maybe because doing detection that ahead of time will break several funtions' semantic,
> such as try_to_merge_two_pages(), try_to_merge_with_ksm_page() and try_to_merge_one_page()
>
> Adding the backup code don't change the old code and fixing the old problem, it's good.

It's absolutely counter-intuitive to check for something that cannot 
possibly work after the effects. This better has a good reason to make 
that code more complicated.
--
Cheers,

David / dhildenb