Received-SPF: pass (google.com: domain of linux-kernel+bounces-42070-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240123132730.2297719-1-alexghiti@rivosinc.com>
 <CAEEQ3wk5edUFTuE3H3KDGkCXj0+=i7Z1BM2M+6X-Tk9_m8X_iQ@mail.gmail.com>
 <ZbDICZkatO3/lGf/@snowbird> <cd68cf4e-7a6b-4875-ae1b-0c28a6eacaad@ghiti.fr> <65bd3d38-dcc0-4e64-b35b-8a2e697768ff@ghiti.fr>
In-Reply-To: <65bd3d38-dcc0-4e64-b35b-8a2e697768ff@ghiti.fr>
From: yunhui cui <cuiyunhui@bytedance.com>
Date: Mon, 29 Jan 2024 09:43:15 +0800
Message-ID: <CAEEQ3w=+yDpvsjd3NqZjJOCR8WaHyMBArpavvXPDgU5eKczk+g@mail.gmail.com>
Subject: Re: [External] [PATCH] riscv: Fix wrong size passed to local_flush_tlb_range_asid()
To: Alexandre Ghiti <alex@ghiti.fr>
Cc: Dennis Zhou <dennis@kernel.org>, Alexandre Ghiti <alexghiti@rivosinc.com>, 
	Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, 
	Albert Ou <aou@eecs.berkeley.edu>, Geert Uytterhoeven <geert@linux-m68k.org>, 
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Alexandre,

On Wed, Jan 24, 2024 at 4:41=E2=80=AFPM Alexandre Ghiti <alex@ghiti.fr> wro=
te:
>
> 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=E2=80=AFPM 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 a=
nd
> >>>> 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 lo=
ok
> >> 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=E2=80=99t 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