| Message ID | 20240917073117.1531207-8-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Use pxdp_get() for accessing page table entries | expand |
Hi Anshuman, kernel test robot noticed the following build errors: [auto build test ERROR on char-misc/char-misc-testing] [also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus brauner-vfs/vfs.all dennis-percpu/for-next linus/master v6.11] [cannot apply to akpm-mm/mm-everything next-20240918] [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/Anshuman-Khandual/m68k-mm-Change-pmd_val/20240917-153331 base: char-misc/char-misc-testing patch link: https://lore.kernel.org/r/20240917073117.1531207-8-anshuman.khandual%40arm.com patch subject: [PATCH V2 7/7] mm: Use pgdp_get() for accessing PGD entries config: arm-footbridge_defconfig (https://download.01.org/0day-ci/archive/20240919/202409190310.ViHBRe12-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240919/202409190310.ViHBRe12-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/202409190310.ViHBRe12-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:30: >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/pgtable.h:1243:48: note: 'pgd' declared here 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) | ^ >> include/linux/pgtable.h:1245:8: error: array initializer must be an initializer list or wide string literal 1245 | pgd_t old_pgd = pgdp_get(pgd); | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:98:11: warning: array index 3 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 98 | return (set->sig[3] | set->sig[2] | | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:98:25: warning: array index 2 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 98 | return (set->sig[3] | set->sig[2] | | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:114:11: warning: array index 3 is past the end of the array (that has type 'const unsigned long[2]') [-Warray-bounds] 114 | return (set1->sig[3] == set2->sig[3]) && | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:114:27: warning: array index 3 is past the end of the array (that has type 'const unsigned long[2]') [-Warray-bounds] 114 | return (set1->sig[3] == set2->sig[3]) && | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:115:5: warning: array index 2 is past the end of the array (that has type 'const unsigned long[2]') [-Warray-bounds] 115 | (set1->sig[2] == set2->sig[2]) && | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:115:21: warning: array index 2 is past the end of the array (that has type 'const unsigned long[2]') [-Warray-bounds] 115 | (set1->sig[2] == set2->sig[2]) && | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:157:1: warning: array index 3 is past the end of the array (that has type 'const unsigned long[2]') [-Warray-bounds] 157 | _SIG_SET_BINOP(sigorsets, _sig_or) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:138:8: note: expanded from macro '_SIG_SET_BINOP' 138 | a3 = a->sig[3]; a2 = a->sig[2]; \ | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: -- 163 | _SIG_SET_BINOP(sigandnsets, _sig_andn) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:140:3: note: expanded from macro '_SIG_SET_BINOP' 140 | r->sig[3] = op(a3, b3); \ | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:163:1: warning: array index 2 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 163 | _SIG_SET_BINOP(sigandnsets, _sig_andn) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:141:3: note: expanded from macro '_SIG_SET_BINOP' 141 | r->sig[2] = op(a2, b2); \ | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:187:1: warning: array index 3 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 187 | _SIG_SET_OP(signotset, _sig_not) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:174:27: note: expanded from macro '_SIG_SET_OP' 174 | case 4: set->sig[3] = op(set->sig[3]); \ | ^ ~ include/linux/signal.h:186:24: note: expanded from macro '_sig_not' 186 | #define _sig_not(x) (~(x)) | ^ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:187:1: warning: array index 3 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 187 | _SIG_SET_OP(signotset, _sig_not) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:174:10: note: expanded from macro '_SIG_SET_OP' 174 | case 4: set->sig[3] = op(set->sig[3]); \ | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:187:1: warning: array index 2 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 187 | _SIG_SET_OP(signotset, _sig_not) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:175:20: note: expanded from macro '_SIG_SET_OP' 175 | set->sig[2] = op(set->sig[2]); \ | ^ ~ include/linux/signal.h:186:24: note: expanded from macro '_sig_not' 186 | #define _sig_not(x) (~(x)) | ^ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:1131: In file included from include/linux/huge_mm.h:8: In file included from include/linux/fs.h:33: In file included from include/linux/percpu-rwsem.h:7: In file included from include/linux/rcuwait.h:6: In file included from include/linux/sched/signal.h:6: include/linux/signal.h:187:1: warning: array index 2 is past the end of the array (that has type 'unsigned long[2]') [-Warray-bounds] 187 | _SIG_SET_OP(signotset, _sig_not) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/signal.h:175:3: note: expanded from macro '_SIG_SET_OP' 175 | set->sig[2] = op(set->sig[2]); \ | ^ ~ arch/arm/include/asm/signal.h:17:2: note: array 'sig' declared here 17 | unsigned long sig[_NSIG_WORDS]; | ^ In file included from arch/arm/kernel/asm-offsets.c:12: In file included from include/linux/mm.h:2232: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 517 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ In file included from arch/arm/kernel/asm-offsets.c:12: >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ^ arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/linux/mm.h:2819:61: note: 'pgd' declared here 2819 | static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, | ^ >> include/linux/mm.h:2822:28: error: passing 'const volatile pmdval_t *' (aka 'const volatile unsigned int *') to parameter of type 'pmdval_t *' (aka 'unsigned int *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 2822 | return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? | ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~ arch/arm/include/asm/pgtable.h:154:25: note: expanded from macro 'pgdp_get' 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) | ^ include/asm-generic/rwonce.h:47:28: note: expanded from macro 'READ_ONCE' 47 | #define READ_ONCE(x) \ | ^ include/linux/compiler.h:77:42: note: expanded from macro 'unlikely' 77 | # define unlikely(x) __builtin_expect(!!(x), 0) | ^ include/asm-generic/pgtable-nop4d.h:21:34: note: passing argument to parameter 'pgd' here 21 | static inline int pgd_none(pgd_t pgd) { return 0; } | ^ 29 warnings and 18 errors generated. make[3]: *** [scripts/Makefile.build:117: arch/arm/kernel/asm-offsets.s] Error 1 make[3]: Target 'prepare' not remade because of errors. make[2]: *** [Makefile:1194: prepare0] Error 2 make[2]: Target 'prepare' not remade because of errors. make[1]: *** [Makefile:224: __sub-make] Error 2 make[1]: Target 'prepare' not remade because of errors. make: *** [Makefile:224: __sub-make] Error 2 make: Target 'prepare' not remade because of errors. vim +1245 include/linux/pgtable.h 1242 1243 static inline int pgd_none_or_clear_bad(pgd_t *pgd) 1244 { > 1245 pgd_t old_pgd = pgdp_get(pgd); 1246 1247 if (pgd_none(old_pgd)) 1248 return 1; 1249 if (unlikely(pgd_bad(old_pgd))) { 1250 pgd_clear_bad(pgd); 1251 return 1; 1252 } 1253 return 0; 1254 } 1255
On 9/19/24 02:00, kernel test robot wrote: > Hi Anshuman, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on char-misc/char-misc-testing] > [also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus brauner-vfs/vfs.all dennis-percpu/for-next linus/master v6.11] > [cannot apply to akpm-mm/mm-everything next-20240918] > [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/Anshuman-Khandual/m68k-mm-Change-pmd_val/20240917-153331 > base: char-misc/char-misc-testing > patch link: https://lore.kernel.org/r/20240917073117.1531207-8-anshuman.khandual%40arm.com > patch subject: [PATCH V2 7/7] mm: Use pgdp_get() for accessing PGD entries > config: arm-footbridge_defconfig (https://download.01.org/0day-ci/archive/20240919/202409190310.ViHBRe12-lkp@intel.com/config) > compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88) > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240919/202409190310.ViHBRe12-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/202409190310.ViHBRe12-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > In file included from arch/arm/kernel/asm-offsets.c:12: > In file included from include/linux/mm.h:30: >>> include/linux/pgtable.h:1245:18: error: use of undeclared identifier 'pgdp'; did you mean 'pgd'? > 1245 | pgd_t old_pgd = pgdp_get(pgd); > | ^ > arch/arm/include/asm/pgtable.h:154:36: note: expanded from macro 'pgdp_get' > 154 | #define pgdp_get(pgpd) READ_ONCE(*pgdp) > | ^ > include/linux/pgtable.h:1243:48: note: 'pgd' declared here > 1243 | static inline int pgd_none_or_clear_bad(pgd_t *pgd) > | ^ arm (32) platform currently overrides pgdp_get() helper in the platform but defines that like the exact same version as the generic one, albeit with a typo which can be fixed with something like this. diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h index be91e376df79..aedb32d49c2a 100644 --- a/arch/arm/include/asm/pgtable.h +++ b/arch/arm/include/asm/pgtable.h @@ -151,7 +151,7 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, extern pgd_t swapper_pg_dir[PTRS_PER_PGD]; -#define pgdp_get(pgpd) READ_ONCE(*pgdp) +#define pgdp_get(pgdp) READ_ONCE(*pgdp) #define pud_page(pud) pmd_page(__pmd(pud_val(pud))) #define pud_write(pud) pmd_write(__pmd(pud_val(pud))) Regardless there is another problem here. On arm platform there are multiple pgd_t definitions available depending on various configs but some are arrays instead of a single data element, although platform pgdp_get() helper remains the same for all. arch/arm/include/asm/page-nommu.h:typedef unsigned long pgd_t[2]; arch/arm/include/asm/pgtable-2level-types.h:typedef struct { pmdval_t pgd[2]; } pgd_t; arch/arm/include/asm/pgtable-2level-types.h:typedef pmdval_t pgd_t[2]; arch/arm/include/asm/pgtable-3level-types.h:typedef struct { pgdval_t pgd; } pgd_t; arch/arm/include/asm/pgtable-3level-types.h:typedef pgdval_t pgd_t; I guess it might need different pgdp_get() variants depending applicable pgd_t definition. Will continue looking into this further but meanwhile copied Russel King in case he might be able to give some direction.
On Thu, Sep 19, 2024 at 01:25:08PM +0530, Anshuman Khandual wrote: > arm (32) platform currently overrides pgdp_get() helper in the platform but > defines that like the exact same version as the generic one, albeit with a > typo which can be fixed with something like this. pgdp_get() was added to arm in eba2591d99d1 ("mm: Introduce pudp/p4dp/pgdp_get() functions") with the typo you've spotted. It seems it was added with no users, otherwise the error would have been spotted earlier. I'm not a fan of adding dead code to the kernel for this reason. > Regardless there is another problem here. On arm platform there are multiple > pgd_t definitions available depending on various configs but some are arrays > instead of a single data element, although platform pgdp_get() helper remains > the same for all. > > arch/arm/include/asm/page-nommu.h:typedef unsigned long pgd_t[2]; > arch/arm/include/asm/pgtable-2level-types.h:typedef struct { pmdval_t pgd[2]; } pgd_t; > arch/arm/include/asm/pgtable-2level-types.h:typedef pmdval_t pgd_t[2]; > arch/arm/include/asm/pgtable-3level-types.h:typedef struct { pgdval_t pgd; } pgd_t; > arch/arm/include/asm/pgtable-3level-types.h:typedef pgdval_t pgd_t; > > I guess it might need different pgdp_get() variants depending applicable pgd_t > definition. Will continue looking into this further but meanwhile copied Russel > King in case he might be able to give some direction. That's Russel*L*, thanks. 32-bit arm uses, in some circumstances, an array because each level 1 page table entry is actually two descriptors. It needs to be this way because each level 2 table pointed to by each level 1 entry has 256 entries, meaning it only occupies 1024 bytes in a 4096 byte page. In order to cut down on the wastage, treat the level 1 page table as groups of two entries, which point to two consecutive 1024 byte tables in the level 2 page. The level 2 entry isn't suitable for the kernel's use cases (there are no bits to represent accessed/dirty and other important stuff that the Linux MM wants) so we maintain the hardware page tables and a separate set that Linux uses in the same page. Again, the software tables are consecutive, so from Linux's perspective, the level 2 page tables have 512 entries in them and occupy one full page. This is documented in arch/arm/include/asm/pgtable-2level.h However, what this means is that from the software perspective, the level 1 page table descriptors are an array of two entries, both of which need to be setup when creating a level 2 page table, but only the first one should ever be dereferenced when walking the tables, otherwise the code that walks the second level of page table entries will walk off the end of the software table into the actual hardware descriptors. I've no idea what the idea is behind introducing pgd_get() and what it's semantics are, so I can't comment further.
On 19/09/2024 10:11, Russell King (Oracle) wrote: > On Thu, Sep 19, 2024 at 01:25:08PM +0530, Anshuman Khandual wrote: >> arm (32) platform currently overrides pgdp_get() helper in the platform but >> defines that like the exact same version as the generic one, albeit with a >> typo which can be fixed with something like this. > > pgdp_get() was added to arm in eba2591d99d1 ("mm: Introduce > pudp/p4dp/pgdp_get() functions") with the typo you've spotted. It seems > it was added with no users, otherwise the error would have been spotted > earlier. I'm not a fan of adding dead code to the kernel for this > reason. > >> Regardless there is another problem here. On arm platform there are multiple >> pgd_t definitions available depending on various configs but some are arrays >> instead of a single data element, although platform pgdp_get() helper remains >> the same for all. >> >> arch/arm/include/asm/page-nommu.h:typedef unsigned long pgd_t[2]; >> arch/arm/include/asm/pgtable-2level-types.h:typedef struct { pmdval_t pgd[2]; } pgd_t; >> arch/arm/include/asm/pgtable-2level-types.h:typedef pmdval_t pgd_t[2]; >> arch/arm/include/asm/pgtable-3level-types.h:typedef struct { pgdval_t pgd; } pgd_t; >> arch/arm/include/asm/pgtable-3level-types.h:typedef pgdval_t pgd_t; >> >> I guess it might need different pgdp_get() variants depending applicable pgd_t >> definition. Will continue looking into this further but meanwhile copied Russel >> King in case he might be able to give some direction. > > That's Russel*L*, thanks. > > 32-bit arm uses, in some circumstances, an array because each level 1 > page table entry is actually two descriptors. It needs to be this way > because each level 2 table pointed to by each level 1 entry has 256 > entries, meaning it only occupies 1024 bytes in a 4096 byte page. > > In order to cut down on the wastage, treat the level 1 page table as > groups of two entries, which point to two consecutive 1024 byte tables > in the level 2 page. > > The level 2 entry isn't suitable for the kernel's use cases (there are > no bits to represent accessed/dirty and other important stuff that the > Linux MM wants) so we maintain the hardware page tables and a separate > set that Linux uses in the same page. Again, the software tables are > consecutive, so from Linux's perspective, the level 2 page tables > have 512 entries in them and occupy one full page. > > This is documented in arch/arm/include/asm/pgtable-2level.h > > However, what this means is that from the software perspective, the > level 1 page table descriptors are an array of two entries, both of > which need to be setup when creating a level 2 page table, but only > the first one should ever be dereferenced when walking the tables, > otherwise the code that walks the second level of page table entries > will walk off the end of the software table into the actual hardware > descriptors. > > I've no idea what the idea is behind introducing pgd_get() and what > it's semantics are, so I can't comment further. The helper is intended to read the value of the entry pointed to by the passed in pointer. And it shoiuld be read in a "single copy atomic" manner, meaning no tearing. Further, the PTL is expected to be held when calling the getter. If the HW can write to the entry such that its racing with the lock holder (i.e. HW update of access/dirty) then READ_ONCE() should be suitable for most architectures. If there is no possibility of racing (because HW doesn't write to the entry), then a simple dereference would be sufficient, I think (which is what the core code was already doing in most cases). There is additional benefit that the architecture can hook this function if it has exotic use cases (see contpte feature on arm64 as an example, which hooks ptep_get()). It sounds to me like the arm (32) implementation of pgdp_get() could just continue to do a direct dereference and this should be safe? I don't think it supports HW update of access/dirty? Thanks, Ryan
On Thu, Sep 19, 2024 at 05:48:58PM +0200, Ryan Roberts wrote: > > 32-bit arm uses, in some circumstances, an array because each level 1 > > page table entry is actually two descriptors. It needs to be this way > > because each level 2 table pointed to by each level 1 entry has 256 > > entries, meaning it only occupies 1024 bytes in a 4096 byte page. > > > > In order to cut down on the wastage, treat the level 1 page table as > > groups of two entries, which point to two consecutive 1024 byte tables > > in the level 2 page. > > > > The level 2 entry isn't suitable for the kernel's use cases (there are > > no bits to represent accessed/dirty and other important stuff that the > > Linux MM wants) so we maintain the hardware page tables and a separate > > set that Linux uses in the same page. Again, the software tables are > > consecutive, so from Linux's perspective, the level 2 page tables > > have 512 entries in them and occupy one full page. > > > > This is documented in arch/arm/include/asm/pgtable-2level.h > > > > However, what this means is that from the software perspective, the > > level 1 page table descriptors are an array of two entries, both of > > which need to be setup when creating a level 2 page table, but only > > the first one should ever be dereferenced when walking the tables, > > otherwise the code that walks the second level of page table entries > > will walk off the end of the software table into the actual hardware > > descriptors. > > > > I've no idea what the idea is behind introducing pgd_get() and what > > it's semantics are, so I can't comment further. > > The helper is intended to read the value of the entry pointed to by the passed > in pointer. And it shoiuld be read in a "single copy atomic" manner, meaning no > tearing. Further, the PTL is expected to be held when calling the getter. If the > HW can write to the entry such that its racing with the lock holder (i.e. HW > update of access/dirty) then READ_ONCE() should be suitable for most > architectures. If there is no possibility of racing (because HW doesn't write to > the entry), then a simple dereference would be sufficient, I think (which is > what the core code was already doing in most cases). The core code should be making no access to the PGD entries on 32-bit ARM since the PGD level does not exist. Writes are done at PMD level in arch code. Reads are done by core code at PMD level. It feels to me like pgd_get() just doesn't fit the model to which 32-bit ARM was designed to use decades ago, so I want full details about what pgd_get() is going to be used for and how it is going to be used, because I feel completely in the dark over this new development. I fear that someone hasn't understood the Linux page table model if they're wanting to access stuff at levels that effectively "aren't implemented" in the architecture specific kernel model of the page tables. Essentially, on 32-bit 2-level ARM, the PGD is merely indexed by the virtual address. As far as the kernel is concerned, each entry is 64-bit, and the generic kernel code has no business accessing that through the pgd pointer. The pgd pointer is passed through the PUD and PMD levels, where it is typecast down through the kernel layers to a pmd_t pointer, where it becomes a 32-bit quantity. This results in only the _first_ level 1 pointer being dereferenced by kernel code to a 32-bit pmd_t quantity. pmd_page_vaddr() converts this pmd_t quantity to a pte pointer (which points at the software level 2 page tables, not the hardware page tables.) So, as I'm now being told that the kernel wants to dereference the pgd level despite the model I describe above, alarm bells are ringing. I want full information please.
On 19/09/2024 18:06, Russell King (Oracle) wrote: > On Thu, Sep 19, 2024 at 05:48:58PM +0200, Ryan Roberts wrote: >>> 32-bit arm uses, in some circumstances, an array because each level 1 >>> page table entry is actually two descriptors. It needs to be this way >>> because each level 2 table pointed to by each level 1 entry has 256 >>> entries, meaning it only occupies 1024 bytes in a 4096 byte page. >>> >>> In order to cut down on the wastage, treat the level 1 page table as >>> groups of two entries, which point to two consecutive 1024 byte tables >>> in the level 2 page. >>> >>> The level 2 entry isn't suitable for the kernel's use cases (there are >>> no bits to represent accessed/dirty and other important stuff that the >>> Linux MM wants) so we maintain the hardware page tables and a separate >>> set that Linux uses in the same page. Again, the software tables are >>> consecutive, so from Linux's perspective, the level 2 page tables >>> have 512 entries in them and occupy one full page. >>> >>> This is documented in arch/arm/include/asm/pgtable-2level.h >>> >>> However, what this means is that from the software perspective, the >>> level 1 page table descriptors are an array of two entries, both of >>> which need to be setup when creating a level 2 page table, but only >>> the first one should ever be dereferenced when walking the tables, >>> otherwise the code that walks the second level of page table entries >>> will walk off the end of the software table into the actual hardware >>> descriptors. >>> >>> I've no idea what the idea is behind introducing pgd_get() and what >>> it's semantics are, so I can't comment further. >> >> The helper is intended to read the value of the entry pointed to by the passed >> in pointer. And it shoiuld be read in a "single copy atomic" manner, meaning no >> tearing. Further, the PTL is expected to be held when calling the getter. If the >> HW can write to the entry such that its racing with the lock holder (i.e. HW >> update of access/dirty) then READ_ONCE() should be suitable for most >> architectures. If there is no possibility of racing (because HW doesn't write to >> the entry), then a simple dereference would be sufficient, I think (which is >> what the core code was already doing in most cases). > > The core code should be making no access to the PGD entries on 32-bit > ARM since the PGD level does not exist. Writes are done at PMD level > in arch code. Reads are done by core code at PMD level. > > It feels to me like pgd_get() just doesn't fit the model to which 32-bit > ARM was designed to use decades ago, so I want full details about what > pgd_get() is going to be used for and how it is going to be used, > because I feel completely in the dark over this new development. I fear > that someone hasn't understood the Linux page table model if they're > wanting to access stuff at levels that effectively "aren't implemented" > in the architecture specific kernel model of the page tables. This change isn't as big and scary as I think you fear. The core-mm today dereferences pgd pointers (and p4d, pud, pmd pointers) directly in its code. See follow_pfnmap_start(), gup_fast_pgd_leaf(), and many other sites. These changes aim to abstract those dereferences into an inline function that the architecture can override and implement if it so wishes. The core-mm implements default versions of these helper functions which do READ_ONCE(), but does not currently use them consistently. From Anshuman's comments earlier in this thread, it looked to me like the arm pgd_t type is too big to read with READ_ONCE() - it can't be atomically read on that arch. So my proposal was to implement the override for arm to do exactly what the core-mm used to do, which is a pointer dereference. So that would result in exact same behaviour for the arm arch. > > Essentially, on 32-bit 2-level ARM, the PGD is merely indexed by the > virtual address. As far as the kernel is concerned, each entry is > 64-bit, and the generic kernel code has no business accessing that > through the pgd pointer. > > The pgd pointer is passed through the PUD and PMD levels, where it is > typecast down through the kernel layers to a pmd_t pointer, where it > becomes a 32-bit quantity. This results in only the _first_ level 1 > pointer being dereferenced by kernel code to a 32-bit pmd_t quantity. > pmd_page_vaddr() converts this pmd_t quantity to a pte pointer (which > points at the software level 2 page tables, not the hardware page > tables.) As an aside, my understanding of Linux's pgtable model differs from what you describe. As I understand it, Linux's logical page table model has 5 levels (pgd, p4d, pud, pmd, pte). If an arch doesn't support all 5 levels, then the middle levels can be folded away (p4d first, then pud, then pmd). But the core-mm still logically walks all 5 levels. So if the HW supports 2 levels, those levels are (pgd, pte). But you are suggesting that arm exposes pmd and pte, which is not what Linux expects? (Perhaps you call it the pmd in the arch, but that is being folded and accessed through the pgd helpers in core code, I believe? > > So, as I'm now being told that the kernel wants to dereference the > pgd level despite the model I describe above, alarm bells are ringing. > I want full information please. > This is not new; the kernel already dereferences the pgd pointers. Thanks, Ryan
On Thu, Sep 19, 2024 at 07:49:09PM +0200, Ryan Roberts wrote: > On 19/09/2024 18:06, Russell King (Oracle) wrote: > > On Thu, Sep 19, 2024 at 05:48:58PM +0200, Ryan Roberts wrote: > >>> 32-bit arm uses, in some circumstances, an array because each level 1 > >>> page table entry is actually two descriptors. It needs to be this way > >>> because each level 2 table pointed to by each level 1 entry has 256 > >>> entries, meaning it only occupies 1024 bytes in a 4096 byte page. > >>> > >>> In order to cut down on the wastage, treat the level 1 page table as > >>> groups of two entries, which point to two consecutive 1024 byte tables > >>> in the level 2 page. > >>> > >>> The level 2 entry isn't suitable for the kernel's use cases (there are > >>> no bits to represent accessed/dirty and other important stuff that the > >>> Linux MM wants) so we maintain the hardware page tables and a separate > >>> set that Linux uses in the same page. Again, the software tables are > >>> consecutive, so from Linux's perspective, the level 2 page tables > >>> have 512 entries in them and occupy one full page. > >>> > >>> This is documented in arch/arm/include/asm/pgtable-2level.h > >>> > >>> However, what this means is that from the software perspective, the > >>> level 1 page table descriptors are an array of two entries, both of > >>> which need to be setup when creating a level 2 page table, but only > >>> the first one should ever be dereferenced when walking the tables, > >>> otherwise the code that walks the second level of page table entries > >>> will walk off the end of the software table into the actual hardware > >>> descriptors. > >>> > >>> I've no idea what the idea is behind introducing pgd_get() and what > >>> it's semantics are, so I can't comment further. > >> > >> The helper is intended to read the value of the entry pointed to by the passed > >> in pointer. And it shoiuld be read in a "single copy atomic" manner, meaning no > >> tearing. Further, the PTL is expected to be held when calling the getter. If the > >> HW can write to the entry such that its racing with the lock holder (i.e. HW > >> update of access/dirty) then READ_ONCE() should be suitable for most > >> architectures. If there is no possibility of racing (because HW doesn't write to > >> the entry), then a simple dereference would be sufficient, I think (which is > >> what the core code was already doing in most cases). > > > > The core code should be making no access to the PGD entries on 32-bit > > ARM since the PGD level does not exist. Writes are done at PMD level > > in arch code. Reads are done by core code at PMD level. > > > > It feels to me like pgd_get() just doesn't fit the model to which 32-bit > > ARM was designed to use decades ago, so I want full details about what > > pgd_get() is going to be used for and how it is going to be used, > > because I feel completely in the dark over this new development. I fear > > that someone hasn't understood the Linux page table model if they're > > wanting to access stuff at levels that effectively "aren't implemented" > > in the architecture specific kernel model of the page tables. > > This change isn't as big and scary as I think you fear. The situation is as I state above. Core code must _not_ dereference pgd pointers on 32-bit ARM. > The core-mm today > dereferences pgd pointers (and p4d, pud, pmd pointers) directly in its code. See > follow_pfnmap_start(), Doesn't seem to exist at least not in 6.11. > gup_fast_pgd_leaf(), and many other sites. Only built when CONFIG_HAVE_GUP_FAST is set, which 32-bit ARM doesn't set because its meaningless there, except when LPAE is in use (which is basically the situation I'm discussing.) > These changes > aim to abstract those dereferences into an inline function that the architecture > can override and implement if it so wishes. > > The core-mm implements default versions of these helper functions which do > READ_ONCE(), but does not currently use them consistently. > > From Anshuman's comments earlier in this thread, it looked to me like the arm > pgd_t type is too big to read with READ_ONCE() - it can't be atomically read on > that arch. So my proposal was to implement the override for arm to do exactly > what the core-mm used to do, which is a pointer dereference. So that would > result in exact same behaviour for the arm arch. Let me say this again: core code must NOT dereference pgds on 32-bit non-LPAE ARM. They are meaningless to core code. A pgd_t does not reference a single entry in hardware. It references two entries. > > Essentially, on 32-bit 2-level ARM, the PGD is merely indexed by the > > virtual address. As far as the kernel is concerned, each entry is > > 64-bit, and the generic kernel code has no business accessing that > > through the pgd pointer. > > > > The pgd pointer is passed through the PUD and PMD levels, where it is > > typecast down through the kernel layers to a pmd_t pointer, where it > > becomes a 32-bit quantity. This results in only the _first_ level 1 > > pointer being dereferenced by kernel code to a 32-bit pmd_t quantity. > > pmd_page_vaddr() converts this pmd_t quantity to a pte pointer (which > > points at the software level 2 page tables, not the hardware page > > tables.) > > As an aside, my understanding of Linux's pgtable model differs from what you > describe. As I understand it, Linux's logical page table model has 5 levels > (pgd, p4d, pud, pmd, pte). If an arch doesn't support all 5 levels, then the > middle levels can be folded away (p4d first, then pud, then pmd). But the > core-mm still logically walks all 5 levels. So if the HW supports 2 levels, > those levels are (pgd, pte). But you are suggesting that arm exposes pmd and > pte, which is not what Linux expects? (Perhaps you call it the pmd in the arch, > but that is being folded and accessed through the pgd helpers in core code, I > believe? What ARM does dates from before the Linux MM invented the current "folding" method when we had three page table levels - pgd, pmd and pte. The current folding techniques were invented well after 32-bit ARM was implemented, which was using the original idea of how to fold the page tables. The new folding came up with a totally different way of doing it, and I looked into converting 32-bit ARM over to it, but it wasn't possible to do so with the need for two level-1 entries to be managed for each level-2 page table. > > So, as I'm now being told that the kernel wants to dereference the > > pgd level despite the model I describe above, alarm bells are ringing. > > I want full information please. > > > > This is not new; the kernel already dereferences the pgd pointers. Consider that 32-bit ARM has been this way for decades (Linux was ported to 32-bit ARM by me back in the 1990s - so it's about 30 years old.) Compare that to what you're stating is "not new"... I beg to differ with your opinion on what is new and what isn't. It's all about the relative time. This is how the page tables are walked: static inline pgd_t *pgd_offset_pgd(pgd_t *pgd, unsigned long address) { return (pgd + pgd_index(address)); } #define pgd_offset(mm, address) pgd_offset_pgd((mm)->pgd, (address)) This returns a pointer to the pgd. This is then used with p4d_offset() when walking the next level, and this is defined on 32-bit ARM from include/asm-generic/pgtable-nop4d.h: static inline p4d_t *p4d_offset(pgd_t *pgd, unsigned long address) { return (p4d_t *)pgd; } Then from include/asm-generic/pgtable-nopud.h: static inline pud_t *pud_offset(p4d_t *p4d, unsigned long address) { return (pud_t *)p4d; } Then from arch/arm/include/asm/pgtable-2level.h: static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr) { return (pmd_t *)pud; } All of the above casts result in the pgd_t pointer being cast down to a pmd_t pointer. Now, looking at stuff in mm/memory.c such as unmap_page_range(). pgd = pgd_offset(vma->vm_mm, addr); This gets the pgd pointer into the level 1 page tables associated with addr, and passes it down to zap_p4d_range(). That passes it to p4d_offset() without dereferencing it, which on 32-bit ARM, merely casts the pgd_t pointer to a p4d_t pointer. Since a p4d_t is defined to be a struct of a pgd_t, this also points at an array of two 32-bit quantities. This pointer is passed down to zap_pud_range(). zap_pud_range() passes this pointer to pud_offset(), again without dereferencing it, and we end up with a pud_t pointer. Since pud_t is defined to be a struct of p4d_t, this also points to an array of two 32-bit quantities. We then have: if (pud_trans_huge(*pud) || pud_devmap(*pud)) { These is an implicit memory copy/access between the memory pointed to by pud, and their destination (which might be a register). However, these are optimised away because 32-bit ARM doesn't set HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD nor ARCH_HAS_PTE_DEVMAP (as neither inline function make use of their argument.) NOTE: If making these use READ_ONCE results in an access that can not be optimised away, that is a bug that needs to be addressed. zap_pud_range() then passes the pud pointer to zap_pmd_range(). zap_pmd_range() passes this pointer to pud_offset() with no further dereferences, and this gets cast to a pmd_t pointer, which is a pointer to the first 32-bit quantity pointed to by the pgd_t pointer. All the dereferences from this point on are 32-bit which can be done as single-copy atomic accesses. This will be the first real access to the level-1 page tables in this code path as the code stands today, and from this point on, accesses to the page tables are as the architecture intends them to be. Now, realise that for all of the accesses above that have all been optimised away, none of that code even existed when 32-bit ARM was using this method. The addition of these features not intefering with the way 32-bit non-LPAE ARM works relies on all of those accesses being optimised away, and they need to continue to be so going forward. Maybe that means that this new (and I mean new in relative terms compared to the age of the 32-bit ARM code) pgdp_get() accessor needs to be a non-dereferencing operation, so something like: #define pgdp_get(pgdp) ((pgd_t){ }) in arch/arm/include/asm/pgtable-2level.h (note the corrected spelling of pgdp), and the existing pgdp_get() moved to arch/arm/include/asm/pgtable-3level.h. This isn't tested. However, let me say this again... without knowing exactly how and where pgdp_get() is intended to be used, I'm clutching at straws here. Even looking at Linus' tree, there's very little in evidence there to suggest how pgdp_get() is intended to be used. For example, there's no references to it in mm/. Please realise that I have _no_ _clue_ what "[PATCH V2 7/7] mm: Use pgdp_get() for accessing PGD entries" is proposing. I wasn't on its Cc list. I haven't seen the patch. The first I knew anything about this was with the email that Anshuman Khandual sent in response to the kernel build bot's build error. I'm afraid that the kernel build bot's build error means that this patch: commit eba2591d99d1f14a04c8a8a845ab0795b93f5646 Author: Alexandre Ghiti <alexghiti@rivosinc.com> Date: Wed Dec 13 21:29:59 2023 +0100 mm: Introduce pudp/p4dp/pgdp_get() functions is actually broken. I'm sorry that I didn't review that, but how the series looked when it landed in my mailbox, it looked like it was specific to RISC-V and of no interest to me, so I didn't bother reading it (I get _lots_ of email, I can't read everything.) This is how it looks like in my mailbox (and note that they're marked as new to this day): 3218 N T Dec 13 Alexandre Ghiti ( 0) [PATCH v2 0/4] riscv: Use READ_ONCE()/WRI 3219 N T Dec 13 Alexandre Ghiti ( 0) ├─>[PATCH v2 1/4] riscv: Use WRITE_ONCE() 3220 N T Dec 13 Alexandre Ghiti ( 0) ├─>[PATCH v2 2/4] mm: Introduce pudp/p4dp 3221 N T Dec 13 Alexandre Ghiti ( 0) ├─>[PATCH v2 3/4] riscv: mm: Only compile 3222 N T Dec 13 Alexandre Ghiti ( 0) └─>[PATCH v2 4/4] riscv: Use accessors to 3223 N C Dec 14 Anup Patel ( 0) └─> Sorry, but I'm not even going to look at something like that when it looks like it's for RISC-V and nothing else. One final point... because I'm sure someone's going to say "but you were in the To: header". I've long since given up using "am I in the Cc/To header" to carry any useful or meaningful information to indicate whether it's something I should read. I'm afraid that the kernel community has long since taught me that is of no value what so ever, so I merely go by "does this look of any interest". If not, I don't bother even _opening_ the email.
diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c index fcaceac60659..6aeccbd440e7 100644 --- a/drivers/misc/sgi-gru/grufault.c +++ b/drivers/misc/sgi-gru/grufault.c @@ -212,7 +212,7 @@ static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr, pte_t pte; pgdp = pgd_offset(vma->vm_mm, vaddr); - if (unlikely(pgd_none(*pgdp))) + if (unlikely(pgd_none(pgdp_get(pgdp)))) goto err; p4dp = p4d_offset(pgdp, vaddr); diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 4044e15cdfd9..6d33c7a9eb01 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -304,7 +304,7 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, assert_fault_locked(vmf); pgd = pgd_offset(mm, address); - if (!pgd_present(*pgd)) + if (!pgd_present(pgdp_get(pgd))) goto out; p4d = p4d_offset(pgd, address); if (!p4d_present(p4dp_get(p4d))) diff --git a/include/linux/mm.h b/include/linux/mm.h index 1bb1599b5779..1978a4b1fcf5 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2819,7 +2819,7 @@ int __pte_alloc_kernel(pmd_t *pmd); static inline p4d_t *p4d_alloc(struct mm_struct *mm, pgd_t *pgd, unsigned long address) { - return (unlikely(pgd_none(*pgd)) && __p4d_alloc(mm, pgd, address)) ? + return (unlikely(pgd_none(pgdp_get(pgd))) && __p4d_alloc(mm, pgd, address)) ? NULL : p4d_offset(pgd, address); } diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h index 689cd5a32157..6d12ae7e3982 100644 --- a/include/linux/pgtable.h +++ b/include/linux/pgtable.h @@ -1088,7 +1088,8 @@ static inline int pgd_same(pgd_t pgd_a, pgd_t pgd_b) #define set_pgd_safe(pgdp, pgd) \ ({ \ - WARN_ON_ONCE(pgd_present(*pgdp) && !pgd_same(*pgdp, pgd)); \ + pgd_t __old = pgdp_get(pgdp); \ + WARN_ON_ONCE(pgd_present(__old) && !pgd_same(__old, pgd)); \ set_pgd(pgdp, pgd); \ }) @@ -1241,9 +1242,11 @@ void pmd_clear_bad(pmd_t *); static inline int pgd_none_or_clear_bad(pgd_t *pgd) { - if (pgd_none(*pgd)) + pgd_t old_pgd = pgdp_get(pgd); + + if (pgd_none(old_pgd)) return 1; - if (unlikely(pgd_bad(*pgd))) { + if (unlikely(pgd_bad(old_pgd))) { pgd_clear_bad(pgd); return 1; } diff --git a/kernel/events/core.c b/kernel/events/core.c index 4e56a276ed25..1e3142211cce 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -7603,7 +7603,7 @@ static u64 perf_get_pgtable_size(struct mm_struct *mm, unsigned long addr) pte_t *ptep, pte; pgdp = pgd_offset(mm, addr); - pgd = READ_ONCE(*pgdp); + pgd = pgdp_get(pgdp); if (pgd_none(pgd)) return 0; diff --git a/mm/gup.c b/mm/gup.c index 3a97d0263052..3aff3555ba19 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1051,7 +1051,7 @@ static struct page *follow_page_mask(struct vm_area_struct *vma, unsigned long address, unsigned int flags, struct follow_page_context *ctx) { - pgd_t *pgd; + pgd_t *pgd, old_pgd; struct mm_struct *mm = vma->vm_mm; struct page *page; @@ -1060,7 +1060,8 @@ static struct page *follow_page_mask(struct vm_area_struct *vma, ctx->page_mask = 0; pgd = pgd_offset(mm, address); - if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd))) + old_pgd = pgdp_get(pgd); + if (pgd_none(old_pgd) || unlikely(pgd_bad(old_pgd))) page = no_page_table(vma, flags, address); else page = follow_p4d_mask(vma, address, pgd, flags, ctx); @@ -1111,7 +1112,7 @@ static int get_gate_page(struct mm_struct *mm, unsigned long address, pgd = pgd_offset_k(address); else pgd = pgd_offset_gate(mm, address); - if (pgd_none(*pgd)) + if (pgd_none(pgdp_get(pgd))) return -EFAULT; p4d = p4d_offset(pgd, address); if (p4d_none(p4dp_get(p4d))) @@ -3158,7 +3159,7 @@ static int gup_fast_pgd_leaf(pgd_t orig, pgd_t *pgdp, unsigned long addr, if (!folio) return 0; - if (unlikely(pgd_val(orig) != pgd_val(*pgdp))) { + if (unlikely(pgd_val(orig) != pgd_val(pgdp_get(pgdp)))) { gup_put_folio(folio, refs, flags); return 0; } @@ -3267,7 +3268,7 @@ static void gup_fast_pgd_range(unsigned long addr, unsigned long end, pgdp = pgd_offset(current->mm, addr); do { - pgd_t pgd = READ_ONCE(*pgdp); + pgd_t pgd = pgdp_get(pgdp); next = pgd_addr_end(addr, end); if (pgd_none(pgd)) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 4fdb91c8cc2b..294d74b03d83 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -7451,7 +7451,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm, pmd_t *pmd; pgd = pgd_offset(mm, addr); - if (!pgd_present(*pgd)) + if (!pgd_present(pgdp_get(pgd))) return NULL; p4d = p4d_offset(pgd, addr); if (!p4d_present(p4dp_get(p4d))) diff --git a/mm/kasan/init.c b/mm/kasan/init.c index 02af738fee5e..c2b307716551 100644 --- a/mm/kasan/init.c +++ b/mm/kasan/init.c @@ -271,7 +271,7 @@ int __ref kasan_populate_early_shadow(const void *shadow_start, continue; } - if (pgd_none(*pgd)) { + if (pgd_none(pgdp_get(pgd))) { p4d_t *p; if (slab_is_available()) { @@ -345,7 +345,7 @@ static void kasan_free_p4d(p4d_t *p4d_start, pgd_t *pgd) return; } - p4d_free(&init_mm, (p4d_t *)page_to_virt(pgd_page(*pgd))); + p4d_free(&init_mm, (p4d_t *)page_to_virt(pgd_page(pgdp_get(pgd)))); pgd_clear(pgd); } @@ -468,10 +468,10 @@ void kasan_remove_zero_shadow(void *start, unsigned long size) next = pgd_addr_end(addr, end); pgd = pgd_offset_k(addr); - if (!pgd_present(*pgd)) + if (!pgd_present(pgdp_get(pgd))) continue; - if (kasan_p4d_table(*pgd)) { + if (kasan_p4d_table(pgdp_get(pgd))) { if (IS_ALIGNED(addr, PGDIR_SIZE) && IS_ALIGNED(next, PGDIR_SIZE)) { pgd_clear(pgd); diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c index 52150cc5ae5f..7f3c46237816 100644 --- a/mm/kasan/shadow.c +++ b/mm/kasan/shadow.c @@ -191,7 +191,7 @@ static bool shadow_mapped(unsigned long addr) pmd_t *pmd; pte_t *pte; - if (pgd_none(*pgd)) + if (pgd_none(pgdp_get(pgd))) return false; p4d = p4d_offset(pgd, addr); if (p4d_none(p4dp_get(p4d))) diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 3d900cc039b3..c9397eab52bd 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -411,7 +411,7 @@ static unsigned long dev_pagemap_mapping_shift(struct vm_area_struct *vma, VM_BUG_ON_VMA(address == -EFAULT, vma); pgd = pgd_offset(vma->vm_mm, address); - if (!pgd_present(*pgd)) + if (!pgd_present(pgdp_get(pgd))) return 0; p4d = p4d_offset(pgd, address); if (!p4d_present(p4dp_get(p4d))) diff --git a/mm/memory.c b/mm/memory.c index 5056f39f2c3b..b4845a84ceb5 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -2942,7 +2942,7 @@ static int __apply_to_page_range(struct mm_struct *mm, unsigned long addr, unsigned long size, pte_fn_t fn, void *data, bool create) { - pgd_t *pgd; + pgd_t *pgd, old_pgd; unsigned long start = addr, next; unsigned long end = addr + size; pgtbl_mod_mask mask = 0; @@ -2954,11 +2954,12 @@ static int __apply_to_page_range(struct mm_struct *mm, unsigned long addr, pgd = pgd_offset(mm, addr); do { next = pgd_addr_end(addr, end); - if (pgd_none(*pgd) && !create) + old_pgd = pgdp_get(pgd); + if (pgd_none(old_pgd) && !create) continue; - if (WARN_ON_ONCE(pgd_leaf(*pgd))) + if (WARN_ON_ONCE(pgd_leaf(old_pgd))) return -EINVAL; - if (!pgd_none(*pgd) && WARN_ON_ONCE(pgd_bad(*pgd))) { + if (!pgd_none(old_pgd) && WARN_ON_ONCE(pgd_bad(old_pgd))) { if (!create) continue; pgd_clear_bad(pgd); @@ -6053,7 +6054,7 @@ int __p4d_alloc(struct mm_struct *mm, pgd_t *pgd, unsigned long address) return -ENOMEM; spin_lock(&mm->page_table_lock); - if (pgd_present(*pgd)) { /* Another has populated it */ + if (pgd_present(pgdp_get(pgd))) { /* Another has populated it */ p4d_free(mm, new); } else { smp_wmb(); /* See comment in pmd_install() */ @@ -6143,7 +6144,7 @@ int follow_pte(struct vm_area_struct *vma, unsigned long address, pte_t **ptepp, spinlock_t **ptlp) { struct mm_struct *mm = vma->vm_mm; - pgd_t *pgd; + pgd_t *pgd, old_pgd; p4d_t *p4d, old_p4d; pud_t *pud; pmd_t *pmd; @@ -6157,7 +6158,8 @@ int follow_pte(struct vm_area_struct *vma, unsigned long address, goto out; pgd = pgd_offset(mm, address); - if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd))) + old_pgd = pgdp_get(pgd); + if (pgd_none(old_pgd) || unlikely(pgd_bad(old_pgd))) goto out; p4d = p4d_offset(pgd, address); diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c index a33f92db2666..fb8b610f7378 100644 --- a/mm/page_vma_mapped.c +++ b/mm/page_vma_mapped.c @@ -212,7 +212,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) restart: do { pgd = pgd_offset(mm, pvmw->address); - if (!pgd_present(*pgd)) { + if (!pgd_present(pgdp_get(pgd))) { step_forward(pvmw, PGDIR_SIZE); continue; } diff --git a/mm/percpu.c b/mm/percpu.c index 58660e8eb892..70e68ab002e9 100644 --- a/mm/percpu.c +++ b/mm/percpu.c @@ -3184,7 +3184,7 @@ void __init __weak pcpu_populate_pte(unsigned long addr) pud_t *pud; pmd_t *pmd; - if (pgd_none(*pgd)) { + if (pgd_none(pgdp_get(pgd))) { p4d = memblock_alloc(P4D_TABLE_SIZE, P4D_TABLE_SIZE); if (!p4d) goto err_alloc; diff --git a/mm/pgalloc-track.h b/mm/pgalloc-track.h index 3db8ccbcb141..644f632c7cba 100644 --- a/mm/pgalloc-track.h +++ b/mm/pgalloc-track.h @@ -7,7 +7,7 @@ static inline p4d_t *p4d_alloc_track(struct mm_struct *mm, pgd_t *pgd, unsigned long address, pgtbl_mod_mask *mod_mask) { - if (unlikely(pgd_none(*pgd))) { + if (unlikely(pgd_none(pgdp_get(pgd)))) { if (__p4d_alloc(mm, pgd, address)) return NULL; *mod_mask |= PGTBL_PGD_MODIFIED; diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c index f5ab52beb536..16c1ed5b3d0b 100644 --- a/mm/pgtable-generic.c +++ b/mm/pgtable-generic.c @@ -24,7 +24,7 @@ void pgd_clear_bad(pgd_t *pgd) { - pgd_ERROR(*pgd); + pgd_ERROR(pgdp_get(pgd)); pgd_clear(pgd); } diff --git a/mm/rmap.c b/mm/rmap.c index a0ff325467eb..5f4c52f34192 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -809,7 +809,7 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address) pmd_t *pmd = NULL; pgd = pgd_offset(mm, address); - if (!pgd_present(*pgd)) + if (!pgd_present(pgdp_get(pgd))) goto out; p4d = p4d_offset(pgd, address); diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c index 2bd1c95f107a..ffc78329a130 100644 --- a/mm/sparse-vmemmap.c +++ b/mm/sparse-vmemmap.c @@ -233,7 +233,7 @@ p4d_t * __meminit vmemmap_p4d_populate(pgd_t *pgd, unsigned long addr, int node) pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node) { pgd_t *pgd = pgd_offset_k(addr); - if (pgd_none(*pgd)) { + if (pgd_none(pgdp_get(pgd))) { void *p = vmemmap_alloc_block_zero(PAGE_SIZE, node); if (!p) return NULL; diff --git a/mm/vmalloc.c b/mm/vmalloc.c index f27ecac7bd6e..a40323a8c6ab 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -450,7 +450,7 @@ void __vunmap_range_noflush(unsigned long start, unsigned long end) pgd = pgd_offset_k(addr); do { next = pgd_addr_end(addr, end); - if (pgd_bad(*pgd)) + if (pgd_bad(pgdp_get(pgd))) mask |= PGTBL_PGD_MODIFIED; if (pgd_none_or_clear_bad(pgd)) continue; @@ -582,7 +582,7 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, pgd = pgd_offset_k(addr); do { next = pgd_addr_end(addr, end); - if (pgd_bad(*pgd)) + if (pgd_bad(pgdp_get(pgd))) mask |= PGTBL_PGD_MODIFIED; err = vmap_pages_p4d_range(pgd, addr, next, prot, pages, &nr, &mask); if (err) @@ -740,7 +740,7 @@ struct page *vmalloc_to_page(const void *vmalloc_addr) { unsigned long addr = (unsigned long) vmalloc_addr; struct page *page = NULL; - pgd_t *pgd = pgd_offset_k(addr); + pgd_t *pgd = pgd_offset_k(addr), old_pgd; p4d_t *p4d, old_p4d; pud_t *pud, old_pud; pmd_t *pmd, old_pmd; @@ -752,11 +752,12 @@ struct page *vmalloc_to_page(const void *vmalloc_addr) */ VIRTUAL_BUG_ON(!is_vmalloc_or_module_addr(vmalloc_addr)); - if (pgd_none(*pgd)) + old_pgd = pgdp_get(pgd); + if (pgd_none(old_pgd)) return NULL; - if (WARN_ON_ONCE(pgd_leaf(*pgd))) + if (WARN_ON_ONCE(pgd_leaf(old_pgd))) return NULL; /* XXX: no allowance for huge pgd */ - if (WARN_ON_ONCE(pgd_bad(*pgd))) + if (WARN_ON_ONCE(pgd_bad(old_pgd))) return NULL; p4d = p4d_offset(pgd, addr);
Convert PGD accesses via pgdp_get() helper that defaults as READ_ONCE() but also provides the platform an opportunity to override when required. This stores read page table entry value in a local variable which can be used in multiple instances there after. This helps in avoiding multiple memory load operations as well possible race conditions. Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Muchun Song <muchun.song@linux.dev> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> Cc: Miaohe Lin <linmiaohe@huawei.com> Cc: Dennis Zhou <dennis@kernel.org> Cc: Tejun Heo <tj@kernel.org> cc: Christoph Lameter <cl@linux.com> Cc: Uladzislau Rezki <urezki@gmail.com> Cc: Christoph Hellwig <hch@infradead.org> Cc: linux-kernel@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Cc: linux-perf-users@vger.kernel.org Cc: kasan-dev@googlegroups.com Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- drivers/misc/sgi-gru/grufault.c | 2 +- fs/userfaultfd.c | 2 +- include/linux/mm.h | 2 +- include/linux/pgtable.h | 9 ++++++--- kernel/events/core.c | 2 +- mm/gup.c | 11 ++++++----- mm/hugetlb.c | 2 +- mm/kasan/init.c | 8 ++++---- mm/kasan/shadow.c | 2 +- mm/memory-failure.c | 2 +- mm/memory.c | 16 +++++++++------- mm/page_vma_mapped.c | 2 +- mm/percpu.c | 2 +- mm/pgalloc-track.h | 2 +- mm/pgtable-generic.c | 2 +- mm/rmap.c | 2 +- mm/sparse-vmemmap.c | 2 +- mm/vmalloc.c | 13 +++++++------ 18 files changed, 45 insertions(+), 38 deletions(-)