Re: [External] [PATCH] riscv: Fix wrong size passed to local_flush_tlb_range_asid()

[yunhui cui]

Hi Alexandre,

On Wed, Jan 24, 2024 at 4:41 PM Alexandre Ghiti <alex@xxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >>>> ---
> >>>>   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