Messages in this thread | | | From | yunhui cui <> | Date | Mon, 29 Jan 2024 09:43:15 +0800 | Subject | Re: [External] [PATCH] riscv: Fix wrong size passed to local_flush_tlb_range_asid() |
| |
Hi Alexandre,
On Wed, Jan 24, 2024 at 4:41 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > On 24/01/2024 09:38, Alexandre Ghiti wrote: > > Hi Dennis, Yunhui, > > > > On 24/01/2024 09:19, Dennis Zhou wrote: > >> Hello, > >> > >> On Wed, Jan 24, 2024 at 10:44:12AM +0800, yunhui cui wrote: > >>> Hi Alexandre, > >>> > >>> On Tue, Jan 23, 2024 at 9:31 PM Alexandre Ghiti > >>> <alexghiti@rivosinc.com> wrote: > >>>> local_flush_tlb_range_asid() takes the size as argument, not the > >>>> end of > >>>> the range to flush, so fix this by computing the size from the end and > >>>> the start of the range. > >>>> > >>>> Fixes: 7a92fc8b4d20 ("mm: Introduce flush_cache_vmap_early()") > >>>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > >>>> --- > >>>> arch/riscv/mm/tlbflush.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/riscv/mm/tlbflush.c b/arch/riscv/mm/tlbflush.c > >>>> index 8d12b26f5ac3..9619965f6501 100644 > >>>> --- a/arch/riscv/mm/tlbflush.c > >>>> +++ b/arch/riscv/mm/tlbflush.c > >>>> @@ -68,7 +68,7 @@ static inline void > >>>> local_flush_tlb_range_asid(unsigned long start, > >>>> > >>>> void local_flush_tlb_kernel_range(unsigned long start, unsigned > >>>> long end) > >>>> { > >>>> - local_flush_tlb_range_asid(start, end, PAGE_SIZE, > >>>> FLUSH_TLB_NO_ASID); > >>>> + local_flush_tlb_range_asid(start, end - start, PAGE_SIZE, > >>>> FLUSH_TLB_NO_ASID); > >>>> } > >> Well this was a miss during code review.. I'm going to take another look > >> tomorrow and then likely pull this into a fixes branch. > >> > >>> What makes me curious is that this patch has not been tested? > >>> BTW, It is best to keep the parameter order of all functions in > >>> tlbflush.c consistent: cpumask, start, size, stride, asid. > >>> > >> I can't speak to the riscv communities testing/regression suites, but > >> this would only be caught in a performance regression test. > >> > >> That being said, Alexandre, can you please lmk what level of testing > >> this has gone through? > > > > > > All my patches go through the same level of testing: > > > > * Build/boot an Ubuntu kernel with and without KASAN + a few simple > > testsuites (libhugetlbfs, riscv kselftests and other) > > * Build/boot a simple rootfs on ~40 different rv64 configs > > * Build/boot a simple rootfs on ~30 different rv32 configs > > > > And I run LTP/full kselftests/perf testsuite on a weekly basis on > > every rc. All this validation is done on qemu. > > > > The patch is functional, it "simply" flushes the whole TLB instead of > > a few entries, so the only way to catch that would have been a > > performance regression. But given it only runs on qemu, it would have > > been hard to catch any performance regression since that involves the > > TLB. > > > > @Yunhui: Please let me know how I should validate my patches better. > > > @Yunhui: And BTW, we lack reviewers, so feel free to help ;)
Okay, if you don’t mind, I will also review the RISC-V TLB related patches later. BTW, I mailed a patch "RISC-V: add uniprocessor flush_tlb_range() support", and please help me review it, thank you ~
Thanks, Yunhui
| |