| Message ID | 20240123132730.2297719-1-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | riscv: Fix wrong size passed to local_flush_tlb_range_asid() | expand |
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); > } 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. Thanks, Yunhui
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? Thanks, Dennis
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. Thanks, Alex > Thanks, > Dennis > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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 ;) > > Thanks, > > Alex > > >> Thanks, >> Dennis >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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
Hi Alexandre, On Tue, Jan 23, 2024 at 02:27:30PM +0100, Alexandre Ghiti 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); > } > > static void __ipi_flush_tlb_all(void *info) > -- > 2.39.2 > Sorry for the delay, I just pulled this into percpu#for-6.8-fixes. I'll send it to Linus this week. Thanks, Dennis
On Mon, Jan 29, 2024 at 10:01 AM Dennis Zhou <dennis@kernel.org> wrote: > > Hi Alexandre, > > On Tue, Jan 23, 2024 at 02:27:30PM +0100, Alexandre Ghiti 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); > > } > > > > static void __ipi_flush_tlb_all(void *info) > > -- > > 2.39.2 > > > > Sorry for the delay, I just pulled this into percpu#for-6.8-fixes. I'll > send it to Linus this week. > > Thanks, > Dennis No problem, thanks! Alex
On Mon, 29 Jan 2024 01:01:00 PST (-0800), dennis@kernel.org wrote: > Hi Alexandre, > > On Tue, Jan 23, 2024 at 02:27:30PM +0100, Alexandre Ghiti 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); >> } >> >> static void __ipi_flush_tlb_all(void *info) >> -- >> 2.39.2 >> > > Sorry for the delay, I just pulled this into percpu#for-6.8-fixes. I'll > send it to Linus this week. Do you mind if we do a shared tag or something? It's going to conflict with https://lore.kernel.org/all/20240117140333.2479667-1-vincent.chen@sifive.com/ . No big deal as it's a pretty trivial conflict, but they'll both need stable backports. > > Thanks, > Dennis
Hi Palmer, On Wed, Jan 31, 2024 at 12:34:40PM -0800, Palmer Dabbelt wrote: > On Mon, 29 Jan 2024 01:01:00 PST (-0800), dennis@kernel.org wrote: > > Hi Alexandre, > > > > On Tue, Jan 23, 2024 at 02:27:30PM +0100, Alexandre Ghiti 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); > > > } > > > > > > static void __ipi_flush_tlb_all(void *info) > > > -- > > > 2.39.2 > > > > > > > Sorry for the delay, I just pulled this into percpu#for-6.8-fixes. I'll > > send it to Linus this week. > > Do you mind if we do a shared tag or something? It's going to conflict with > https://lore.kernel.org/all/20240117140333.2479667-1-vincent.chen@sifive.com/ > . No big deal as it's a pretty trivial conflict, but they'll both need > stable backports. This alone won't need a stable backport, I merged the bug as part of enabling the percpu page allocator in the recent 6.8 merge window. That being said, this is the only patch I'm carrying for v6.8. I'm happy to drop it and have you pick it up instead. Saves me a tag and a PR. Lmk if that works for you. Thanks, Dennis
On Wed, 31 Jan 2024 19:45:27 PST (-0800), dennis@kernel.org wrote: > Hi Palmer, > > On Wed, Jan 31, 2024 at 12:34:40PM -0800, Palmer Dabbelt wrote: >> On Mon, 29 Jan 2024 01:01:00 PST (-0800), dennis@kernel.org wrote: >> > Hi Alexandre, >> > >> > On Tue, Jan 23, 2024 at 02:27:30PM +0100, Alexandre Ghiti 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); >> > > } >> > > >> > > static void __ipi_flush_tlb_all(void *info) >> > > -- >> > > 2.39.2 >> > > >> > >> > Sorry for the delay, I just pulled this into percpu#for-6.8-fixes. I'll >> > send it to Linus this week. >> >> Do you mind if we do a shared tag or something? It's going to conflict with >> https://lore.kernel.org/all/20240117140333.2479667-1-vincent.chen@sifive.com/ >> . No big deal as it's a pretty trivial conflict, but they'll both need >> stable backports. > > This alone won't need a stable backport, I merged the bug as part of > enabling the percpu page allocator in the recent 6.8 merge window. > > That being said, this is the only patch I'm carrying for v6.8. I'm happy > to drop it and have you pick it up instead. Saves me a tag and a PR. > Lmk if that works for you. Sorry I missed this, but that seems like the easiest way to go here -- the other patch fixes the same bug (and another one), so I think we're safe with just that (which I'm sending to Linus as part of my rc4 fixes now-ish, as I'd need to anyway). It's d9807d60c145 ("riscv: mm: execute local TLB flush after populating vmemmap"), in case anyone's looking later...
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); } static void __ipi_flush_tlb_all(void *info)
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(-)