[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cd68cf4e-7a6b-4875-ae1b-0c28a6eacaad@ghiti.fr>
Date: Wed, 24 Jan 2024 09:38:30 +0100
From: Alexandre Ghiti <alex@...ti.fr>
To: Dennis Zhou <dennis@...nel.org>, yunhui cui <cuiyunhui@...edance.com>,
Alexandre Ghiti <alexghiti@...osinc.com>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Geert Uytterhoeven <geert@...ux-m68k.org>, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [External] [PATCH] riscv: Fix wrong size passed to
local_flush_tlb_range_asid()
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@...osinc.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@...osinc.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Powered by blists - more mailing lists