Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 14 Jan 2021 11:46:49 -0800 (PST)
Subject:     Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32
In-Reply-To: <CAOnJCU+mCPwbeOQpmHmu3ar_17otmgftiKHLL+Z4_nExpj0=cA@mail.gmail.com>
CC:     Atish Patra <Atish.Patra@wdc.com>, aou@eecs.berkeley.edu,
        Anup Patel <Anup.Patel@wdc.com>, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org,
        Paul Walmsley <paul.walmsley@sifive.com>, mick@ics.forth.gr,
        akpm@linux-foundation.org, ardb@kernel.org, rppt@kernel.org
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     atishp@atishpatra.org
Message-ID: <mhng-4458ef4d-9443-491e-9118-aec3596058ed@penguin>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
>> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
>> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
>> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>> >
>> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> > ---
>> >  arch/riscv/include/asm/cache.h | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
>> > index 9b58b104559e..c9c669ea2fe6 100644
>> > --- a/arch/riscv/include/asm/cache.h
>> > +++ b/arch/riscv/include/asm/cache.h
>> > @@ -7,7 +7,11 @@
>> >  #ifndef _ASM_RISCV_CACHE_H
>> >  #define _ASM_RISCV_CACHE_H
>> >
>> > +#ifdef CONFIG_64BIT
>> >  #define L1_CACHE_SHIFT               6
>> > +#else
>> > +#define L1_CACHE_SHIFT               5
>> > +#endif
>> >
>> >  #define L1_CACHE_BYTES               (1 << L1_CACHE_SHIFT)
>>
>> Should we not instead just
>>
>> #define SMP_CACHE_BYTES L1_CACHE_BYTES
>>
>> like a handful of architectures do?
>>
>
> The generic code already defines it that way in include/linux/cache.h
>
>> The cache size is sort of fake here, as we don't have any non-coherent
>> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
>> cache lines in RISC-V implementations as software may assume that for
>> performance reasons.  Not really a strong reason, but I'd prefer to just make
>> these match.
>>
>
> If it is documented somewhere in the kernel, we should update that. I
> think SMP_CACHE_BYTES being 64
> actually degrades the performance as there will be a fragmented memory
> blocks with 32 bit bytes gap wherever
> SMP_CACHE_BYTES is used as an alignment requirement.

I don't buy that: if you're trying to align to the cache size then the gaps are 
the whole point.  IIUC the 64-byte cache lines come from DDR, not XLEN, so 
there's really no reason for these to be different between the base ISAs.

> In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> without this patch.
> I didn't see anything in Qemu though.

Something like that is probably only going to show up on real hardware, QEMU 
doesn't really do anything with the cache line size.  That said, as there's 
nothing in our kernel now related to non-coherent memory there really should 
only be performance issue (at least until we have non-coherent systems).

I'd bet that the change is just masking some other bug, either in the software 
or the hardware.  I'd prefer to root cause this rather than just working around 
it, as it'll probably come back later and in a more difficult way to find.

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