| Message ID | 20230807082305.198784-2-dylan@andestech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Enhanced TLB flushing for vmap/vmalloc() | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 471aba2e4760 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | fail | Errors and warnings before: 2808 this patch: 2809 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | fail | Errors and warnings before: 15876 this patch: 15879 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 12 this patch: 12 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | warning | WARNING: adding a line without newline at end of file |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Hi Dylan, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.5-rc5 next-20230807] [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/Dylan-Jhong/riscv-Implement-arch_sync_kernel_mappings-for-preventive-TLB-flush/20230807-162922 base: linus/master patch link: https://lore.kernel.org/r/20230807082305.198784-2-dylan%40andestech.com patch subject: [PATCH 1/1] riscv: Implement arch_sync_kernel_mappings() for "preventive" TLB flush config: riscv-allyesconfig (https://download.01.org/0day-ci/archive/20230807/202308071710.irjERWVF-lkp@intel.com/config) compiler: riscv64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230807/202308071710.irjERWVF-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/202308071710.irjERWVF-lkp@intel.com/ All warnings (new ones prefixed by >>): >> arch/riscv/mm/tlbflush.c:159:6: warning: no previous prototype for 'arch_sync_kernel_mappings' [-Wmissing-prototypes] 159 | void arch_sync_kernel_mappings(unsigned long start, unsigned long end) | ^~~~~~~~~~~~~~~~~~~~~~~~~ vim +/arch_sync_kernel_mappings +159 arch/riscv/mm/tlbflush.c 152 153 /* 154 * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, 155 * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains 156 * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel 157 * page table mappings created via vmap/vmalloc() are updated before switching MM. 158 */ > 159 void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
Hi Dylan, kernel test robot noticed the following build errors: [auto build test ERROR on linus/master] [also build test ERROR on v6.5-rc5 next-20230807] [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/Dylan-Jhong/riscv-Implement-arch_sync_kernel_mappings-for-preventive-TLB-flush/20230807-162922 base: linus/master patch link: https://lore.kernel.org/r/20230807082305.198784-2-dylan%40andestech.com patch subject: [PATCH 1/1] riscv: Implement arch_sync_kernel_mappings() for "preventive" TLB flush config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20230807/202308072050.0T0FlSpT-lkp@intel.com/config) compiler: riscv64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230807/202308072050.0T0FlSpT-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/202308072050.0T0FlSpT-lkp@intel.com/ All errors (new ones prefixed by >>): riscv64-linux-ld: mm/memory.o: in function `.L1539': >> memory.c:(.text+0x3b5c): undefined reference to `arch_sync_kernel_mappings' riscv64-linux-ld: mm/vmalloc.o: in function `.L301': >> vmalloc.c:(.text+0xd24): undefined reference to `arch_sync_kernel_mappings' riscv64-linux-ld: mm/vmalloc.o: in function `vb_alloc.constprop.0': vmalloc.c:(.text+0x2c4e): undefined reference to `arch_sync_kernel_mappings' riscv64-linux-ld: mm/vmalloc.o: in function `.L0 ': vmalloc.c:(.text+0x2f2c): undefined reference to `arch_sync_kernel_mappings'
Hi Dylan, On 07/08/2023 10:23, Dylan Jhong wrote: > Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, > it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains > the correct kernel mapping. > > The patch implements TLB flushing in arch_sync_kernel_mappings(), ensuring that kernel > page table mappings created via vmap/vmalloc() are updated before switching MM. > > Signed-off-by: Dylan Jhong <dylan@andestech.com> > --- > arch/riscv/include/asm/page.h | 2 ++ > arch/riscv/mm/tlbflush.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h > index b55ba20903ec..6c86ab69687e 100644 > --- a/arch/riscv/include/asm/page.h > +++ b/arch/riscv/include/asm/page.h > @@ -21,6 +21,8 @@ > #define HPAGE_MASK (~(HPAGE_SIZE - 1)) > #define HUGETLB_PAGE_ORDER (HPAGE_SHIFT - PAGE_SHIFT) > > +#define ARCH_PAGE_TABLE_SYNC_MASK PGTBL_PTE_MODIFIED > + > /* > * PAGE_OFFSET -- the first address of the first page of memory. > * When not using MMU this corresponds to the first free page in > diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c > index 77be59aadc73..d63364948c85 100644 > --- a/arch/riscv/mm/tlbflush.c > +++ b/arch/riscv/mm/tlbflush.c > @@ -149,3 +149,15 @@ void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start, > __flush_tlb_range(vma->vm_mm, start, end - start, PMD_SIZE); > } > #endif > + > +/* > + * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, > + * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains > + * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel > + * page table mappings created via vmap/vmalloc() are updated before switching MM. > + */ > +void arch_sync_kernel_mappings(unsigned long start, unsigned long end) > +{ > + if (start < VMALLOC_END && end > VMALLOC_START) This test is too restrictive, it should catch the range [MODULES_VADDR; MODULES_END[ too, sorry I did not notice that at first. > + flush_tlb_all(); > +} > \ No newline at end of file I have to admit that I *think* both your patch and mine are wrong: one of the problem that led to the removal of vmalloc_fault() is the possibility for tracing functions to actually allocate vmalloc regions in the vmalloc page fault path, which could give rise to nested exceptions (see https://lore.kernel.org/lkml/20200508144043.13893-1-joro@8bytes.org/). Here, everytime we allocate a vmalloc region, we send an IPI. If a vmalloc allocation happens in this path (if it is traced for example), it will give rise to an IPI...and so on. So I came to the conclusion that the only way to actually fix this issue is by resolving the vmalloc faults very early in the page fault path (by emitting a sfence.vma on uarch that cache invalid entries), before the kernel stack is even accessed. That's the best solution since it would completely remove all the preventive sfence.vma in flush_cache_vmap()/arch_sync_kernel_mappings(), we would rely on faulting which I assume should not happen a lot (?). I'm implementing this solution, but I'm pretty sure it won't be ready for 6.5. In the meantime, we need either your patch or mine to fix your issue...
On Tue, Aug 08, 2023 at 12:16:50PM +0200, Alexandre Ghiti wrote: > Hi Dylan, > > On 07/08/2023 10:23, Dylan Jhong wrote: > > Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, > > it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains > > the correct kernel mapping. > > > > The patch implements TLB flushing in arch_sync_kernel_mappings(), ensuring that kernel > > page table mappings created via vmap/vmalloc() are updated before switching MM. > > > > Signed-off-by: Dylan Jhong <dylan@andestech.com> > > --- > > arch/riscv/include/asm/page.h | 2 ++ > > arch/riscv/mm/tlbflush.c | 12 ++++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h > > index b55ba20903ec..6c86ab69687e 100644 > > --- a/arch/riscv/include/asm/page.h > > +++ b/arch/riscv/include/asm/page.h > > @@ -21,6 +21,8 @@ > > #define HPAGE_MASK (~(HPAGE_SIZE - 1)) > > #define HUGETLB_PAGE_ORDER (HPAGE_SHIFT - PAGE_SHIFT) > > +#define ARCH_PAGE_TABLE_SYNC_MASK PGTBL_PTE_MODIFIED > > + > > /* > > * PAGE_OFFSET -- the first address of the first page of memory. > > * When not using MMU this corresponds to the first free page in > > diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c > > index 77be59aadc73..d63364948c85 100644 > > --- a/arch/riscv/mm/tlbflush.c > > +++ b/arch/riscv/mm/tlbflush.c > > @@ -149,3 +149,15 @@ void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start, > > __flush_tlb_range(vma->vm_mm, start, end - start, PMD_SIZE); > > } > > #endif > > + > > +/* > > + * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, > > + * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains > > + * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel > > + * page table mappings created via vmap/vmalloc() are updated before switching MM. > > + */ > > +void arch_sync_kernel_mappings(unsigned long start, unsigned long end) > > +{ > > + if (start < VMALLOC_END && end > VMALLOC_START) > > > This test is too restrictive, it should catch the range [MODULES_VADDR; > MODULES_END[ too, sorry I did not notice that at first. > > > > + flush_tlb_all(); > > +} > > \ No newline at end of file > > > I have to admit that I *think* both your patch and mine are wrong: one of > the problem that led to the removal of vmalloc_fault() is the possibility > for tracing functions to actually allocate vmalloc regions in the vmalloc > page fault path, which could give rise to nested exceptions (see > https://lore.kernel.org/lkml/20200508144043.13893-1-joro@8bytes.org/). > > Here, everytime we allocate a vmalloc region, we send an IPI. If a vmalloc > allocation happens in this path (if it is traced for example), it will give > rise to an IPI...and so on. > > So I came to the conclusion that the only way to actually fix this issue is > by resolving the vmalloc faults very early in the page fault path (by > emitting a sfence.vma on uarch that cache invalid entries), before the > kernel stack is even accessed. That's the best solution since it would > completely remove all the preventive sfence.vma in > flush_cache_vmap()/arch_sync_kernel_mappings(), we would rely on faulting > which I assume should not happen a lot (?). > Hi Alex, Agree. If we could introduce a "new vmalloc_fault()" function before accessing the kernel stack, which would trigger an SFENCE.VMA instruction, then each time we call vmalloc() or vmap() to create new kernel mappings, we wouldn't need to execute flush_cache_vmap() or arch_sync_kernel_mappings() to update the TLB. This should be able to balance both performance and correctness. > I'm implementing this solution, but I'm pretty sure it won't be ready for > 6.5. In the meantime, we need either your patch or mine to fix your issue... > If there are no others reporting this issues, I believe encountering this TLB flush problem might not be so common. Perhaps we could wait until you've finished implementing the "new vmalloc_fault()" feature. If anyone encounters problems in the meantime, I think they can temporarily apply either my patch or yours to workaround the issue of updating TLB for vmalloc. Best regards, Dylan Jhong
diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h index b55ba20903ec..6c86ab69687e 100644 --- a/arch/riscv/include/asm/page.h +++ b/arch/riscv/include/asm/page.h @@ -21,6 +21,8 @@ #define HPAGE_MASK (~(HPAGE_SIZE - 1)) #define HUGETLB_PAGE_ORDER (HPAGE_SHIFT - PAGE_SHIFT) +#define ARCH_PAGE_TABLE_SYNC_MASK PGTBL_PTE_MODIFIED + /* * PAGE_OFFSET -- the first address of the first page of memory. * When not using MMU this corresponds to the first free page in diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c index 77be59aadc73..d63364948c85 100644 --- a/arch/riscv/mm/tlbflush.c +++ b/arch/riscv/mm/tlbflush.c @@ -149,3 +149,15 @@ void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long start, __flush_tlb_range(vma->vm_mm, start, end - start, PMD_SIZE); } #endif + +/* + * Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, + * it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains + * the correct kernel mapping. arch_sync_kernel_mappings() will ensure that kernel + * page table mappings created via vmap/vmalloc() are updated before switching MM. + */ +void arch_sync_kernel_mappings(unsigned long start, unsigned long end) +{ + if (start < VMALLOC_END && end > VMALLOC_START) + flush_tlb_all(); +} \ No newline at end of file
Since RISC-V is a microarchitecture that allows caching invalid entries in the TLB, it is necessary to issue a "preventive" SFENCE.VMA to ensure that each core obtains the correct kernel mapping. The patch implements TLB flushing in arch_sync_kernel_mappings(), ensuring that kernel page table mappings created via vmap/vmalloc() are updated before switching MM. Signed-off-by: Dylan Jhong <dylan@andestech.com> --- arch/riscv/include/asm/page.h | 2 ++ arch/riscv/mm/tlbflush.c | 12 ++++++++++++ 2 files changed, 14 insertions(+)