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

From: Alexandre Ghiti
Date: Wed Jan 24 2024 - 03:38:53 EST


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.

Thanks,

Alex


Thanks,
Dennis

_______________________________________________
linux-riscv mailing list
linux-riscv@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-riscv