1. Patchew
  2. linux
  3. View series

[PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios

Baolin Wang posted 1 patch 1 month, 1 week ago 
mm/khugepaged.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios
Posted by Baolin Wang 1 month, 1 week ago 
Khugepaged already supports collapsing file large folios (including shmem mTHP)
by commit 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse"), and the
control parameters in khugepaged: 'khugepaged_max_ptes_swap' and 'khugepaged_max_ptes_none',
still compare based on PTE granularity to determine whether a file collapse is
needed. However, the statistics for 'present' and 'swap' in hpage_collapse_scan_file()
do not take into account the large folios, which may lead to incorrect judgments
regarding the khugepaged_max_ptes_swap/none parameters, resulting in unnecessary
file collapses.

To fix this issue, take into account the large folios' statistics for 'present'
and 'swap' variables in the hpage_collapse_scan_file().

Fixes: 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse")
Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/khugepaged.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index ba28ba09fe89..6f8d46d107b4 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2267,7 +2267,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
 			continue;
 
 		if (xa_is_value(folio)) {
-			++swap;
+			swap += 1 << xas_get_order(&xas);
 			if (cc->is_khugepaged &&
 			    swap > khugepaged_max_ptes_swap) {
 				result = SCAN_EXCEED_SWAP_PTE;
@@ -2314,7 +2314,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
 		 * is just too costly...
 		 */
 
-		present++;
+		present += folio_nr_pages(folio);
 
 		if (need_resched()) {
 			xas_pause(&xas);
-- 
2.39.3
Re: [PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios
Posted by Yang Shi 1 month, 1 week ago 
On Mon, Oct 14, 2024 at 3:25 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Khugepaged already supports collapsing file large folios (including shmem mTHP)
> by commit 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse"), and the
> control parameters in khugepaged: 'khugepaged_max_ptes_swap' and 'khugepaged_max_ptes_none',
> still compare based on PTE granularity to determine whether a file collapse is
> needed. However, the statistics for 'present' and 'swap' in hpage_collapse_scan_file()
> do not take into account the large folios, which may lead to incorrect judgments
> regarding the khugepaged_max_ptes_swap/none parameters, resulting in unnecessary
> file collapses.
>
> To fix this issue, take into account the large folios' statistics for 'present'
> and 'swap' variables in the hpage_collapse_scan_file().
>
> Fixes: 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Reviewed-by: Yang Shi <shy828301@gmail.com>

> ---
>  mm/khugepaged.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ba28ba09fe89..6f8d46d107b4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2267,7 +2267,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>                         continue;
>
>                 if (xa_is_value(folio)) {
> -                       ++swap;
> +                       swap += 1 << xas_get_order(&xas);
>                         if (cc->is_khugepaged &&
>                             swap > khugepaged_max_ptes_swap) {
>                                 result = SCAN_EXCEED_SWAP_PTE;
> @@ -2314,7 +2314,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>                  * is just too costly...
>                  */
>
> -               present++;
> +               present += folio_nr_pages(folio);
>
>                 if (need_resched()) {
>                         xas_pause(&xas);
> --
> 2.39.3
>
Re: [PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios
Posted by Zi Yan 1 month, 1 week ago 
On 14 Oct 2024, at 6:24, Baolin Wang wrote:

> Khugepaged already supports collapsing file large folios (including shmem mTHP)
> by commit 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse"), and the
> control parameters in khugepaged: 'khugepaged_max_ptes_swap' and 'khugepaged_max_ptes_none',
> still compare based on PTE granularity to determine whether a file collapse is
> needed. However, the statistics for 'present' and 'swap' in hpage_collapse_scan_file()
> do not take into account the large folios, which may lead to incorrect judgments
> regarding the khugepaged_max_ptes_swap/none parameters, resulting in unnecessary
> file collapses.
>
> To fix this issue, take into account the large folios' statistics for 'present'
> and 'swap' variables in the hpage_collapse_scan_file().
>
> Fixes: 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/khugepaged.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ba28ba09fe89..6f8d46d107b4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2267,7 +2267,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>  			continue;
>
>  		if (xa_is_value(folio)) {
> -			++swap;
> +			swap += 1 << xas_get_order(&xas);

xas_get_order() scans xarry sibling entries to get the order. You probably
can call xas_get_order() outside the loop after rcu_read_lock() and use
the result inside, since xas order is not changed, right?

>  			if (cc->is_khugepaged &&
>  			    swap > khugepaged_max_ptes_swap) {
>  				result = SCAN_EXCEED_SWAP_PTE;
> @@ -2314,7 +2314,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>  		 * is just too costly...
>  		 */
>
> -		present++;
> +		present += folio_nr_pages(folio);
>
>  		if (need_resched()) {
>  			xas_pause(&xas);
> -- 
> 2.39.3

Otherwise, LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi
Re: [PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios
Posted by Zi Yan 1 month, 1 week ago 
On 14 Oct 2024, at 11:42, Zi Yan wrote:

> On 14 Oct 2024, at 6:24, Baolin Wang wrote:
>
>> Khugepaged already supports collapsing file large folios (including shmem mTHP)
>> by commit 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse"), and the
>> control parameters in khugepaged: 'khugepaged_max_ptes_swap' and 'khugepaged_max_ptes_none',
>> still compare based on PTE granularity to determine whether a file collapse is
>> needed. However, the statistics for 'present' and 'swap' in hpage_collapse_scan_file()
>> do not take into account the large folios, which may lead to incorrect judgments
>> regarding the khugepaged_max_ptes_swap/none parameters, resulting in unnecessary
>> file collapses.
>>
>> To fix this issue, take into account the large folios' statistics for 'present'
>> and 'swap' variables in the hpage_collapse_scan_file().
>>
>> Fixes: 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse")
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>  mm/khugepaged.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index ba28ba09fe89..6f8d46d107b4 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2267,7 +2267,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>>  			continue;
>>
>>  		if (xa_is_value(folio)) {
>> -			++swap;
>> +			swap += 1 << xas_get_order(&xas);
>
> xas_get_order() scans xarry sibling entries to get the order. You probably
> can call xas_get_order() outside the loop after rcu_read_lock() and use
> the result inside, since xas order is not changed, right?

Forget about this. Order can change. Sorry for the noise.

>
>>  			if (cc->is_khugepaged &&
>>  			    swap > khugepaged_max_ptes_swap) {
>>  				result = SCAN_EXCEED_SWAP_PTE;
>> @@ -2314,7 +2314,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>>  		 * is just too costly...
>>  		 */
>>
>> -		present++;
>> +		present += folio_nr_pages(folio);
>>
>>  		if (need_resched()) {
>>  			xas_pause(&xas);
>> -- 
>> 2.39.3
>
> Otherwise, LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>

Feel free to take this RB.


Best Regards,
Yan, Zi
Re: [PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios
Posted by Barry Song 1 month, 1 week ago 
On Mon, Oct 14, 2024 at 6:25 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Khugepaged already supports collapsing file large folios (including shmem mTHP)
> by commit 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse"), and the
> control parameters in khugepaged: 'khugepaged_max_ptes_swap' and 'khugepaged_max_ptes_none',
> still compare based on PTE granularity to determine whether a file collapse is
> needed. However, the statistics for 'present' and 'swap' in hpage_collapse_scan_file()
> do not take into account the large folios, which may lead to incorrect judgments
> regarding the khugepaged_max_ptes_swap/none parameters, resulting in unnecessary
> file collapses.
>
> To fix this issue, take into account the large folios' statistics for 'present'
> and 'swap' variables in the hpage_collapse_scan_file().
>
> Fixes: 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Reviewed-by: Barry Song <baohua@kernel.org>

> ---
>  mm/khugepaged.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ba28ba09fe89..6f8d46d107b4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2267,7 +2267,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>                         continue;
>
>                 if (xa_is_value(folio)) {
> -                       ++swap;
> +                       swap += 1 << xas_get_order(&xas);
>                         if (cc->is_khugepaged &&
>                             swap > khugepaged_max_ptes_swap) {
>                                 result = SCAN_EXCEED_SWAP_PTE;
> @@ -2314,7 +2314,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>                  * is just too costly...
>                  */
>
> -               present++;
> +               present += folio_nr_pages(folio);
>
>                 if (need_resched()) {
>                         xas_pause(&xas);
> --
> 2.39.3
>
Re: [PATCH] mm: khugepaged: fix the incorrect statistics when collapsing large file folios
Posted by David Hildenbrand 1 month, 1 week ago 
On 14.10.24 12:24, Baolin Wang wrote:
> Khugepaged already supports collapsing file large folios (including shmem mTHP)
> by commit 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse"), and the
> control parameters in khugepaged: 'khugepaged_max_ptes_swap' and 'khugepaged_max_ptes_none',
> still compare based on PTE granularity to determine whether a file collapse is
> needed. However, the statistics for 'present' and 'swap' in hpage_collapse_scan_file()
> do not take into account the large folios, which may lead to incorrect judgments
> regarding the khugepaged_max_ptes_swap/none parameters, resulting in unnecessary
> file collapses.
> 
> To fix this issue, take into account the large folios' statistics for 'present'
> and 'swap' variables in the hpage_collapse_scan_file().
> 
> Fixes: 7de856ffd007 ("mm: khugepaged: support shmem mTHP collapse")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>   mm/khugepaged.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ba28ba09fe89..6f8d46d107b4 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2267,7 +2267,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>   			continue;
>   
>   		if (xa_is_value(folio)) {
> -			++swap;
> +			swap += 1 << xas_get_order(&xas);
>   			if (cc->is_khugepaged &&
>   			    swap > khugepaged_max_ptes_swap) {
>   				result = SCAN_EXCEED_SWAP_PTE;
> @@ -2314,7 +2314,7 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>   		 * is just too costly...
>   		 */
>   
> -		present++;
> +		present += folio_nr_pages(folio);
>   
>   		if (need_resched()) {
>   			xas_pause(&xas);

Looks correct to me:

Acked-by: David Hildenbrand <david@redhat.com>


-- 
Cheers,

David / dhildenb

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.