diff mbox series

[v2] arm64/mm: Hoist synchronization out of set_ptes() loop

Message ID 20231005140730.2191134-1-ryan.roberts@arm.com (mailing list archive)
State New, archived
Headers show
Series [v2] arm64/mm: Hoist synchronization out of set_ptes() loop | expand

Commit Message

Ryan Roberts Oct. 5, 2023, 2:07 p.m. UTC 
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@arm.com>
---
v2 fixes the invocations of __set_pte_at() to pass nr_pages rather than order.
Thanks to Steven Price for pointing that out.

 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(-)

--
2.25.1

Comments

Steven Price Oct. 5, 2023, 2:41 p.m. UTC | #1 
On 05/10/2023 15:07, 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@arm.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
> v2 fixes the invocations of __set_pte_at() to pass nr_pages rather than order.
> Thanks to Steven Price for pointing that out.
> 
>  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..68984ba9ce2a 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_SIZE >> PAGE_SHIFT);
>  }
> 
>  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_SIZE >> 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
>
kernel test robot Oct. 13, 2023, 9:15 p.m. UTC | #2 
Hi Ryan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on arm-perf/for-next/perf arm/for-next kvmarm/next soc/for-next linus/master v6.6-rc5 next-20231013]
[cannot apply to arm/fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Roberts/arm64-mm-Hoist-synchronization-out-of-set_ptes-loop/20231005-231636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20231005140730.2191134-1-ryan.roberts%40arm.com
patch subject: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from net/ipv4/route.c:66:
   In file included from include/linux/mm.h:29:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   1 warning generated.
--
   In file included from sound/soc/qcom/qdsp6/q6apm-dai.c:9:
   In file included from include/sound/soc.h:18:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/arm64/include/asm/hardirq.h:17:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   sound/soc/qcom/qdsp6/q6apm-dai.c:355:38: warning: cast from 'void (*)(uint32_t, uint32_t, uint32_t *, void *)' (aka 'void (*)(unsigned int, unsigned int, unsigned int *, void *)') to 'q6apm_cb' (aka 'void (*)(unsigned int, unsigned int, void *, void *)') converts to incompatible function type [-Wcast-function-type-strict]
     355 |         prtd->graph = q6apm_graph_open(dev, (q6apm_cb)event_handler, prtd, graph_id);
         |                                             ^~~~~~~~~~~~~~~~~~~~~~~
   sound/soc/qcom/qdsp6/q6apm-dai.c:499:38: warning: cast from 'void (*)(uint32_t, uint32_t, uint32_t *, void *)' (aka 'void (*)(unsigned int, unsigned int, unsigned int *, void *)') to 'q6apm_cb' (aka 'void (*)(unsigned int, unsigned int, void *, void *)') converts to incompatible function type [-Wcast-function-type-strict]
     499 |         prtd->graph = q6apm_graph_open(dev, (q6apm_cb)event_handler_compr, prtd, graph_id);
         |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   3 warnings generated.
--
   In file included from arch/arm64/kernel/mte.c:9:
   In file included from include/linux/mm.h:29:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   arch/arm64/kernel/mte.c:79:20: warning: unused function '__mte_enable_kernel' [-Wunused-function]
      79 | static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
         |                    ^
   2 warnings generated.
--
   In file included from drivers/gpu/drm/radeon/radeon_ttm.c:33:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:29:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/radeon/radeon_ttm.c:200:20: warning: variable 'rbo' set but not used [-Wunused-but-set-variable]
     200 |         struct radeon_bo *rbo;
         |                           ^
   2 warnings generated.
--
   In file included from drivers/gpu/drm/kmb/kmb_dsi.c:12:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/kmb/kmb_dsi.c:822:2: warning: unused function 'set_test_mode_src_osc_freq_target_low_bits' [-Wunused-function]
     822 |         set_test_mode_src_osc_freq_target_low_bits(struct kmb_dsi *kmb_dsi,
         |         ^
   drivers/gpu/drm/kmb/kmb_dsi.c:834:2: warning: unused function 'set_test_mode_src_osc_freq_target_hi_bits' [-Wunused-function]
     834 |         set_test_mode_src_osc_freq_target_hi_bits(struct kmb_dsi *kmb_dsi,
         |         ^
   3 warnings generated.
--
   In file included from drivers/gpu/drm/nouveau/nouveau_svm.c:22:
   In file included from drivers/gpu/drm/nouveau/nouveau_svm.h:3:
   In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:8:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/arm64/include/asm/hardirq.h:17:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/nouveau/nouveau_svm.c:115:24: warning: variable 'priority' set but not used [-Wunused-but-set-variable]
     115 |         unsigned target, cmd, priority;
         |                               ^
   drivers/gpu/drm/nouveau/nouveau_svm.c:929:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
     929 |         int ret;
         |             ^
   3 warnings generated.
--
   In file included from drivers/gpu/drm/nouveau/nouveau_connector.c:30:
   In file included from include/linux/vga_switcheroo.h:34:
   In file included from include/linux/fb.h:6:
   In file included from include/linux/kgdb.h:19:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/arm64/include/asm/hardirq.h:17:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/nouveau/nouveau_connector.c:1301:7: warning: variable 'entry' set but not used [-Wunused-but-set-variable]
    1301 |                 u32 entry = ROM16(nv_connector->dcb[0]);
         |                     ^
   2 warnings generated.
--
   In file included from drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c:22:
   In file included from drivers/gpu/drm/nouveau/nvkm/subdev/acr/priv.h:3:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/subdev/acr.h:5:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/device.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/oclass.h:3:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/os.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:8:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/arm64/include/asm/hardirq.h:17:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c:221:7: warning: variable 'loc' set but not used [-Wunused-but-set-variable]
     221 |                 u32 loc, sig, cnt, *meta;
         |                     ^
   2 warnings generated.
--
   In file included from drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadow.c:24:
   In file included from drivers/gpu/drm/nouveau/nvkm/subdev/bios/priv.h:5:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/subdev/bios.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/subdev.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/device.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/oclass.h:3:
   In file included from drivers/gpu/drm/nouveau/include/nvkm/core/os.h:4:
   In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:8:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/arm64/include/asm/hardirq.h:17:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadow.c:161:10: warning: cast from 'void (*)(const struct firmware *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
     161 |         .fini = (void(*)(void *))release_firmware,
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 warnings generated.
--
   In file included from drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c:8:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c:35:19: warning: unused function 'rcar_cmm_read' [-Wunused-function]
      35 | static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
         |                   ^
   2 warnings generated.
--
   In file included from drivers/gpu/drm/qxl/qxl_cmd.c:30:
   In file included from include/drm/drm_util.h:35:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/arm64/include/asm/hardirq.h:17:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/arm64/include/asm/io.h:12:
   In file included from include/linux/pgtable.h:6:
>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
     344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
         |                                                                 ^
   drivers/gpu/drm/qxl/qxl_cmd.c:424:6: warning: variable 'count' set but not used [-Wunused-but-set-variable]
     424 |         int count = 0;
         |             ^
   2 warnings generated.
..


vim +/addr +344 arch/arm64/include/asm/pgtable.h

4f04d8f0054511 Catalin Marinas         2012-03-05  343  
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02 @344) static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  345) 			      pte_t *ptep, pte_t pte, unsigned int nr)
42b2547137f5c9 Kefeng Wang             2022-05-12  346  {
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  347) 	page_table_check_ptes_set(mm, ptep, pte, nr);
3ba82bb647345d Ryan Roberts            2023-10-05  348  	__sync_cache_and_tags(pte, nr);
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  349) 
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  350) 	for (;;) {
3ba82bb647345d Ryan Roberts            2023-10-05  351  		__check_safe_pte_update(mm, ptep, pte);
3ba82bb647345d Ryan Roberts            2023-10-05  352  		set_pte(ptep, pte);
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  353) 		if (--nr == 0)
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  354) 			break;
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  355) 		ptep++;
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  356) 		addr += PAGE_SIZE;
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  357) 		pte_val(pte) += PAGE_SIZE;
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  358) 	}
42b2547137f5c9 Kefeng Wang             2022-05-12  359  }
4a169d61c2ede9 Matthew Wilcox (Oracle  2023-08-02  360) #define set_ptes set_ptes
42b2547137f5c9 Kefeng Wang             2022-05-12  361
Catalin Marinas Oct. 16, 2023, 5:54 p.m. UTC | #3 
On Sat, Oct 14, 2023 at 05:15:51AM +0800, kernel test robot wrote:
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on arm64/for-next/core]
> [also build test WARNING on arm-perf/for-next/perf arm/for-next kvmarm/next soc/for-next linus/master v6.6-rc5 next-20231013]
> [cannot apply to arm/fixes]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Roberts/arm64-mm-Hoist-synchronization-out-of-set_ptes-loop/20231005-231636
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> patch link:    https://lore.kernel.org/r/20231005140730.2191134-1-ryan.roberts%40arm.com
> patch subject: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
> config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/config)
> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from net/ipv4/route.c:66:
>    In file included from include/linux/mm.h:29:
>    In file included from include/linux/pgtable.h:6:
> >> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
>      344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>          |                                                                 ^
>    1 warning generated.

Thanks for the report. I think something like below will do (I'll test
and commit as a separate patch, it's not something that Ryan's patch
introduces):

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 68984ba9ce2a..b19a8aee684c 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -341,8 +341,9 @@ static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
 		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)
+static inline void set_ptes(struct mm_struct *mm,
+			    unsigned long __always_unused 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);
@@ -353,7 +354,6 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
 		if (--nr == 0)
 			break;
 		ptep++;
-		addr += PAGE_SIZE;
 		pte_val(pte) += PAGE_SIZE;
 	}
 }
@@ -528,7 +528,8 @@ 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,
+static inline void __set_pte_at(struct mm_struct *mm,
+				unsigned long __always_unused addr,
 				pte_t *ptep, pte_t pte, unsigned int nr)
 {
 	__sync_cache_and_tags(pte, nr);
Ryan Roberts Oct. 17, 2023, 7:36 a.m. UTC | #4 
On 16/10/2023 18:54, Catalin Marinas wrote:
> On Sat, Oct 14, 2023 at 05:15:51AM +0800, kernel test robot wrote:
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on arm64/for-next/core]
>> [also build test WARNING on arm-perf/for-next/perf arm/for-next kvmarm/next soc/for-next linus/master v6.6-rc5 next-20231013]
>> [cannot apply to arm/fixes]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Roberts/arm64-mm-Hoist-synchronization-out-of-set_ptes-loop/20231005-231636
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
>> patch link:    https://lore.kernel.org/r/20231005140730.2191134-1-ryan.roberts%40arm.com
>> patch subject: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
>> config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/config)
>> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
>>
>> All warnings (new ones prefixed by >>):
>>
>>    In file included from net/ipv4/route.c:66:
>>    In file included from include/linux/mm.h:29:
>>    In file included from include/linux/pgtable.h:6:
>>>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
>>      344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>>          |                                                                 ^
>>    1 warning generated.
> 
> Thanks for the report. I think something like below will do (I'll test
> and commit as a separate patch, it's not something that Ryan's patch
> introduces):

I was actually just trying to repro this and was planning to send out a v3 of my
patch. But if you are happy to handle it as you suggest, then I guess you don't
need anything further from me?

Thanks,
Ryan


> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 68984ba9ce2a..b19a8aee684c 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -341,8 +341,9 @@ static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
>  		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)
> +static inline void set_ptes(struct mm_struct *mm,
> +			    unsigned long __always_unused 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);
> @@ -353,7 +354,6 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>  		if (--nr == 0)
>  			break;
>  		ptep++;
> -		addr += PAGE_SIZE;
>  		pte_val(pte) += PAGE_SIZE;
>  	}
>  }
> @@ -528,7 +528,8 @@ 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,
> +static inline void __set_pte_at(struct mm_struct *mm,
> +				unsigned long __always_unused addr,
>  				pte_t *ptep, pte_t pte, unsigned int nr)
>  {
>  	__sync_cache_and_tags(pte, nr);
>
Catalin Marinas Oct. 17, 2023, 12:57 p.m. UTC | #5 
On Tue, Oct 17, 2023 at 08:36:43AM +0100, Ryan Roberts wrote:
> On 16/10/2023 18:54, Catalin Marinas wrote:
> > On Sat, Oct 14, 2023 at 05:15:51AM +0800, kernel test robot wrote:
> >> kernel test robot noticed the following build warnings:
> >>
> >> [auto build test WARNING on arm64/for-next/core]
> >> [also build test WARNING on arm-perf/for-next/perf arm/for-next kvmarm/next soc/for-next linus/master v6.6-rc5 next-20231013]
> >> [cannot apply to arm/fixes]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>
> >> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Roberts/arm64-mm-Hoist-synchronization-out-of-set_ptes-loop/20231005-231636
> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> >> patch link:    https://lore.kernel.org/r/20231005140730.2191134-1-ryan.roberts%40arm.com
> >> patch subject: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
> >> config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/config)
> >> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> >> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/reproduce)
> >>
> >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >> the same patch/commit), kindly add following tags
> >> | Reported-by: kernel test robot <lkp@intel.com>
> >> | Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
> >>
> >> All warnings (new ones prefixed by >>):
> >>
> >>    In file included from net/ipv4/route.c:66:
> >>    In file included from include/linux/mm.h:29:
> >>    In file included from include/linux/pgtable.h:6:
> >>>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
> >>      344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> >>          |                                                                 ^
> >>    1 warning generated.
> > 
> > Thanks for the report. I think something like below will do (I'll test
> > and commit as a separate patch, it's not something that Ryan's patch
> > introduces):
> 
> I was actually just trying to repro this and was planning to send out a v3 of my
> patch. But if you are happy to handle it as you suggest, then I guess you don't
> need anything further from me?

If you feel like testing, please give this a go ;)

------------8<---------------------------
From e6255237acfc21e92252653c3ed42446ef67f625 Mon Sep 17 00:00:00 2001
From: Catalin Marinas <catalin.marinas@arm.com>
Date: Tue, 17 Oct 2023 11:57:55 +0100
Subject: [PATCH] arm64: Mark the 'addr' argument to set_ptes() and
 __set_pte_at() as unused

This argument is not used by the arm64 implementation. Mark it as
__always_unused and also remove the unnecessary 'addr' increment in
set_ptes().

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
Cc: Will Deacon <will@kernel.org>
Cc: Ryan Roberts <ryan.roberts@arm.com>
---
 arch/arm64/include/asm/pgtable.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 68984ba9ce2a..b19a8aee684c 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -341,8 +341,9 @@ static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
 		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)
+static inline void set_ptes(struct mm_struct *mm,
+			    unsigned long __always_unused 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);
@@ -353,7 +354,6 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
 		if (--nr == 0)
 			break;
 		ptep++;
-		addr += PAGE_SIZE;
 		pte_val(pte) += PAGE_SIZE;
 	}
 }
@@ -528,7 +528,8 @@ 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,
+static inline void __set_pte_at(struct mm_struct *mm,
+				unsigned long __always_unused addr,
 				pte_t *ptep, pte_t pte, unsigned int nr)
 {
 	__sync_cache_and_tags(pte, nr);
Ryan Roberts Oct. 18, 2023, 8:21 a.m. UTC | #6 
On 17/10/2023 13:57, Catalin Marinas wrote:
> On Tue, Oct 17, 2023 at 08:36:43AM +0100, Ryan Roberts wrote:
>> On 16/10/2023 18:54, Catalin Marinas wrote:
>>> On Sat, Oct 14, 2023 at 05:15:51AM +0800, kernel test robot wrote:
>>>> kernel test robot noticed the following build warnings:
>>>>
>>>> [auto build test WARNING on arm64/for-next/core]
>>>> [also build test WARNING on arm-perf/for-next/perf arm/for-next kvmarm/next soc/for-next linus/master v6.6-rc5 next-20231013]
>>>> [cannot apply to arm/fixes]
>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>
>>>> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Roberts/arm64-mm-Hoist-synchronization-out-of-set_ptes-loop/20231005-231636
>>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
>>>> patch link:    https://lore.kernel.org/r/20231005140730.2191134-1-ryan.roberts%40arm.com
>>>> patch subject: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
>>>> config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/config)
>>>> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
>>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/reproduce)
>>>>
>>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>>> the same patch/commit), kindly add following tags
>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
>>>>
>>>> All warnings (new ones prefixed by >>):
>>>>
>>>>    In file included from net/ipv4/route.c:66:
>>>>    In file included from include/linux/mm.h:29:
>>>>    In file included from include/linux/pgtable.h:6:
>>>>>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
>>>>      344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>>>>          |                                                                 ^
>>>>    1 warning generated.
>>>
>>> Thanks for the report. I think something like below will do (I'll test
>>> and commit as a separate patch, it's not something that Ryan's patch
>>> introduces):
>>
>> I was actually just trying to repro this and was planning to send out a v3 of my
>> patch. But if you are happy to handle it as you suggest, then I guess you don't
>> need anything further from me?
> 
> If you feel like testing, please give this a go ;)

Compile tested and observed that warning is gone with your change. Also ran mm
selftests and all looks good. So:

Tested-by: Ryan Roberts <ryan.roberts@arm.com>


> 
> ------------8<---------------------------
> From e6255237acfc21e92252653c3ed42446ef67f625 Mon Sep 17 00:00:00 2001
> From: Catalin Marinas <catalin.marinas@arm.com>
> Date: Tue, 17 Oct 2023 11:57:55 +0100
> Subject: [PATCH] arm64: Mark the 'addr' argument to set_ptes() and
>  __set_pte_at() as unused
> 
> This argument is not used by the arm64 implementation. Mark it as
> __always_unused and also remove the unnecessary 'addr' increment in
> set_ptes().
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
> Cc: Will Deacon <will@kernel.org>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> ---
>  arch/arm64/include/asm/pgtable.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 68984ba9ce2a..b19a8aee684c 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -341,8 +341,9 @@ static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
>  		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)
> +static inline void set_ptes(struct mm_struct *mm,
> +			    unsigned long __always_unused addr,

Personally I'm not a huge fan of the __always_unused mark up, given that it's
used so patchily in the source. The warning also disappears without these
markups. (the real problem is just the `addr += PAGE_SIZE` below.

Thanks,
Ryan


> +			    pte_t *ptep, pte_t pte, unsigned int nr)
>  {
>  	page_table_check_ptes_set(mm, ptep, pte, nr);
>  	__sync_cache_and_tags(pte, nr);
> @@ -353,7 +354,6 @@ static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
>  		if (--nr == 0)
>  			break;
>  		ptep++;
> -		addr += PAGE_SIZE;
>  		pte_val(pte) += PAGE_SIZE;
>  	}
>  }
> @@ -528,7 +528,8 @@ 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,
> +static inline void __set_pte_at(struct mm_struct *mm,
> +				unsigned long __always_unused addr,
>  				pte_t *ptep, pte_t pte, unsigned int nr)
>  {
>  	__sync_cache_and_tags(pte, nr);
Catalin Marinas Oct. 18, 2023, 9:36 a.m. UTC | #7 
On Wed, Oct 18, 2023 at 09:21:02AM +0100, Ryan Roberts wrote:
> On 17/10/2023 13:57, Catalin Marinas wrote:
> > On Tue, Oct 17, 2023 at 08:36:43AM +0100, Ryan Roberts wrote:
> >> On 16/10/2023 18:54, Catalin Marinas wrote:
> >>> On Sat, Oct 14, 2023 at 05:15:51AM +0800, kernel test robot wrote:
> >>>> kernel test robot noticed the following build warnings:
> >>>>
> >>>> [auto build test WARNING on arm64/for-next/core]
> >>>> [also build test WARNING on arm-perf/for-next/perf arm/for-next kvmarm/next soc/for-next linus/master v6.6-rc5 next-20231013]
> >>>> [cannot apply to arm/fixes]
> >>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >>>> And when submitting patch, we suggest to use '--base' as documented in
> >>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>>>
> >>>> url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Roberts/arm64-mm-Hoist-synchronization-out-of-set_ptes-loop/20231005-231636
> >>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> >>>> patch link:    https://lore.kernel.org/r/20231005140730.2191134-1-ryan.roberts%40arm.com
> >>>> patch subject: [PATCH v2] arm64/mm: Hoist synchronization out of set_ptes() loop
> >>>> config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/config)
> >>>> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
> >>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231014/202310140531.BQQwt3NQ-lkp@intel.com/reproduce)
> >>>>
> >>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >>>> the same patch/commit), kindly add following tags
> >>>> | Reported-by: kernel test robot <lkp@intel.com>
> >>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310140531.BQQwt3NQ-lkp@intel.com/
> >>>>
> >>>> All warnings (new ones prefixed by >>):
> >>>>
> >>>>    In file included from net/ipv4/route.c:66:
> >>>>    In file included from include/linux/mm.h:29:
> >>>>    In file included from include/linux/pgtable.h:6:
> >>>>>> arch/arm64/include/asm/pgtable.h:344:65: warning: parameter 'addr' set but not used [-Wunused-but-set-parameter]
> >>>>      344 | static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> >>>>          |                                                                 ^
> >>>>    1 warning generated.
> >>>
> >>> Thanks for the report. I think something like below will do (I'll test
> >>> and commit as a separate patch, it's not something that Ryan's patch
> >>> introduces):
> >>
> >> I was actually just trying to repro this and was planning to send out a v3 of my
> >> patch. But if you are happy to handle it as you suggest, then I guess you don't
> >> need anything further from me?
> > 
> > If you feel like testing, please give this a go ;)
> 
> Compile tested and observed that warning is gone with your change. Also ran mm
> selftests and all looks good. So:
> 
> Tested-by: Ryan Roberts <ryan.roberts@arm.com>

Thanks. I'll push this patch on top of yours.
Catalin Marinas Oct. 18, 2023, 10:15 a.m. UTC | #8 
On Thu, 05 Oct 2023 15:07:30 +0100, 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.
> 
> [...]

Applied to arm64 (for-next/misc), thanks!

[1/1] arm64/mm: Hoist synchronization out of set_ptes() loop
      https://git.kernel.org/arm64/c/3425cec42c3c

Also pushed the fix to mark 'addr' __always_unused.
diff mbox series

Patch

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..68984ba9ce2a 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_SIZE >> PAGE_SHIFT);
 }

 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_SIZE >> 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++) {