Re: [PATCH v1] arm64/mm: Hoist synchronization out of set_ptes() loop

From: Ryan Roberts
Date: Thu Oct 05 2023 - 11:57:26 EST

On 05/10/2023 13:55, Steven Price wrote:
> On 03/10/2023 14:39, Ryan Roberts wrote:
>> set_ptes() sets a physically contiguous block of memory (which all
>> belongs to the same folio) to a contiguous block of ptes. The arm64
>> implementation of this previously just looped, operating on each
>> individual pte. But the __sync_icache_dcache() and mte_sync_tags()
>> operations can both be hoisted out of the loop so that they are
>> performed once for the contiguous set of pages (which may be less than
>> the whole folio). This should result in minor performance gains.
>>
>> __sync_icache_dcache() already acts on the whole folio, and sets a flag
>> in the folio so that it skips duplicate calls. But by hoisting the call,
>> all the pte testing is done only once.
>>
>> mte_sync_tags() operates on each individual page with its own loop. But
>> by passing the number of pages explicitly, we can rely solely on its
>> loop and do the checks only once. This approach also makes it robust for
>> the future, rather than assuming if a head page of a compound page is
>> being mapped, then the whole compound page is being mapped, instead we
>> explicitly know how many pages are being mapped. The old assumption may
>> not continue to hold once the "anonymous large folios" feature is
>> merged.
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
>> ---
>> arch/arm64/include/asm/mte.h | 4 ++--
>> arch/arm64/include/asm/pgtable.h | 27 +++++++++++++++++----------
>> arch/arm64/kernel/mte.c | 4 ++--
>> 3 files changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
>> index 4cedbaa16f41..91fbd5c8a391 100644
>> --- a/arch/arm64/include/asm/mte.h
>> +++ b/arch/arm64/include/asm/mte.h
>> @@ -90,7 +90,7 @@ static inline bool try_page_mte_tagging(struct page *page)
>> }
>>
>> void mte_zero_clear_page_tags(void *addr);
>> -void mte_sync_tags(pte_t pte);
>> +void mte_sync_tags(pte_t pte, unsigned int nr_pages);
>> void mte_copy_page_tags(void *kto, const void *kfrom);
>> void mte_thread_init_user(void);
>> void mte_thread_switch(struct task_struct *next);
>> @@ -122,7 +122,7 @@ static inline bool try_page_mte_tagging(struct page *page)
>> static inline void mte_zero_clear_page_tags(void *addr)
>> {
>> }
>> -static inline void mte_sync_tags(pte_t pte)
>> +static inline void mte_sync_tags(pte_t pte, unsigned int nr_pages)
>> {
>> }
>> static inline void mte_copy_page_tags(void *kto, const void *kfrom)
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 7f7d9b1df4e5..374c1c1485f9 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -325,8 +325,7 @@ static inline void __check_safe_pte_update(struct mm_struct *mm, pte_t *ptep,
>> __func__, pte_val(old_pte), pte_val(pte));
>> }
>>
>> -static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>> - pte_t *ptep, pte_t pte)
>> +static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
>> {
>> if (pte_present(pte) && pte_user_exec(pte) && !pte_special(pte))
>> __sync_icache_dcache(pte);
>> @@ -339,20 +338,18 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>> */
>> if (system_supports_mte() && pte_access_permitted(pte, false) &&
>> !pte_special(pte) && pte_tagged(pte))
>> - mte_sync_tags(pte);
>> -
>> - __check_safe_pte_update(mm, ptep, pte);
>> -
>> - set_pte(ptep, pte);
>> + mte_sync_tags(pte, nr_pages);
>> }
>>
>> static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>> pte_t *ptep, pte_t pte, unsigned int nr)
>> {
>> page_table_check_ptes_set(mm, ptep, pte, nr);
>> + __sync_cache_and_tags(pte, nr);
>>
>> for (;;) {
>> - __set_pte_at(mm, addr, ptep, pte);
>> + __check_safe_pte_update(mm, ptep, pte);
>> + set_pte(ptep, pte);
>> if (--nr == 0)
>> break;
>> ptep++;
>> @@ -531,18 +528,28 @@ static inline pmd_t pmd_mkdevmap(pmd_t pmd)
>> #define pud_pfn(pud) ((__pud_to_phys(pud) & PUD_MASK) >> PAGE_SHIFT)
>> #define pfn_pud(pfn,prot) __pud(__phys_to_pud_val((phys_addr_t)(pfn) << PAGE_SHIFT) | pgprot_val(prot))
>>
>> +static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>> + pte_t *ptep, pte_t pte, unsigned int nr)
>> +{
>> + __sync_cache_and_tags(pte, nr);
>> + __check_safe_pte_update(mm, ptep, pte);
>> + set_pte(ptep, pte);
>> +}
>> +
>> static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
>> pmd_t *pmdp, pmd_t pmd)
>> {
>> page_table_check_pmd_set(mm, pmdp, pmd);
>> - return __set_pte_at(mm, addr, (pte_t *)pmdp, pmd_pte(pmd));
>> + return __set_pte_at(mm, addr, (pte_t *)pmdp, pmd_pte(pmd),
>> + PMD_SHIFT - PAGE_SHIFT);
>
> IIUC the new __set_pte_at takes the number of pages, but PMD_SHIFT -
> PAGE_SHIFT is the log2 of that. Should this be 1 << (PMD_SHIFT -
> PAGE_SHIFT)? Same below for pud.

Ouch - good spot! I'll resubmit a fixed version.

>
> Steve
>
>> }
>>
>> static inline void set_pud_at(struct mm_struct *mm, unsigned long addr,
>> pud_t *pudp, pud_t pud)
>> {
>> page_table_check_pud_set(mm, pudp, pud);
>> - return __set_pte_at(mm, addr, (pte_t *)pudp, pud_pte(pud));
>> + return __set_pte_at(mm, addr, (pte_t *)pudp, pud_pte(pud),
>> + PUD_SHIFT - PAGE_SHIFT);
>> }
>>
>> #define __p4d_to_phys(p4d) __pte_to_phys(p4d_pte(p4d))
>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index 4edecaac8f91..2fb5e7a7a4d5 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -35,10 +35,10 @@ DEFINE_STATIC_KEY_FALSE(mte_async_or_asymm_mode);
>> EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode);
>> #endif
>>
>> -void mte_sync_tags(pte_t pte)
>> +void mte_sync_tags(pte_t pte, unsigned int nr_pages)
>> {
>> struct page *page = pte_page(pte);
>> - long i, nr_pages = compound_nr(page);
>> + unsigned int i;
>>
>> /* if PG_mte_tagged is set, tags have already been initialised */
>> for (i = 0; i < nr_pages; i++, page++) {
>> --
>> 2.25.1
>>
>