| Message ID | c76305d96d12d030a1a346b50503d148364246d2.1728901391.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: khugepaged: fix the incorrect statistics when collapsing large file folios | expand |
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>
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 >
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
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 >
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
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);
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(-)