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

[Alexandre Ghiti]

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


> Thanks,
>
> Alex
>
>
> > Thanks,
> > Dennis
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv