[PATCH] arm64/hugetlb: Simplify the huge_ptep_set_access_flags()

[PATCH] arm64/hugetlb: Simplify the huge_ptep_set_access_flags()

[Baolin Wang]

After commit bc5dfb4fd7bd ("arm64/hugetlb: Implement arm64 specific
huge_ptep_get()"), the arm64 specific huge_ptep_get() will always
consider the subpages' dirty and young state for CONT-PTE/PMD hugetlb,
so there is no need to check them again when setting the access flags
for CONT-PTE/PMD hugetlb in huge_ptep_set_access_flags().

Meanwhile this also fixes an issue when users want to make the CONT-PTE/PMD
hugetlb's pte entry old, which will be failed to make the pte entry old
since the original code will always consider the subpages' young state
if the subpages' young state is set. For example, we will make the
CONT-PTE/PMD hugetlb pte entry old in DAMON to monitoring the accesses,
but we'll failed to monitoring the actual accesses of the CONT-PTE/PMD
hugetlb page, due to we can not make its pte old.

Thus remove the code considering the subpages' dirty and young state in
huge_ptep_set_access_flags() to fix this issue and simplify the function.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 arch/arm64/mm/hugetlbpage.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index e2a5ec9..5c703aa 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -448,7 +448,6 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 	size_t pgsize = 0;
 	unsigned long pfn = pte_pfn(pte), dpfn;
 	pgprot_t hugeprot;
-	pte_t orig_pte;
 
 	if (!pte_cont(pte))
 		return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
@@ -459,14 +458,7 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 	if (!__cont_access_flags_changed(ptep, pte, ncontig))
 		return 0;
 
-	orig_pte = get_clear_contig(vma->vm_mm, addr, ptep, pgsize, ncontig);
-
-	/* Make sure we don't lose the dirty or young state */
-	if (pte_dirty(orig_pte))
-		pte = pte_mkdirty(pte);
-
-	if (pte_young(orig_pte))
-		pte = pte_mkyoung(pte);
+	clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
 
 	hugeprot = pte_pgprot(pte);
 	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn)
-- 
1.8.3.1

Re: [PATCH] arm64/hugetlb: Simplify the huge_ptep_set_access_flags()

[Will Deacon]

On Wed, May 25, 2022 at 06:31:09PM +0800, Baolin Wang wrote:
> After commit bc5dfb4fd7bd ("arm64/hugetlb: Implement arm64 specific
> huge_ptep_get()"), the arm64 specific huge_ptep_get() will always
> consider the subpages' dirty and young state for CONT-PTE/PMD hugetlb,
> so there is no need to check them again when setting the access flags
> for CONT-PTE/PMD hugetlb in huge_ptep_set_access_flags().
> 
> Meanwhile this also fixes an issue when users want to make the CONT-PTE/PMD
> hugetlb's pte entry old, which will be failed to make the pte entry old
> since the original code will always consider the subpages' young state
> if the subpages' young state is set. For example, we will make the
> CONT-PTE/PMD hugetlb pte entry old in DAMON to monitoring the accesses,
> but we'll failed to monitoring the actual accesses of the CONT-PTE/PMD
> hugetlb page, due to we can not make its pte old.
> 
> Thus remove the code considering the subpages' dirty and young state in
> huge_ptep_set_access_flags() to fix this issue and simplify the function.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  arch/arm64/mm/hugetlbpage.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index e2a5ec9..5c703aa 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -448,7 +448,6 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>  	size_t pgsize = 0;
>  	unsigned long pfn = pte_pfn(pte), dpfn;
>  	pgprot_t hugeprot;
> -	pte_t orig_pte;
>  
>  	if (!pte_cont(pte))
>  		return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
> @@ -459,14 +458,7 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>  	if (!__cont_access_flags_changed(ptep, pte, ncontig))
>  		return 0;
>  
> -	orig_pte = get_clear_contig(vma->vm_mm, addr, ptep, pgsize, ncontig);
> -
> -	/* Make sure we don't lose the dirty or young state */
> -	if (pte_dirty(orig_pte))
> -		pte = pte_mkdirty(pte);
> -
> -	if (pte_young(orig_pte))
> -		pte = pte_mkyoung(pte);
> +	clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);

I don't understand what this clear_flush() call is doing here; notably, it
includes TLB invalidation which we don't have for the non-cont case.

Why isn't huge_ptep_set_access_flags() just a loop around
ptep_set_access_flags() if huge_ptep_get() is taking care of collapsing the
dirty/young state?

Will

Re: [PATCH] arm64/hugetlb: Simplify the huge_ptep_set_access_flags()

[Baolin Wang]

On 6/9/2022 11:44 PM, Will Deacon wrote:
> On Wed, May 25, 2022 at 06:31:09PM +0800, Baolin Wang wrote:
>> After commit bc5dfb4fd7bd ("arm64/hugetlb: Implement arm64 specific
>> huge_ptep_get()"), the arm64 specific huge_ptep_get() will always
>> consider the subpages' dirty and young state for CONT-PTE/PMD hugetlb,
>> so there is no need to check them again when setting the access flags
>> for CONT-PTE/PMD hugetlb in huge_ptep_set_access_flags().
>>
>> Meanwhile this also fixes an issue when users want to make the CONT-PTE/PMD
>> hugetlb's pte entry old, which will be failed to make the pte entry old
>> since the original code will always consider the subpages' young state
>> if the subpages' young state is set. For example, we will make the
>> CONT-PTE/PMD hugetlb pte entry old in DAMON to monitoring the accesses,
>> but we'll failed to monitoring the actual accesses of the CONT-PTE/PMD
>> hugetlb page, due to we can not make its pte old.
>>
>> Thus remove the code considering the subpages' dirty and young state in
>> huge_ptep_set_access_flags() to fix this issue and simplify the function.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   arch/arm64/mm/hugetlbpage.c | 10 +---------
>>   1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index e2a5ec9..5c703aa 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -448,7 +448,6 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>   	size_t pgsize = 0;
>>   	unsigned long pfn = pte_pfn(pte), dpfn;
>>   	pgprot_t hugeprot;
>> -	pte_t orig_pte;
>>   
>>   	if (!pte_cont(pte))
>>   		return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
>> @@ -459,14 +458,7 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>   	if (!__cont_access_flags_changed(ptep, pte, ncontig))
>>   		return 0;
>>   
>> -	orig_pte = get_clear_contig(vma->vm_mm, addr, ptep, pgsize, ncontig);
>> -
>> -	/* Make sure we don't lose the dirty or young state */
>> -	if (pte_dirty(orig_pte))
>> -		pte = pte_mkdirty(pte);
>> -
>> -	if (pte_young(orig_pte))
>> -		pte = pte_mkyoung(pte);
>> +	clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
> 
> I don't understand what this clear_flush() call is doing here; notably, it
> includes TLB invalidation which we don't have for the non-cont case.

OK. I can just call a loop of pte_clear() to clear cont-pte to avoid TLB 
flush.

> 
> Why isn't huge_ptep_set_access_flags() just a loop around
> ptep_set_access_flags() if huge_ptep_get() is taking care of collapsing the
> dirty/young state?

IIUC, according to the comments "Changing some bits of contiguous 
entries requires us to follow a Break-Before-Make approach, breaking the 
whole contiguous set before we can change any entries". So we should 
clear the cont-ptes firstly, then re-set them. Then a loop of 
ptep_set_access_flags() is not suitable for the cont-pte case, right? 
Please correct me if I missed something else. Thanks.

Re: [PATCH] arm64/hugetlb: Simplify the huge_ptep_set_access_flags()

[Catalin Marinas]

On Wed, May 25, 2022 at 06:31:09PM +0800, Baolin Wang wrote:
> After commit bc5dfb4fd7bd ("arm64/hugetlb: Implement arm64 specific
> huge_ptep_get()"), the arm64 specific huge_ptep_get() will always
> consider the subpages' dirty and young state for CONT-PTE/PMD hugetlb,
> so there is no need to check them again when setting the access flags
> for CONT-PTE/PMD hugetlb in huge_ptep_set_access_flags().
> 
> Meanwhile this also fixes an issue when users want to make the CONT-PTE/PMD
> hugetlb's pte entry old, which will be failed to make the pte entry old
> since the original code will always consider the subpages' young state
> if the subpages' young state is set. For example, we will make the
> CONT-PTE/PMD hugetlb pte entry old in DAMON to monitoring the accesses,
> but we'll failed to monitoring the actual accesses of the CONT-PTE/PMD
> hugetlb page, due to we can not make its pte old.
> 
> Thus remove the code considering the subpages' dirty and young state in
> huge_ptep_set_access_flags() to fix this issue and simplify the function.

The ptep_set_access_flags() semantics (non-huge) never clear the access
flag, so mkold is not allowed. I think damon_hugetlb_mkold() is wrong in
assuming that huge_ptep_set_access_flags() allows a young->old huge pte
transition.

-- 
Catalin

Re: [PATCH] arm64/hugetlb: Simplify the huge_ptep_set_access_flags()

[Baolin Wang]

On 6/18/2022 1:46 AM, Catalin Marinas wrote:
> On Wed, May 25, 2022 at 06:31:09PM +0800, Baolin Wang wrote:
>> After commit bc5dfb4fd7bd ("arm64/hugetlb: Implement arm64 specific
>> huge_ptep_get()"), the arm64 specific huge_ptep_get() will always
>> consider the subpages' dirty and young state for CONT-PTE/PMD hugetlb,
>> so there is no need to check them again when setting the access flags
>> for CONT-PTE/PMD hugetlb in huge_ptep_set_access_flags().
>>
>> Meanwhile this also fixes an issue when users want to make the CONT-PTE/PMD
>> hugetlb's pte entry old, which will be failed to make the pte entry old
>> since the original code will always consider the subpages' young state
>> if the subpages' young state is set. For example, we will make the
>> CONT-PTE/PMD hugetlb pte entry old in DAMON to monitoring the accesses,
>> but we'll failed to monitoring the actual accesses of the CONT-PTE/PMD
>> hugetlb page, due to we can not make its pte old.
>>
>> Thus remove the code considering the subpages' dirty and young state in
>> huge_ptep_set_access_flags() to fix this issue and simplify the function.
> 
> The ptep_set_access_flags() semantics (non-huge) never clear the access
> flag, so mkold is not allowed. I think damon_hugetlb_mkold() is wrong in
> assuming that huge_ptep_set_access_flags() allows a young->old huge pte
> transition.

After reading the code carefully, yes, you are right. Seems I need 
change to use set_huge_pte_at() to make the huge pte old. Thanks.

By the way, after changing to use set_huge_pte_at() in the 
damon_hugetlb_mkold(), it seems to me that we still do not need to get 
the subpages' dirty and young state again in 
huge_ptep_set_access_flags(). How do you think?