[PATCH 0/2] Address some contpte nits

[PATCH 0/2] Address some contpte nits

[Ryan Roberts]

Hi Andrew,

These 2 patches address some nits raised by Catalin late in the review cycle for
my contpte series [1]. Unfortunately I was out on holiday last week and you have
now moved the original series to mm-stable, so sending these out as separate
patches.

They apply on top of current mm-unstable (ccbd06e764ba).

I've build-tested with allmodconfig for arm64.

Thanks,
Ryan

[1] https://lore.kernel.org/linux-mm/20240215103205.2607016-1-ryan.roberts@arm.com/

Ryan Roberts (2):
  arm64/mm: Export contpte symbols only to GPL users
  arm64/mm: Improve comment in contpte_ptep_get_lockless()

 arch/arm64/mm/contpte.c | 46 ++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

--
2.25.1

[PATCH 1/2] arm64/mm: Export contpte symbols only to GPL users

[Ryan Roberts]

The contpte symbols must be exported since some of the public inline
ptep_* APIs are called from modules and these inlines now call the
contpte functions. Originally they were exported as EXPORT_SYMBOL() for
fear of breaking out-of-tree modules. But we subsequently concluded that
EXPORT_SYMBOL_GPL() should be safe since these functions are deeply core
mm routines, and any module operating at this level is not going to be
able to survive on EXPORT_SYMBOL alone.

Link: https://lore.kernel.org/linux-mm/f9fc2b31-11cb-4969-8961-9c89fea41b74@nvidia.com/
Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 arch/arm64/mm/contpte.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index 16788f07716d..be0a226c4ff9 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -135,7 +135,7 @@ void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
 	pte = pte_mkcont(pte);
 	contpte_convert(mm, addr, orig_ptep, pte);
 }
-EXPORT_SYMBOL(__contpte_try_fold);
+EXPORT_SYMBOL_GPL(__contpte_try_fold);
 
 void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
 			pte_t *ptep, pte_t pte)
@@ -150,7 +150,7 @@ void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
 	pte = pte_mknoncont(pte);
 	contpte_convert(mm, addr, ptep, pte);
 }
-EXPORT_SYMBOL(__contpte_try_unfold);
+EXPORT_SYMBOL_GPL(__contpte_try_unfold);
 
 pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte)
 {
@@ -178,7 +178,7 @@ pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte)
 
 	return orig_pte;
 }
-EXPORT_SYMBOL(contpte_ptep_get);
+EXPORT_SYMBOL_GPL(contpte_ptep_get);
 
 pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
 {
@@ -231,7 +231,7 @@ pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
 
 	return orig_pte;
 }
-EXPORT_SYMBOL(contpte_ptep_get_lockless);
+EXPORT_SYMBOL_GPL(contpte_ptep_get_lockless);
 
 void contpte_set_ptes(struct mm_struct *mm, unsigned long addr,
 					pte_t *ptep, pte_t pte, unsigned int nr)
@@ -274,7 +274,7 @@ void contpte_set_ptes(struct mm_struct *mm, unsigned long addr,
 
 	} while (addr != end);
 }
-EXPORT_SYMBOL(contpte_set_ptes);
+EXPORT_SYMBOL_GPL(contpte_set_ptes);
 
 void contpte_clear_full_ptes(struct mm_struct *mm, unsigned long addr,
 				pte_t *ptep, unsigned int nr, int full)
@@ -282,7 +282,7 @@ void contpte_clear_full_ptes(struct mm_struct *mm, unsigned long addr,
 	contpte_try_unfold_partial(mm, addr, ptep, nr);
 	__clear_full_ptes(mm, addr, ptep, nr, full);
 }
-EXPORT_SYMBOL(contpte_clear_full_ptes);
+EXPORT_SYMBOL_GPL(contpte_clear_full_ptes);
 
 pte_t contpte_get_and_clear_full_ptes(struct mm_struct *mm,
 				unsigned long addr, pte_t *ptep,
@@ -291,7 +291,7 @@ pte_t contpte_get_and_clear_full_ptes(struct mm_struct *mm,
 	contpte_try_unfold_partial(mm, addr, ptep, nr);
 	return __get_and_clear_full_ptes(mm, addr, ptep, nr, full);
 }
-EXPORT_SYMBOL(contpte_get_and_clear_full_ptes);
+EXPORT_SYMBOL_GPL(contpte_get_and_clear_full_ptes);
 
 int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma,
 					unsigned long addr, pte_t *ptep)
@@ -316,7 +316,7 @@ int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma,
 
 	return young;
 }
-EXPORT_SYMBOL(contpte_ptep_test_and_clear_young);
+EXPORT_SYMBOL_GPL(contpte_ptep_test_and_clear_young);
 
 int contpte_ptep_clear_flush_young(struct vm_area_struct *vma,
 					unsigned long addr, pte_t *ptep)
@@ -337,7 +337,7 @@ int contpte_ptep_clear_flush_young(struct vm_area_struct *vma,
 
 	return young;
 }
-EXPORT_SYMBOL(contpte_ptep_clear_flush_young);
+EXPORT_SYMBOL_GPL(contpte_ptep_clear_flush_young);
 
 void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr,
 					pte_t *ptep, unsigned int nr)
@@ -355,7 +355,7 @@ void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr,
 	contpte_try_unfold_partial(mm, addr, ptep, nr);
 	__wrprotect_ptes(mm, addr, ptep, nr);
 }
-EXPORT_SYMBOL(contpte_wrprotect_ptes);
+EXPORT_SYMBOL_GPL(contpte_wrprotect_ptes);
 
 int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
 					unsigned long addr, pte_t *ptep,
@@ -401,4 +401,4 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
 
 	return 1;
 }
-EXPORT_SYMBOL(contpte_ptep_set_access_flags);
+EXPORT_SYMBOL_GPL(contpte_ptep_set_access_flags);
-- 
2.25.1

Re: [PATCH 1/2] arm64/mm: Export contpte symbols only to GPL users

[David Hildenbrand]

On 26.02.24 13:03, Ryan Roberts wrote:
> The contpte symbols must be exported since some of the public inline
> ptep_* APIs are called from modules and these inlines now call the
> contpte functions. Originally they were exported as EXPORT_SYMBOL() for
> fear of breaking out-of-tree modules. But we subsequently concluded that
> EXPORT_SYMBOL_GPL() should be safe since these functions are deeply core
> mm routines, and any module operating at this level is not going to be
> able to survive on EXPORT_SYMBOL alone.
> 

I only looked at __set_ptes() to get a feeling what would currently work.

__set_ptes() might already call __sync_icache_dcache() via 
__sync_cache_and_tags(), that is EXPORT_SYMBOL_GPL.

[mte_sync_tags() is not exported at all, so maybe it's safe to assume 
that some out-of-tree module could not make good use of set_pte_at() in 
general]

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

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] arm64/mm: Export contpte symbols only to GPL users

[Ryan Roberts]

On 26/02/2024 12:25, David Hildenbrand wrote:
> On 26.02.24 13:03, Ryan Roberts wrote:
>> The contpte symbols must be exported since some of the public inline
>> ptep_* APIs are called from modules and these inlines now call the
>> contpte functions. Originally they were exported as EXPORT_SYMBOL() for
>> fear of breaking out-of-tree modules. But we subsequently concluded that
>> EXPORT_SYMBOL_GPL() should be safe since these functions are deeply core
>> mm routines, and any module operating at this level is not going to be
>> able to survive on EXPORT_SYMBOL alone.
>>
> 
> I only looked at __set_ptes() to get a feeling what would currently work.
> 
> __set_ptes() might already call __sync_icache_dcache() via
> __sync_cache_and_tags(), that is EXPORT_SYMBOL_GPL.
> 
> [mte_sync_tags() is not exported at all, so maybe it's safe to assume that some
> out-of-tree module could not make good use of set_pte_at() in general]

That's interesting, some grepping I previously did showed that ptep_get() and
set_pte_at() are used by in-tree drivers (which I assume can be built as modules):

https://lore.kernel.org/linux-arm-kernel/b994ff89-1a1f-26ca-9479-b08c77f94be8@arm.com/

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

Thanks!

Re: [PATCH 1/2] arm64/mm: Export contpte symbols only to GPL users

[John Hubbard]

On 2/26/24 04:03, Ryan Roberts wrote:
> The contpte symbols must be exported since some of the public inline
> ptep_* APIs are called from modules and these inlines now call the
> contpte functions. Originally they were exported as EXPORT_SYMBOL() for
> fear of breaking out-of-tree modules. But we subsequently concluded that
> EXPORT_SYMBOL_GPL() should be safe since these functions are deeply core
> mm routines, and any module operating at this level is not going to be
> able to survive on EXPORT_SYMBOL alone.
> 
> Link: https://lore.kernel.org/linux-mm/f9fc2b31-11cb-4969-8961-9c89fea41b74@nvidia.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   arch/arm64/mm/contpte.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 

Yes, looks good.

Reviewed-by:  John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA


> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index 16788f07716d..be0a226c4ff9 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -135,7 +135,7 @@ void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
>   	pte = pte_mkcont(pte);
>   	contpte_convert(mm, addr, orig_ptep, pte);
>   }
> -EXPORT_SYMBOL(__contpte_try_fold);
> +EXPORT_SYMBOL_GPL(__contpte_try_fold);
>   
>   void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
>   			pte_t *ptep, pte_t pte)
> @@ -150,7 +150,7 @@ void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
>   	pte = pte_mknoncont(pte);
>   	contpte_convert(mm, addr, ptep, pte);
>   }
> -EXPORT_SYMBOL(__contpte_try_unfold);
> +EXPORT_SYMBOL_GPL(__contpte_try_unfold);
>   
>   pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte)
>   {
> @@ -178,7 +178,7 @@ pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte)
>   
>   	return orig_pte;
>   }
> -EXPORT_SYMBOL(contpte_ptep_get);
> +EXPORT_SYMBOL_GPL(contpte_ptep_get);
>   
>   pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>   {
> @@ -231,7 +231,7 @@ pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>   
>   	return orig_pte;
>   }
> -EXPORT_SYMBOL(contpte_ptep_get_lockless);
> +EXPORT_SYMBOL_GPL(contpte_ptep_get_lockless);
>   
>   void contpte_set_ptes(struct mm_struct *mm, unsigned long addr,
>   					pte_t *ptep, pte_t pte, unsigned int nr)
> @@ -274,7 +274,7 @@ void contpte_set_ptes(struct mm_struct *mm, unsigned long addr,
>   
>   	} while (addr != end);
>   }
> -EXPORT_SYMBOL(contpte_set_ptes);
> +EXPORT_SYMBOL_GPL(contpte_set_ptes);
>   
>   void contpte_clear_full_ptes(struct mm_struct *mm, unsigned long addr,
>   				pte_t *ptep, unsigned int nr, int full)
> @@ -282,7 +282,7 @@ void contpte_clear_full_ptes(struct mm_struct *mm, unsigned long addr,
>   	contpte_try_unfold_partial(mm, addr, ptep, nr);
>   	__clear_full_ptes(mm, addr, ptep, nr, full);
>   }
> -EXPORT_SYMBOL(contpte_clear_full_ptes);
> +EXPORT_SYMBOL_GPL(contpte_clear_full_ptes);
>   
>   pte_t contpte_get_and_clear_full_ptes(struct mm_struct *mm,
>   				unsigned long addr, pte_t *ptep,
> @@ -291,7 +291,7 @@ pte_t contpte_get_and_clear_full_ptes(struct mm_struct *mm,
>   	contpte_try_unfold_partial(mm, addr, ptep, nr);
>   	return __get_and_clear_full_ptes(mm, addr, ptep, nr, full);
>   }
> -EXPORT_SYMBOL(contpte_get_and_clear_full_ptes);
> +EXPORT_SYMBOL_GPL(contpte_get_and_clear_full_ptes);
>   
>   int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma,
>   					unsigned long addr, pte_t *ptep)
> @@ -316,7 +316,7 @@ int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma,
>   
>   	return young;
>   }
> -EXPORT_SYMBOL(contpte_ptep_test_and_clear_young);
> +EXPORT_SYMBOL_GPL(contpte_ptep_test_and_clear_young);
>   
>   int contpte_ptep_clear_flush_young(struct vm_area_struct *vma,
>   					unsigned long addr, pte_t *ptep)
> @@ -337,7 +337,7 @@ int contpte_ptep_clear_flush_young(struct vm_area_struct *vma,
>   
>   	return young;
>   }
> -EXPORT_SYMBOL(contpte_ptep_clear_flush_young);
> +EXPORT_SYMBOL_GPL(contpte_ptep_clear_flush_young);
>   
>   void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr,
>   					pte_t *ptep, unsigned int nr)
> @@ -355,7 +355,7 @@ void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr,
>   	contpte_try_unfold_partial(mm, addr, ptep, nr);
>   	__wrprotect_ptes(mm, addr, ptep, nr);
>   }
> -EXPORT_SYMBOL(contpte_wrprotect_ptes);
> +EXPORT_SYMBOL_GPL(contpte_wrprotect_ptes);
>   
>   int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>   					unsigned long addr, pte_t *ptep,
> @@ -401,4 +401,4 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>   
>   	return 1;
>   }
> -EXPORT_SYMBOL(contpte_ptep_set_access_flags);
> +EXPORT_SYMBOL_GPL(contpte_ptep_set_access_flags);

Re: [PATCH 1/2] arm64/mm: Export contpte symbols only to GPL users

[Catalin Marinas]

On Mon, Feb 26, 2024 at 12:03:20PM +0000, Ryan Roberts wrote:
> The contpte symbols must be exported since some of the public inline
> ptep_* APIs are called from modules and these inlines now call the
> contpte functions. Originally they were exported as EXPORT_SYMBOL() for
> fear of breaking out-of-tree modules. But we subsequently concluded that
> EXPORT_SYMBOL_GPL() should be safe since these functions are deeply core
> mm routines, and any module operating at this level is not going to be
> able to survive on EXPORT_SYMBOL alone.
> 
> Link: https://lore.kernel.org/linux-mm/f9fc2b31-11cb-4969-8961-9c89fea41b74@nvidia.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Ryan Roberts]

Make clear the atmicity/consistency requirements of the API and how we
achieve them.

Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index be0a226c4ff9..1b64b4c3f8bf 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
 pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
 {
 	/*
-	 * Gather access/dirty bits, which may be populated in any of the ptes
-	 * of the contig range. We may not be holding the PTL, so any contiguous
-	 * range may be unfolded/modified/refolded under our feet. Therefore we
-	 * ensure we read a _consistent_ contpte range by checking that all ptes
-	 * in the range are valid and have CONT_PTE set, that all pfns are
-	 * contiguous and that all pgprots are the same (ignoring access/dirty).
-	 * If we find a pte that is not consistent, then we must be racing with
-	 * an update so start again. If the target pte does not have CONT_PTE
-	 * set then that is considered consistent on its own because it is not
-	 * part of a contpte range.
+	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
+	 * so that it is self-consistent, without the PTL held, so we may be
+	 * racing with other threads modifying the pte. Usually a READ_ONCE()
+	 * would suffice, but for the contpte case, we also need to gather the
+	 * access and dirty bits from across all ptes in the contiguous block,
+	 * and we can't read all of those neighbouring ptes atomically, so any
+	 * contiguous range may be unfolded/modified/refolded under our feet.
+	 * Therefore we ensure we read a _consistent_ contpte range by checking
+	 * that all ptes in the range are valid and have CONT_PTE set, that all
+	 * pfns are contiguous and that all pgprots are the same (ignoring
+	 * access/dirty). If we find a pte that is not consistent, then we must
+	 * be racing with an update so start again. If the target pte does not
+	 * have CONT_PTE set then that is considered consistent on its own
+	 * because it is not part of a contpte range.
 	 */
 
 	pgprot_t orig_prot;
-- 
2.25.1

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[David Hildenbrand]

On 26.02.24 13:03, Ryan Roberts wrote:
> Make clear the atmicity/consistency requirements of the API and how we
> achieve them.
> 
> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index be0a226c4ff9..1b64b4c3f8bf 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
>   pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>   {
>   	/*
> -	 * Gather access/dirty bits, which may be populated in any of the ptes
> -	 * of the contig range. We may not be holding the PTL, so any contiguous
> -	 * range may be unfolded/modified/refolded under our feet. Therefore we
> -	 * ensure we read a _consistent_ contpte range by checking that all ptes
> -	 * in the range are valid and have CONT_PTE set, that all pfns are
> -	 * contiguous and that all pgprots are the same (ignoring access/dirty).
> -	 * If we find a pte that is not consistent, then we must be racing with
> -	 * an update so start again. If the target pte does not have CONT_PTE
> -	 * set then that is considered consistent on its own because it is not
> -	 * part of a contpte range.
> +	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
> +	 * so that it is self-consistent, without the PTL held, so we may be
> +	 * racing with other threads modifying the pte. Usually a READ_ONCE()
> +	 * would suffice, but for the contpte case, we also need to gather the
> +	 * access and dirty bits from across all ptes in the contiguous block,
> +	 * and we can't read all of those neighbouring ptes atomically, so any
> +	 * contiguous range may be unfolded/modified/refolded under our feet.
> +	 * Therefore we ensure we read a _consistent_ contpte range by checking
> +	 * that all ptes in the range are valid and have CONT_PTE set, that all
> +	 * pfns are contiguous and that all pgprots are the same (ignoring
> +	 * access/dirty). If we find a pte that is not consistent, then we must
> +	 * be racing with an update so start again. If the target pte does not
> +	 * have CONT_PTE set then that is considered consistent on its own
> +	 * because it is not part of a contpte range.
>   	 */
>   
>   	pgprot_t orig_prot;

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

In an ideal world, we'd really not rely on any accessed/dirty on the 
lockless path and remove contpte_ptep_get_lockless() completely :)

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Ryan Roberts]

On 26/02/2024 12:30, David Hildenbrand wrote:
> On 26.02.24 13:03, Ryan Roberts wrote:
>> Make clear the atmicity/consistency requirements of the API and how we
>> achieve them.
>>
>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>   arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>> index be0a226c4ff9..1b64b4c3f8bf 100644
>> --- a/arch/arm64/mm/contpte.c
>> +++ b/arch/arm64/mm/contpte.c
>> @@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
>>   pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>>   {
>>       /*
>> -     * Gather access/dirty bits, which may be populated in any of the ptes
>> -     * of the contig range. We may not be holding the PTL, so any contiguous
>> -     * range may be unfolded/modified/refolded under our feet. Therefore we
>> -     * ensure we read a _consistent_ contpte range by checking that all ptes
>> -     * in the range are valid and have CONT_PTE set, that all pfns are
>> -     * contiguous and that all pgprots are the same (ignoring access/dirty).
>> -     * If we find a pte that is not consistent, then we must be racing with
>> -     * an update so start again. If the target pte does not have CONT_PTE
>> -     * set then that is considered consistent on its own because it is not
>> -     * part of a contpte range.
>> +     * The ptep_get_lockless() API requires us to read and return *orig_ptep
>> +     * so that it is self-consistent, without the PTL held, so we may be
>> +     * racing with other threads modifying the pte. Usually a READ_ONCE()
>> +     * would suffice, but for the contpte case, we also need to gather the
>> +     * access and dirty bits from across all ptes in the contiguous block,
>> +     * and we can't read all of those neighbouring ptes atomically, so any
>> +     * contiguous range may be unfolded/modified/refolded under our feet.
>> +     * Therefore we ensure we read a _consistent_ contpte range by checking
>> +     * that all ptes in the range are valid and have CONT_PTE set, that all
>> +     * pfns are contiguous and that all pgprots are the same (ignoring
>> +     * access/dirty). If we find a pte that is not consistent, then we must
>> +     * be racing with an update so start again. If the target pte does not
>> +     * have CONT_PTE set then that is considered consistent on its own
>> +     * because it is not part of a contpte range.
>>        */
>>         pgprot_t orig_prot;
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

> 
> In an ideal world, we'd really not rely on any accessed/dirty on the lockless
> path and remove contpte_ptep_get_lockless() completely :)

Not sure if you saw my RFC to do exactly that? (well, it doesn't actually remove
[contpte_]ptep_get_lockless() but it does remove all the callers). If you have
any feedback, we could get this moving...

https://lore.kernel.org/linux-mm/20240215121756.2734131-1-ryan.roberts@arm.com/

Thanks,
Ryan

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[David Hildenbrand]

On 26.02.24 13:37, Ryan Roberts wrote:
> On 26/02/2024 12:30, David Hildenbrand wrote:
>> On 26.02.24 13:03, Ryan Roberts wrote:
>>> Make clear the atmicity/consistency requirements of the API and how we
>>> achieve them.
>>>
>>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>    arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>>    1 file changed, 14 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>> index be0a226c4ff9..1b64b4c3f8bf 100644
>>> --- a/arch/arm64/mm/contpte.c
>>> +++ b/arch/arm64/mm/contpte.c
>>> @@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
>>>    pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>>>    {
>>>        /*
>>> -     * Gather access/dirty bits, which may be populated in any of the ptes
>>> -     * of the contig range. We may not be holding the PTL, so any contiguous
>>> -     * range may be unfolded/modified/refolded under our feet. Therefore we
>>> -     * ensure we read a _consistent_ contpte range by checking that all ptes
>>> -     * in the range are valid and have CONT_PTE set, that all pfns are
>>> -     * contiguous and that all pgprots are the same (ignoring access/dirty).
>>> -     * If we find a pte that is not consistent, then we must be racing with
>>> -     * an update so start again. If the target pte does not have CONT_PTE
>>> -     * set then that is considered consistent on its own because it is not
>>> -     * part of a contpte range.
>>> +     * The ptep_get_lockless() API requires us to read and return *orig_ptep
>>> +     * so that it is self-consistent, without the PTL held, so we may be
>>> +     * racing with other threads modifying the pte. Usually a READ_ONCE()
>>> +     * would suffice, but for the contpte case, we also need to gather the
>>> +     * access and dirty bits from across all ptes in the contiguous block,
>>> +     * and we can't read all of those neighbouring ptes atomically, so any
>>> +     * contiguous range may be unfolded/modified/refolded under our feet.
>>> +     * Therefore we ensure we read a _consistent_ contpte range by checking
>>> +     * that all ptes in the range are valid and have CONT_PTE set, that all
>>> +     * pfns are contiguous and that all pgprots are the same (ignoring
>>> +     * access/dirty). If we find a pte that is not consistent, then we must
>>> +     * be racing with an update so start again. If the target pte does not
>>> +     * have CONT_PTE set then that is considered consistent on its own
>>> +     * because it is not part of a contpte range.
>>>         */
>>>          pgprot_t orig_prot;
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> Thanks!
> 
>>
>> In an ideal world, we'd really not rely on any accessed/dirty on the lockless
>> path and remove contpte_ptep_get_lockless() completely :)
> 
> Not sure if you saw my RFC to do exactly that? (well, it doesn't actually remove
> [contpte_]ptep_get_lockless() but it does remove all the callers). If you have
> any feedback, we could get this moving...

Yes, I saw it. Hoping we can get that in and then maybe remove the 
contpte_get_lockless() once all callers are gone :)

... on my todo list.

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[John Hubbard]

On 2/26/24 04:03, Ryan Roberts wrote:

Hi Ryan!

> Make clear the atmicity/consistency requirements of the API and how we

"atomicity"

> achieve them.
> 
> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index be0a226c4ff9..1b64b4c3f8bf 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
>   pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>   {
>   	/*
> -	 * Gather access/dirty bits, which may be populated in any of the ptes
> -	 * of the contig range. We may not be holding the PTL, so any contiguous
> -	 * range may be unfolded/modified/refolded under our feet. Therefore we
> -	 * ensure we read a _consistent_ contpte range by checking that all ptes
> -	 * in the range are valid and have CONT_PTE set, that all pfns are
> -	 * contiguous and that all pgprots are the same (ignoring access/dirty).
> -	 * If we find a pte that is not consistent, then we must be racing with
> -	 * an update so start again. If the target pte does not have CONT_PTE
> -	 * set then that is considered consistent on its own because it is not
> -	 * part of a contpte range.
> +	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
> +	 * so that it is self-consistent, without the PTL held, so we may be
> +	 * racing with other threads modifying the pte. Usually a READ_ONCE()
> +	 * would suffice, but for the contpte case, we also need to gather the
> +	 * access and dirty bits from across all ptes in the contiguous block,
> +	 * and we can't read all of those neighbouring ptes atomically, so any

This still leaves a key detail unexplained: how the accessed and dirty bits
are handled. The above raises the *problem*, but then talks about getting a
consistent set of reads. But during those consistent reads, the HW could have
dirtied or read a page. And this code here is only returning a single pte.

So I'm still feeling vague about what we're trying to say about accessed and
dirty bits.


> +	 * contiguous range may be unfolded/modified/refolded under our feet.
> +	 * Therefore we ensure we read a _consistent_ contpte range by checking
> +	 * that all ptes in the range are valid and have CONT_PTE set, that all
> +	 * pfns are contiguous and that all pgprots are the same (ignoring
> +	 * access/dirty). If we find a pte that is not consistent, then we must
> +	 * be racing with an update so start again. If the target pte does not
> +	 * have CONT_PTE set then that is considered consistent on its own
> +	 * because it is not part of a contpte range.
>   	 */
>   
>   	pgprot_t orig_prot;

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Catalin Marinas]

On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
> Make clear the atmicity/consistency requirements of the API and how we
> achieve them.
> 
> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>  arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index be0a226c4ff9..1b64b4c3f8bf 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
>  pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>  {
>  	/*
> -	 * Gather access/dirty bits, which may be populated in any of the ptes
> -	 * of the contig range. We may not be holding the PTL, so any contiguous
> -	 * range may be unfolded/modified/refolded under our feet. Therefore we
> -	 * ensure we read a _consistent_ contpte range by checking that all ptes
> -	 * in the range are valid and have CONT_PTE set, that all pfns are
> -	 * contiguous and that all pgprots are the same (ignoring access/dirty).
> -	 * If we find a pte that is not consistent, then we must be racing with
> -	 * an update so start again. If the target pte does not have CONT_PTE
> -	 * set then that is considered consistent on its own because it is not
> -	 * part of a contpte range.
> +	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
> +	 * so that it is self-consistent, without the PTL held, so we may be
> +	 * racing with other threads modifying the pte. Usually a READ_ONCE()
> +	 * would suffice, but for the contpte case, we also need to gather the
> +	 * access and dirty bits from across all ptes in the contiguous block,
> +	 * and we can't read all of those neighbouring ptes atomically, so any
> +	 * contiguous range may be unfolded/modified/refolded under our feet.
> +	 * Therefore we ensure we read a _consistent_ contpte range by checking
> +	 * that all ptes in the range are valid and have CONT_PTE set, that all
> +	 * pfns are contiguous and that all pgprots are the same (ignoring
> +	 * access/dirty). If we find a pte that is not consistent, then we must
> +	 * be racing with an update so start again. If the target pte does not
> +	 * have CONT_PTE set then that is considered consistent on its own
> +	 * because it is not part of a contpte range.
>  	 */

I haven't had the time to properly think about this function but,
depending on what its semantics are, we might not guarantee that, at the
time of reading a pte, we have the correct dirty state from the other
ptes in the range.

Theoretical: let's say we read the first pte in the contig range and
it's clean but further down there's a dirty one. Another (v)CPU breaks
the contig range, sets the dirty bit everywhere, there's some
pte_mkclean for all of them and they are collapsed into a contig range
again. The function above on the first (v)CPU returns a clean pte when
it should have actually been dirty at the time of read.

Throughout the callers of this function, I couldn't find one where it
matters. So I concluded that they don't need the dirty state. Normally
the dirty state is passed to the page flags, so not lost after the pte
has been cleaned.

-- 
Catalin

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Ryan Roberts]

On 01/03/2024 18:47, Catalin Marinas wrote:
> On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
>> Make clear the atmicity/consistency requirements of the API and how we
>> achieve them.
>>
>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>  arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>> index be0a226c4ff9..1b64b4c3f8bf 100644
>> --- a/arch/arm64/mm/contpte.c
>> +++ b/arch/arm64/mm/contpte.c
>> @@ -183,16 +183,20 @@ EXPORT_SYMBOL_GPL(contpte_ptep_get);
>>  pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>>  {
>>  	/*
>> -	 * Gather access/dirty bits, which may be populated in any of the ptes
>> -	 * of the contig range. We may not be holding the PTL, so any contiguous
>> -	 * range may be unfolded/modified/refolded under our feet. Therefore we
>> -	 * ensure we read a _consistent_ contpte range by checking that all ptes
>> -	 * in the range are valid and have CONT_PTE set, that all pfns are
>> -	 * contiguous and that all pgprots are the same (ignoring access/dirty).
>> -	 * If we find a pte that is not consistent, then we must be racing with
>> -	 * an update so start again. If the target pte does not have CONT_PTE
>> -	 * set then that is considered consistent on its own because it is not
>> -	 * part of a contpte range.
>> +	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
>> +	 * so that it is self-consistent, without the PTL held, so we may be
>> +	 * racing with other threads modifying the pte. Usually a READ_ONCE()
>> +	 * would suffice, but for the contpte case, we also need to gather the
>> +	 * access and dirty bits from across all ptes in the contiguous block,
>> +	 * and we can't read all of those neighbouring ptes atomically, so any
>> +	 * contiguous range may be unfolded/modified/refolded under our feet.
>> +	 * Therefore we ensure we read a _consistent_ contpte range by checking
>> +	 * that all ptes in the range are valid and have CONT_PTE set, that all
>> +	 * pfns are contiguous and that all pgprots are the same (ignoring
>> +	 * access/dirty). If we find a pte that is not consistent, then we must
>> +	 * be racing with an update so start again. If the target pte does not
>> +	 * have CONT_PTE set then that is considered consistent on its own
>> +	 * because it is not part of a contpte range.
>>  	 */
> 
> I haven't had the time to properly think about this function but,
> depending on what its semantics are, we might not guarantee that, at the
> time of reading a pte, we have the correct dirty state from the other
> ptes in the range.
> 
> Theoretical: let's say we read the first pte in the contig range and
> it's clean but further down there's a dirty one. Another (v)CPU breaks
> the contig range, sets the dirty bit everywhere, there's some
> pte_mkclean for all of them and they are collapsed into a contig range
> again. The function above on the first (v)CPU returns a clean pte when
> it should have actually been dirty at the time of read.

But I think that still conforms to semantics of the function. If you had the
same situation with a non-contpte mapping, the first thread may read the PTE at
any time; when it's dirty, or after its been cleaned by the other thread. It's
inherrently racy.

All that matters is that what is returned is _consistent_; you don't want to be
in a position where the first read of the block is mapping one folio, then by
the time all the access/dirty bits are read, the mapping is actually to a
different folio - that's an example of being inconsistent.

> 
> Throughout the callers of this function, I couldn't find one where it
> matters. So I concluded that they don't need the dirty state. Normally
> the dirty state is passed to the page flags, so not lost after the pte
> has been cleaned.

I agree we can simplify the semantics. But I think its better done in a separate
series (which I previously linked).

What's the bottom line here? Are you ok with this comment as a short term
solution for now, or do you want something more radical (i.e. push to get the
series that does these simplifications reviewed and in time for v6.9).

I still believe the current ptep_get_lockless() implementation is correct. So
given I have a plan to simplify in the long run, I hope we can still get this
series into v6.9 as planned.

Thanks,
Ryan

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Catalin Marinas]

On Mon, Mar 04, 2024 at 12:54:23PM +0000, Ryan Roberts wrote:
> On 01/03/2024 18:47, Catalin Marinas wrote:
> > On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
> >> Make clear the atmicity/consistency requirements of the API and how we
> >> achieve them.
> >>
> >> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> >> ---
> >>  arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
> >>  1 file changed, 14 insertions(+), 10 deletions(-)
[...]
> > Throughout the callers of this function, I couldn't find one where it
> > matters. So I concluded that they don't need the dirty state. Normally
> > the dirty state is passed to the page flags, so not lost after the pte
> > has been cleaned.
> 
> I agree we can simplify the semantics. But I think its better done in a separate
> series (which I previously linked).
> 
> What's the bottom line here? Are you ok with this comment as a short term
> solution for now, or do you want something more radical (i.e. push to get the
> series that does these simplifications reviewed and in time for v6.9).
> 
> I still believe the current ptep_get_lockless() implementation is correct. So
> given I have a plan to simplify in the long run, I hope we can still get this
> series into v6.9 as planned.

Yes, I'm fine with this patch. Assuming Andrew picked them up:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

I'd like to get the simplification in as well at some point as I think
our ptep_get_lockless() is unnecessarily complex for most use-cases.

-- 
Catalin

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Ryan Roberts]

On 04/03/2024 17:37, Catalin Marinas wrote:
> On Mon, Mar 04, 2024 at 12:54:23PM +0000, Ryan Roberts wrote:
>> On 01/03/2024 18:47, Catalin Marinas wrote:
>>> On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
>>>> Make clear the atmicity/consistency requirements of the API and how we
>>>> achieve them.
>>>>
>>>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>  arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>>>  1 file changed, 14 insertions(+), 10 deletions(-)
> [...]
>>> Throughout the callers of this function, I couldn't find one where it
>>> matters. So I concluded that they don't need the dirty state. Normally
>>> the dirty state is passed to the page flags, so not lost after the pte
>>> has been cleaned.
>>
>> I agree we can simplify the semantics. But I think its better done in a separate
>> series (which I previously linked).
>>
>> What's the bottom line here? Are you ok with this comment as a short term
>> solution for now, or do you want something more radical (i.e. push to get the
>> series that does these simplifications reviewed and in time for v6.9).
>>
>> I still believe the current ptep_get_lockless() implementation is correct. So
>> given I have a plan to simplify in the long run, I hope we can still get this
>> series into v6.9 as planned.
> 
> Yes, I'm fine with this patch. Assuming Andrew picked them up:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks! Yes, he did - they are in mm-unstable.

> 
> I'd like to get the simplification in as well at some point as I think
> our ptep_get_lockless() is unnecessarily complex for most use-cases.

Yes, I'll keep pushing it. I know DavidH is keen for it.

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[David Hildenbrand]

On 04.03.24 19:40, Ryan Roberts wrote:
> On 04/03/2024 17:37, Catalin Marinas wrote:
>> On Mon, Mar 04, 2024 at 12:54:23PM +0000, Ryan Roberts wrote:
>>> On 01/03/2024 18:47, Catalin Marinas wrote:
>>>> On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
>>>>> Make clear the atmicity/consistency requirements of the API and how we
>>>>> achieve them.
>>>>>
>>>>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>> ---
>>>>>   arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>>>>   1 file changed, 14 insertions(+), 10 deletions(-)
>> [...]
>>>> Throughout the callers of this function, I couldn't find one where it
>>>> matters. So I concluded that they don't need the dirty state. Normally
>>>> the dirty state is passed to the page flags, so not lost after the pte
>>>> has been cleaned.
>>>
>>> I agree we can simplify the semantics. But I think its better done in a separate
>>> series (which I previously linked).
>>>
>>> What's the bottom line here? Are you ok with this comment as a short term
>>> solution for now, or do you want something more radical (i.e. push to get the
>>> series that does these simplifications reviewed and in time for v6.9).
>>>
>>> I still believe the current ptep_get_lockless() implementation is correct. So
>>> given I have a plan to simplify in the long run, I hope we can still get this
>>> series into v6.9 as planned.
>>
>> Yes, I'm fine with this patch. Assuming Andrew picked them up:
>>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Thanks! Yes, he did - they are in mm-unstable.
> 
>>
>> I'd like to get the simplification in as well at some point as I think
>> our ptep_get_lockless() is unnecessarily complex for most use-cases.
> 
> Yes, I'll keep pushing it. I know DavidH is keen for it.

Maybe just sent a v1 (after the merge window?) if there is no further 
feedback. I still want to look into the details, but it's stuck deep in 
my inbox I'm afraid :)

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Ryan Roberts]

On 04/03/2024 22:04, David Hildenbrand wrote:
> On 04.03.24 19:40, Ryan Roberts wrote:
>> On 04/03/2024 17:37, Catalin Marinas wrote:
>>> On Mon, Mar 04, 2024 at 12:54:23PM +0000, Ryan Roberts wrote:
>>>> On 01/03/2024 18:47, Catalin Marinas wrote:
>>>>> On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
>>>>>> Make clear the atmicity/consistency requirements of the API and how we
>>>>>> achieve them.
>>>>>>
>>>>>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>>> ---
>>>>>>   arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>>>>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>> [...]
>>>>> Throughout the callers of this function, I couldn't find one where it
>>>>> matters. So I concluded that they don't need the dirty state. Normally
>>>>> the dirty state is passed to the page flags, so not lost after the pte
>>>>> has been cleaned.
>>>>
>>>> I agree we can simplify the semantics. But I think its better done in a
>>>> separate
>>>> series (which I previously linked).
>>>>
>>>> What's the bottom line here? Are you ok with this comment as a short term
>>>> solution for now, or do you want something more radical (i.e. push to get the
>>>> series that does these simplifications reviewed and in time for v6.9).
>>>>
>>>> I still believe the current ptep_get_lockless() implementation is correct. So
>>>> given I have a plan to simplify in the long run, I hope we can still get this
>>>> series into v6.9 as planned.
>>>
>>> Yes, I'm fine with this patch. Assuming Andrew picked them up:
>>>
>>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> Thanks! Yes, he did - they are in mm-unstable.
>>
>>>
>>> I'd like to get the simplification in as well at some point as I think
>>> our ptep_get_lockless() is unnecessarily complex for most use-cases.
>>
>> Yes, I'll keep pushing it. I know DavidH is keen for it.
> 
> Maybe just sent a v1 (after the merge window?) if there is no further feedback.
> I still want to look into the details, but it's stuck deep in my inbox I'm
> afraid :)

Yep will do! I will also add the final patch to actually remove
ptep_get_lockless() since it is no longer used by the end of the series.

Re: [PATCH 2/2] arm64/mm: Improve comment in contpte_ptep_get_lockless()

[Ryan Roberts]

On 04/03/2024 22:04, David Hildenbrand wrote:
> On 04.03.24 19:40, Ryan Roberts wrote:
>> On 04/03/2024 17:37, Catalin Marinas wrote:
>>> On Mon, Mar 04, 2024 at 12:54:23PM +0000, Ryan Roberts wrote:
>>>> On 01/03/2024 18:47, Catalin Marinas wrote:
>>>>> On Mon, Feb 26, 2024 at 12:03:21PM +0000, Ryan Roberts wrote:
>>>>>> Make clear the atmicity/consistency requirements of the API and how we
>>>>>> achieve them.
>>>>>>
>>>>>> Link: https://lore.kernel.org/linux-mm/Zc-Tqqfksho3BHmU@arm.com/
>>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>>> ---
>>>>>>   arch/arm64/mm/contpte.c | 24 ++++++++++++++----------
>>>>>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>> [...]
>>>>> Throughout the callers of this function, I couldn't find one where it
>>>>> matters. So I concluded that they don't need the dirty state. Normally
>>>>> the dirty state is passed to the page flags, so not lost after the pte
>>>>> has been cleaned.
>>>>
>>>> I agree we can simplify the semantics. But I think its better done in a
>>>> separate
>>>> series (which I previously linked).
>>>>
>>>> What's the bottom line here? Are you ok with this comment as a short term
>>>> solution for now, or do you want something more radical (i.e. push to get the
>>>> series that does these simplifications reviewed and in time for v6.9).
>>>>
>>>> I still believe the current ptep_get_lockless() implementation is correct. So
>>>> given I have a plan to simplify in the long run, I hope we can still get this
>>>> series into v6.9 as planned.
>>>
>>> Yes, I'm fine with this patch. Assuming Andrew picked them up:
>>>
>>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> Thanks! Yes, he did - they are in mm-unstable.
>>
>>>
>>> I'd like to get the simplification in as well at some point as I think
>>> our ptep_get_lockless() is unnecessarily complex for most use-cases.
>>
>> Yes, I'll keep pushing it. I know DavidH is keen for it.
> 
> Maybe just sent a v1 (after the merge window?) if there is no further feedback.
> I still want to look into the details, but it's stuck deep in my inbox I'm
> afraid :)

Yep will do! I will also add the final patch to actually remove
ptep_get_lockless() since it is no longer used by the end of the series.