[RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[Baolin Wang]

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/85bd80b4-b4fd-0d3f-a2e5-149559f2f387@oracle.com/

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

[RFC PATCH 1/3] arm64/hugetlb: Introduce new huge_ptep_get_access_flags() interface

[Baolin Wang]

Now we use huge_ptep_get() to get the pte value of a hugetlb page,
however it will only return one specific pte value for the CONT-PTE
or CONT-PMD size hugetlb on ARM64 system, which can contain seravel
continuous pte or pmd entries with same page table attributes. And it
will not take into account the subpages' dirty or young bits of a
CONT-PTE/PMD size hugetlb page.

So the huge_ptep_get() is inconsistent with huge_ptep_get_and_clear(),
which already takes account the dirty or young bits for any subpages
in this CONT-PTE/PMD size hugetlb [1]. Meanwhile we can miss dirty or
young flags statistics for hugetlb pages with current huge_ptep_get(),
such as the gather_hugetlb_stats() function.

Thus introduce 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, for those functions
that want to check the dirty and young flags of a hugetlb page.

[1] https://lore.kernel.org/linux-mm/85bd80b4-b4fd-0d3f-a2e5-149559f2f387@oracle.com/

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 arch/arm64/include/asm/hugetlb.h |  2 ++
 arch/arm64/mm/hugetlbpage.c      | 24 ++++++++++++++++++++++++
 include/asm-generic/hugetlb.h    |  7 +++++++
 3 files changed, 33 insertions(+)

diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
index 616b2ca..a473544 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -44,6 +44,8 @@ extern pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
 #define __HAVE_ARCH_HUGE_PTE_CLEAR
 extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
 			   pte_t *ptep, unsigned long sz);
+#define __HAVE_ARCH_HUGE_PTEP_GET_ACCESS_FLAGS
+extern pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long sz);
 extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
 				 pte_t *ptep, pte_t pte, unsigned long sz);
 #define set_huge_swap_pte_at set_huge_swap_pte_at
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index ca8e65c..ce39699 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -158,6 +158,30 @@ static inline int num_contig_ptes(unsigned long size, size_t *pgsize)
 	return contig_ptes;
 }
 
+pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long sz)
+{
+	int ncontig, i;
+	size_t pgsize;
+	pte_t orig_pte = ptep_get(ptep);
+
+	if (!pte_cont(orig_pte))
+		return orig_pte;
+
+	ncontig = num_contig_ptes(sz, &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;
+}
+
 /*
  * Changing some bits of contiguous entries requires us to follow a
  * Break-Before-Make approach, breaking the whole contiguous set
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index a57d667..bb77fb0 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -150,6 +150,13 @@ static inline pte_t huge_ptep_get(pte_t *ptep)
 }
 #endif
 
+#ifndef __HAVE_ARCH_HUGE_PTEP_GET_ACCESS_FLAGS
+static inline pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long sz)
+{
+	return ptep_get(ptep);
+}
+#endif
+
 #ifndef __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
 static inline bool gigantic_page_runtime_supported(void)
 {
-- 
1.8.3.1

Re: [RFC PATCH 1/3] arm64/hugetlb: Introduce new huge_ptep_get_access_flags() interface

[Baolin Wang]

On 5/8/2022 9:14 PM, nh26223@qq.com wrote:
> On 2022年5月8日星期日 CST 下午4:58:52 Baolin Wang wrote:
>> Now we use huge_ptep_get() to get the pte value of a hugetlb page,
>> however it will only return one specific pte value for the CONT-PTE
>> or CONT-PMD size hugetlb on ARM64 system, which can contain seravel
>> continuous pte or pmd entries with same page table attributes. And it
>> will not take into account the subpages' dirty or young bits of a
>> CONT-PTE/PMD size hugetlb page.
>>
>> So the huge_ptep_get() is inconsistent with huge_ptep_get_and_clear(),
>> which already takes account the dirty or young bits for any subpages
>> in this CONT-PTE/PMD size hugetlb [1]. Meanwhile we can miss dirty or
>> young flags statistics for hugetlb pages with current huge_ptep_get(),
>> such as the gather_hugetlb_stats() function.
>>
>> Thus introduce 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, for those functions
>> that want to check the dirty and young flags of a hugetlb page.
>>
>> [1]
>> https://lore.kernel.org/linux-mm/85bd80b4-b4fd-0d3f-a2e5-149559f2f387@oracl
>> e.com/
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   arch/arm64/include/asm/hugetlb.h |  2 ++
>>   arch/arm64/mm/hugetlbpage.c      | 24 ++++++++++++++++++++++++
>>   include/asm-generic/hugetlb.h    |  7 +++++++
>>   3 files changed, 33 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/hugetlb.h
>> b/arch/arm64/include/asm/hugetlb.h index 616b2ca..a473544 100644
>> --- a/arch/arm64/include/asm/hugetlb.h
>> +++ b/arch/arm64/include/asm/hugetlb.h
>> @@ -44,6 +44,8 @@ extern pte_t huge_ptep_clear_flush(struct vm_area_struct
>> *vma, #define __HAVE_ARCH_HUGE_PTE_CLEAR
>>   extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>>   			   pte_t *ptep, unsigned long sz);
>> +#define __HAVE_ARCH_HUGE_PTEP_GET_ACCESS_FLAGS
>> +extern pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long sz);
>>   extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
>>   				 pte_t *ptep, pte_t pte, unsigned long
> sz);
>>   #define set_huge_swap_pte_at set_huge_swap_pte_at
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index ca8e65c..ce39699 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -158,6 +158,30 @@ static inline int num_contig_ptes(unsigned long size,
>> size_t *pgsize) return contig_ptes;
>>   }
>>
>> +pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long sz)
> The function name looks to me that it returns access flags of PTE.

Yes, not a good name. That's why this is a RFC patch set to get more 
suggestion :)

Maybe huge_ptep_get_with_access_flags()? or do you have some better idea?

> 
>> +{
>> +	int ncontig, i;
>> +	size_t pgsize;
>> +	pte_t orig_pte = ptep_get(ptep);
>> +
>> +	if (!pte_cont(orig_pte))
>> +		return orig_pte;
>> +
>> +	ncontig = num_contig_ptes(sz, &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;
>> +}
> Not sure whether it's worthy being changed to:
> 
>          bool dirty = false, young = false;
> 
>          for (i = 0; i < ncontig; i++, ptep++) {
>                  pte_t pte = ptep_get(ptep);
> 
>                  if (pte_dirty(pte))
>                          dirty = true;
> 
>                  if (pte_young(pte))
>                          young = true;
> 
>                  if (dirty && young)
>                          break;
>          }
> 
>          if (dirty)
>                  orig_pte = pte_mkdirty(orig_pte);
> 
>          if (young)
>                  orig_pte = pte_mkyoung(orit_pte);
> 
>          return orig_pte;

I followed the same logics in get_clear_flush(), which is more readable 
I think. Yes, your approach can save some cycles, I can change to use it 
in next version if arm64 maintainers have no objection.

>> +
>>   /*
>>    * Changing some bits of contiguous entries requires us to follow a
>>    * Break-Before-Make approach, breaking the whole contiguous set
>> diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
>> index a57d667..bb77fb0 100644
>> --- a/include/asm-generic/hugetlb.h
>> +++ b/include/asm-generic/hugetlb.h
>> @@ -150,6 +150,13 @@ static inline pte_t huge_ptep_get(pte_t *ptep)
>>   }
>>   #endif
>>
>> +#ifndef __HAVE_ARCH_HUGE_PTEP_GET_ACCESS_FLAGS
>> +static inline pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long
>> sz) +{
>> +	return ptep_get(ptep);
> Should be:
> 	return huge_ptep_get(ptep) ?

I don't think so. If no ARCH-specific definition, the 
huge_ptep_get_access_flags() implementation should be same as 
huge_ptep_get(). Thanks for your comments.

#ifndef __HAVE_ARCH_HUGE_PTEP_GET
static inline pte_t huge_ptep_get(pte_t *ptep)
{
         return ptep_get(ptep);
}
#endif

Re: [RFC PATCH 1/3] arm64/hugetlb: Introduce new huge_ptep_get_access_flags() interface

[Baolin Wang]

On 5/9/2022 12:10 PM, nh26223@qq.com write:
> ----------------8<---------------
>>>>
>>>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>>> index ca8e65c..ce39699 100644
>>>> --- a/arch/arm64/mm/hugetlbpage.c
>>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>>> @@ -158,6 +158,30 @@ static inline int num_contig_ptes(unsigned long
>>>> size,
>>>> size_t *pgsize) return contig_ptes;
>>>>
>>>>    }
>>>>
>>>> +pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned long sz)
>>>
>>> The function name looks to me that it returns access flags of PTE.
>>
>> Yes, not a good name. That's why this is a RFC patch set to get more
>> suggestion :)
>>
>> Maybe huge_ptep_get_with_access_flags()? or do you have some better idea?
> I don't have either. "Naming is hard". :)
> 
>>>> diff --git a/include/asm-generic/hugetlb.h
>>>> b/include/asm-generic/hugetlb.h
>>>> index a57d667..bb77fb0 100644
>>>> --- a/include/asm-generic/hugetlb.h
>>>> +++ b/include/asm-generic/hugetlb.h
>>>> @@ -150,6 +150,13 @@ static inline pte_t huge_ptep_get(pte_t *ptep)
>>>>
>>>>    }
>>>>    #endif
>>>>
>>>> +#ifndef __HAVE_ARCH_HUGE_PTEP_GET_ACCESS_FLAGS
>>>> +static inline pte_t huge_ptep_get_access_flags(pte_t *ptep, unsigned
>>>> long
>>>> sz) +{
>>>> +	return ptep_get(ptep);
>>>
>>> Should be:
>>> 	return huge_ptep_get(ptep) ?
>>
>> I don't think so. If no ARCH-specific definition, the
>> huge_ptep_get_access_flags() implementation should be same as
>> huge_ptep_get(). Thanks for your comments.
> If no __HAVE_ARCH_HUGE_PTEP_GET, huge_ptep_get() is same as
> ptep_get().
> 
> Or it's not possible no __HAVE_ARCH_HUGE_PTEP_GET_ACCESS_FLAGS
> but with __HAVE_ARCH_HUGE_PTEP_GET?

Yes, I am wrong, shoule be huge_ptep_get(). Thanks for pointing out 
issues :)

PS: I think I will follow Muchun's suggestion in next version, so no 
need to add a new interface.

[RFC PATCH 2/3] fs/proc/task_mmu: Change to use huge_ptep_get_access_flags()

[Baolin Wang]

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 <baolin.wang@linux.alibaba.com>
---
 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

[RFC PATCH 3/3] mm/damon/vaddr: Change to use huge_ptep_get_access_flags()

[Baolin Wang]

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. That will make the hugetlb pages
monitoring inaccurate with missing young flags.

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.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/damon/vaddr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d6abf76..29459ed 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -400,7 +400,8 @@ static void damon_hugetlb_mkold(pte_t *pte, struct mm_struct *mm,
 				struct vm_area_struct *vma, unsigned long addr)
 {
 	bool referenced = false;
-	pte_t entry = huge_ptep_get(pte);
+	pte_t entry = huge_ptep_get_access_flags(pte,
+					huge_page_size(hstate_vma(vma)));
 	struct page *page = pte_page(entry);
 
 	get_page(page);
@@ -557,7 +558,7 @@ static int damon_young_hugetlb_entry(pte_t *pte, unsigned long hmask,
 	pte_t entry;
 
 	ptl = huge_pte_lock(h, walk->mm, pte);
-	entry = huge_ptep_get(pte);
+	entry = huge_ptep_get_access_flags(pte, huge_page_size(h));
 	if (!pte_present(entry))
 		goto out;
 
-- 
1.8.3.1

Re: [RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[Muchun Song]

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/85bd80b4-b4fd-0d3f-a2e5-149559f2f387@oracle.com/
> 
> 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
> 
> 

Re: [RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[Baolin Wang]

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.

Re: [RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[Matthew Wilcox]

On Sun, May 08, 2022 at 04:58:51PM +0800, Baolin Wang wrote:
> 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.

I question whether this is the right approach.  I understand that
different hardware implementations have different requirements here,
but at least one that I'm aware of (AMD Zen 2/3) requires that all
PTEs that are part of a contig PTE must have identical A/D bits.  Now,
you could say that's irrelevant because it's x86 and we don't currently
support contPTE on x86, but I wouldn't be surprised to see that other
hardware has the same requirement.

So what if we make that a Linux requirement?  Setting a contPTE dirty or
accessed becomes a bit more expensive (although still one/two cachelines,
so not really much more expensive than a single write).  Then there's no
need to change the "get" side of things because they're always identical.

It does mean that we can't take advantage of hardware setting A/D bits,
unless hardware can be persuaded to behave this way.  I don't have any
ARM specs in front of me to check.

I don't have a hard objection to your approach, I just want to discuss
other possibilities.

Re: [RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[Baolin Wang]

On 5/9/2022 1:08 AM, Matthew Wilcox wrote:
> On Sun, May 08, 2022 at 04:58:51PM +0800, Baolin Wang wrote:
>> 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.
> 
> I question whether this is the right approach.  I understand that
> different hardware implementations have different requirements here,
> but at least one that I'm aware of (AMD Zen 2/3) requires that all
> PTEs that are part of a contig PTE must have identical A/D bits.  Now,
> you could say that's irrelevant because it's x86 and we don't currently
> support contPTE on x86, but I wouldn't be surprised to see that other
> hardware has the same requirement.

Yes, so on x86, we can use the default huge_ptep_get(). But for ARM64, 
unfortunately the A/D bits of a contig PTE is independent, that's why we 
want a ARM64 specific huge_ptep_get().

> So what if we make that a Linux requirement?  Setting a contPTE dirty or
> accessed becomes a bit more expensive (although still one/two cachelines,
> so not really much more expensive than a single write).  Then there's no
> need to change the "get" side of things because they're always identical.
> 
> It does mean that we can't take advantage of hardware setting A/D bits,
> unless hardware can be persuaded to behave this way.  I don't have any
> ARM specs in front of me to check.

I hope the hardware can make sure the contPTE are always identical, 
however in fact like I said the A/D bits setting of a contig PTE by 
hardware is independent in a contig-PTE size hugetlb page, they are not 
always identical.

 From my testing, if I monitored a contig-PTE size hugetlb page with 
DAMON, and I only modified the subpages of the contig-PTE size hugetlb 
page. The result is I can not monitor any accesses, but actually there are.

So I think an ARM64 specific huge_ptep_get() implementation seems the 
right way as Muchun suggested?

Thanks.

Re: [RFC PATCH 0/3] Introduce new huge_ptep_get_access_flags() interface

[Muchun Song]

On Sun, May 08, 2022 at 06:08:18PM +0100, Matthew Wilcox wrote:
> On Sun, May 08, 2022 at 04:58:51PM +0800, Baolin Wang wrote:
> > 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.
> 
> I question whether this is the right approach.  I understand that
> different hardware implementations have different requirements here,
> but at least one that I'm aware of (AMD Zen 2/3) requires that all
> PTEs that are part of a contig PTE must have identical A/D bits.  Now,
> you could say that's irrelevant because it's x86 and we don't currently
> support contPTE on x86, but I wouldn't be surprised to see that other
> hardware has the same requirement.
> 
> So what if we make that a Linux requirement?  Setting a contPTE dirty or
> accessed becomes a bit more expensive (although still one/two cachelines,
> so not really much more expensive than a single write).  Then there's no
> need to change the "get" side of things because they're always identical.
> 
> It does mean that we can't take advantage of hardware setting A/D bits,
> unless hardware can be persuaded to behave this way.  I don't have any
> ARM specs in front of me to check.
>

I have looked at the comments in get_clear_flush() (in arch/arm64/mm/hugetlbpage.c).
That says:

	/*
	 * If HW_AFDBM is enabled, then the HW could turn on
	 * the dirty or accessed bit for any page in the set,
	 * so check them all.
	 */

Unfortunately, the AD bits are not identical in all subpages.

Thanks.