Hi,
As Mike pointed out [1], the huge_ptep_get() will only return one specific
pte value for the CONT-PTE or CONT-PMD size hugetlb on ARM64 system, which
will not take into account the subpages' dirty or young bits of a CONT-PTE/PMD
size hugetlb page. That will make us miss dirty or young flags of a CONT-PTE/PMD
size hugetlb page for those functions that want to check the dirty or
young flags of a hugetlb page. For example, the gather_hugetlb_stats() will
get inaccurate dirty hugetlb page statistics, and the DAMON for hugetlb monitoring
will also get inaccurate access statistics.
To fix this issue, one approach is that we can define an ARM64 specific huge_ptep_get()
implementation, which will take into account any subpages' dirty or young bits.
However we should add a new parameter for ARM64 specific huge_ptep_get() to check
how many continuous PTEs or PMDs in this CONT-PTE/PMD size hugetlb, that means we
should convert all the places using huge_ptep_get(), meanwhile most places using
huge_ptep_get() did not care about the dirty or young flags at all.
So instead of changing the prototype of huge_ptep_get(), this patch set introduces
a new huge_ptep_get_access_flags() interface and define an ARM64 specific implementation,
that will take into account any subpages' dirty or young bits for CONT-PTE/PMD size
hugetlb page. And we can only change to use huge_ptep_get_access_flags() for those
functions that care about the dirty or young flags of a hugetlb page.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Baolin Wang (3):
arm64/hugetlb: Introduce new huge_ptep_get_access_flags() interface
fs/proc/task_mmu: Change to use huge_ptep_get_access_flags()
mm/damon/vaddr: Change to use huge_ptep_get_access_flags()
arch/arm64/include/asm/hugetlb.h | 2 ++
arch/arm64/mm/hugetlbpage.c | 24 ++++++++++++++++++++++++
fs/proc/task_mmu.c | 3 ++-
include/asm-generic/hugetlb.h | 7 +++++++
mm/damon/vaddr.c | 5 +++--
5 files changed, 38 insertions(+), 3 deletions(-)
--
1.8.3.1
The ARM64 platform can support CONT-PTE/PMD size hugetlb, which can
contain seravel continuous pte or pmd entries. However current
huge_ptep_get() only return one specific pte value for the CONT-PTE
or CONT-PMD size hugetlb, which did not take into accounts the
subpages' dirty or young flags. So the gather_hugetlb_stats()
will miss some dirty hugetlb statistics.
Thus change to use huge_ptep_get_access_flags() taking into accounts
the subpages' dirty or young flags of a CONT-PTE/PMD size hugetlb,
to make the hugetlb statistics more accurate.
Signed-off-by: Baolin Wang <[email protected]>
---
fs/proc/task_mmu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f9c9abb..3f224a7 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1880,7 +1880,8 @@ static int gather_pte_stats(pmd_t *pmd, unsigned long addr,
static int gather_hugetlb_stats(pte_t *pte, unsigned long hmask,
unsigned long addr, unsigned long end, struct mm_walk *walk)
{
- pte_t huge_pte = huge_ptep_get(pte);
+ pte_t huge_pte = huge_ptep_get_access_flags(pte,
+ huge_page_size(hstate_vma(walk->vma)));
struct numa_maps *md;
struct page *page;
--
1.8.3.1
On 5/8/2022 11:26 PM, Muchun Song wrote:
> On Sun, May 08, 2022 at 04:58:51PM +0800, Baolin Wang wrote:
>> Hi,
>>
>> As Mike pointed out [1], the huge_ptep_get() will only return one specific
>> pte value for the CONT-PTE or CONT-PMD size hugetlb on ARM64 system, which
>> will not take into account the subpages' dirty or young bits of a CONT-PTE/PMD
>> size hugetlb page. That will make us miss dirty or young flags of a CONT-PTE/PMD
>> size hugetlb page for those functions that want to check the dirty or
>> young flags of a hugetlb page. For example, the gather_hugetlb_stats() will
>> get inaccurate dirty hugetlb page statistics, and the DAMON for hugetlb monitoring
>> will also get inaccurate access statistics.
>>
>> To fix this issue, one approach is that we can define an ARM64 specific huge_ptep_get()
>> implementation, which will take into account any subpages' dirty or young bits.
>
> IIUC, we could get the page size by page_size(pte_page(pte)).
> So, how about the following implementation of huge_ptep_get()?
> Does this work for you?
>
> pte_t huge_ptep_get(pte_t *ptep)
> {
> int ncontig, i;
> size_t pgsize;
> pte_t orig_pte = ptep_get(ptep);
>
> if (!pte_present(orig_pte) || !pte_cont(orig_pte))
> return orig_pte;
>
> ncontig = num_contig_ptes(page_size(pte_page(orig_pte)), &pgsize);
>
> for (i = 0; i < ncontig; i++, ptep++) {
> pte_t pte = ptep_get(ptep);
>
> if (pte_dirty(pte))
> orig_pte = pte_mkdirty(orig_pte);
>
> if (pte_young(pte))
> orig_pte = pte_mkyoung(orig_pte);
> }
>
> return orig_pte;
> }
Thanks for your suggestion, and I think this works for me and looks more
straight forward in case some functions using huge_ptep_get() will care
about the young or dirty bits in future.
My only concern is that all the functions using huge_ptep_get() will set
a contPTE dirty or accessed bit, however most functions do not care
about the dirty and accessed bit, which becomes a bit more expensive for
them? Also mentioned by Matthew in his comments. Anyway, I still think
your suggestion is straight forward and I can change in next version if
no other objections.
On Sun, May 08, 2022 at 04:58:51PM +0800, Baolin Wang wrote:
> Hi,
>
> As Mike pointed out [1], the huge_ptep_get() will only return one specific
> pte value for the CONT-PTE or CONT-PMD size hugetlb on ARM64 system, which
> will not take into account the subpages' dirty or young bits of a CONT-PTE/PMD
> size hugetlb page. That will make us miss dirty or young flags of a CONT-PTE/PMD
> size hugetlb page for those functions that want to check the dirty or
> young flags of a hugetlb page. For example, the gather_hugetlb_stats() will
> get inaccurate dirty hugetlb page statistics, and the DAMON for hugetlb monitoring
> will also get inaccurate access statistics.
>
> To fix this issue, one approach is that we can define an ARM64 specific huge_ptep_get()
> implementation, which will take into account any subpages' dirty or young bits.
IIUC, we could get the page size by page_size(pte_page(pte)).
So, how about the following implementation of huge_ptep_get()?
Does this work for you?
pte_t huge_ptep_get(pte_t *ptep)
{
int ncontig, i;
size_t pgsize;
pte_t orig_pte = ptep_get(ptep);
if (!pte_present(orig_pte) || !pte_cont(orig_pte))
return orig_pte;
ncontig = num_contig_ptes(page_size(pte_page(orig_pte)), &pgsize);
for (i = 0; i < ncontig; i++, ptep++) {
pte_t pte = ptep_get(ptep);
if (pte_dirty(pte))
orig_pte = pte_mkdirty(orig_pte);
if (pte_young(pte))
orig_pte = pte_mkyoung(orig_pte);
}
return orig_pte;
}
> However we should add a new parameter for ARM64 specific huge_ptep_get() to check
> how many continuous PTEs or PMDs in this CONT-PTE/PMD size hugetlb, that means we
> should convert all the places using huge_ptep_get(), meanwhile most places using
> huge_ptep_get() did not care about the dirty or young flags at all.
>
> So instead of changing the prototype of huge_ptep_get(), this patch set introduces
> a new huge_ptep_get_access_flags() interface and define an ARM64 specific implementation,
> that will take into account any subpages' dirty or young bits for CONT-PTE/PMD size
> hugetlb page. And we can only change to use huge_ptep_get_access_flags() for those
> functions that care about the dirty or young flags of a hugetlb page.
>
> [1] https://lore.kernel.org/linux-mm/[email protected]/
>
> Baolin Wang (3):
> arm64/hugetlb: Introduce new huge_ptep_get_access_flags() interface
> fs/proc/task_mmu: Change to use huge_ptep_get_access_flags()
> mm/damon/vaddr: Change to use huge_ptep_get_access_flags()
>
> arch/arm64/include/asm/hugetlb.h | 2 ++
> arch/arm64/mm/hugetlbpage.c | 24 ++++++++++++++++++++++++
> fs/proc/task_mmu.c | 3 ++-
> include/asm-generic/hugetlb.h | 7 +++++++
> mm/damon/vaddr.c | 5 +++--
> 5 files changed, 38 insertions(+), 3 deletions(-)
>
> --
> 1.8.3.1
>
>