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 <[email protected]>.
> 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
>>>> @@ -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?
Do you mean like this?
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2229,23 +2229,21 @@ 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;
+ bool SameCompound;
+ /*
+ * If they belongs to the same compound page, its' reference
+ * get twice, so need to put_page once to avoid that
+ * split_huge_page fails in try_to_merge_two_pages().
+ */
+ if (SameCompound = Is_SameCompound(page, tree_page))
+ put_page(tree_page);
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.
- */
- split = PageTransCompound(page)
- && compound_head(page) == compound_head(tree_page);
- put_page(tree_page);
+
+ if (!SameCompound)
+ put_page(tree_page);
+
if (kpage) {
/*
* The pages were successfully merged: insert new
@@ -2271,20 +2269,6 @@ 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) {
- /*
- * We are here if we tried to merge two pages and
- * failed because they both belonged to the same
- * compound page. We will split the page now, but no
- * merging will take place.
- * We do not want to add the cost of a full lock; if
- * the page is locked, it is better to skip it and
- * perhaps try again later.
- */
- if (!trylock_page(page))
- return;
- split_huge_page(page);
- unlock_page(page);
}
}
}
>>
>> I don't know the original reason, better ask Claudio Imbrenda <[email protected]>.
>> 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.
>--
On 16.11.23 13:17, xu wrote:
>>>>> @@ -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?
>
> Do you mean like this?
Let me see if the refcounting here is sane:
(a) The caller has a reference on "page" that it will put just after the
cmp_and_merge_page() call.
(b) unstable_tree_search_insert() obtained a reference to the
"tree_page" using get_mergeable_page()->follow_page(). We will put
that reference.
So indeed, if both references are to the same folio, *we* have two
references to the folio and can safely drop one of both.
>
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2229,23 +2229,21 @@ 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;
> + bool SameCompound;
> + /*
> + * If they belongs to the same compound page, its' reference
> + * get twice, so need to put_page once to avoid that
> + * split_huge_page fails in try_to_merge_two_pages().
> + */
> + if (SameCompound = Is_SameCompound(page, tree_page))
> + put_page(tree_page);
>
bool same_folio = page_folio(page) == page_folio(tree_page);
/*
* If both pages belong to the same folio, we are holding two references
* to the same large folio: splitting the folio in
* try_to_merge_one_page() will fail for that reason. So let's just drop
* one reference early. Note that this is only possible if tree_page is
* not a KSM page yet.
*/
if (same_folio)
put_page(tree_page);
[we could use more folio operations here, but lets KIS]
--
Cheers,
David / dhildenb
On 16.11.23 18:39, David Hildenbrand wrote:
> On 16.11.23 13:17, xu wrote:
>>>>>> @@ -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?
>>
>> Do you mean like this?
>
> Let me see if the refcounting here is sane:
>
> (a) The caller has a reference on "page" that it will put just after the
> cmp_and_merge_page() call.
> (b) unstable_tree_search_insert() obtained a reference to the
> "tree_page" using get_mergeable_page()->follow_page(). We will put
> that reference.
>
> So indeed, if both references are to the same folio, *we* have two
> references to the folio and can safely drop one of both.
>
>>
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -2229,23 +2229,21 @@ 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;
>> + bool SameCompound;
>> + /*
>> + * If they belongs to the same compound page, its' reference
>> + * get twice, so need to put_page once to avoid that
>> + * split_huge_page fails in try_to_merge_two_pages().
>> + */
>> + if (SameCompound = Is_SameCompound(page, tree_page))
>> + put_page(tree_page);
>>
>
> bool same_folio = page_folio(page) == page_folio(tree_page);
>
> /*
> * If both pages belong to the same folio, we are holding two references
> * to the same large folio: splitting the folio in
> * try_to_merge_one_page() will fail for that reason. So let's just drop
> * one reference early. Note that this is only possible if tree_page is
> * not a KSM page yet.
> */
> if (same_folio)
> put_page(tree_page);
>
> [we could use more folio operations here, but lets KIS]
>
Looking into the details, that doesn't work unfortunately.
try_to_merge_one_page() will call split_huge_page(), but that
will only hold a reference to "page", not to "tree page" after the
split.
Long story short, after split_huge_page() tree_page could get freed
because we don't hold a reference to that one anymore.
So we most probably cannot do better than the following (untested):
diff --git a/mm/ksm.c b/mm/ksm.c
index 7efcc68ccc6e..afb079524585 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2226,25 +2226,30 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
if (!err)
return;
}
+retry:
tree_rmap_item =
unstable_tree_search_insert(rmap_item, page, &tree_page);
if (tree_rmap_item) {
- bool split;
+ /*
+ * If both pages belong to the same folio, we are holding two
+ * references to the same large folio: splitting the folio in
+ * try_to_merge_one_page() will fail for that reason.
+ *
+ * We have to split manually, and lookup the tree_page
+ * again. Note that we'll only hold a reference to "page" after
+ * the split.
+ */
+ if (page_folio(page) == page_folio(tree_page)) {
+ put_page(tree_page);
+
+ if (!trylock_page(page))
+ return;
+ split_huge_page(page);
+ unlock_page(page);
+ goto retry;
+ }
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.
- */
- split = PageTransCompound(page)
- && compound_head(page) == compound_head(tree_page);
put_page(tree_page);
if (kpage) {
/*
@@ -2271,20 +2276,6 @@ 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) {
- /*
- * We are here if we tried to merge two pages and
- * failed because they both belonged to the same
- * compound page. We will split the page now, but no
- * merging will take place.
- * We do not want to add the cost of a full lock; if
- * the page is locked, it is better to skip it and
- * perhaps try again later.
- */
- if (!trylock_page(page))
- return;
- split_huge_page(page);
- unlock_page(page);
}
}
}
--
Cheers,
David / dhildenb