ARCH_DMA_MINALIGN on PA-RISC

ARCH_DMA_MINALIGN on PA-RISC

[Mikulas Patocka]

Hi

Thanks for fixing the cache aliasing issues on PA-RISC in the commit 
72d95924ee35c8cd16ef52f912483ee938a34d49.

I think there is still one problem left - and that is ARCH_DMA_MINALIGN. 
Currently, it is 16, which is obviously wrong.

Some comments in the kernel say that PA8900 has L2 cache with 128-byte 
line size, so I think that ARCH_DMA_MINALIGN should be 128 as well.


The question is - can the CPU speculatively mark a cache line as dirty and 
write it back? If yes, we have a big problem - Linux assumes that a part 
of the page may be used for DMA transfer and another part of that page may 
be used for normal cacheable structures. If the PA-RISC CPU speculatively 
prefetched and wrote back a cache line, it could corrupt the DMA transfer.

If the CPU doesn't speculatively mark cache lines as dirty, then 
increasing ARCH_DMA_MINALIGN would be sufficient solution.

Mikulas

Re: ARCH_DMA_MINALIGN on PA-RISC

[John David Anglin]

On 2024-07-24 2:17 p.m., Mikulas Patocka wrote:
> Hi
>
> Thanks for fixing the cache aliasing issues on PA-RISC in the commit
> 72d95924ee35c8cd16ef52f912483ee938a34d49.
>
> I think there is still one problem left - and that is ARCH_DMA_MINALIGN.
> Currently, it is 16, which is obviously wrong.
>
> Some comments in the kernel say that PA8900 has L2 cache with 128-byte
Yes, that is correct.  The PA8800 and PA8900 processors have 128-byte cache lines.
Previous PA 2.0 processors have 64-byte lines.
> line size, so I think that ARCH_DMA_MINALIGN should be 128 as well.
>
>
> The question is - can the CPU speculatively mark a cache line as dirty and
> write it back? If yes, we have a big problem - Linux assumes that a part
> of the page may be used for DMA transfer and another part of that page may
> be used for normal cacheable structures. If the PA-RISC CPU speculatively
> prefetched and wrote back a cache line,ove-in it could corrupt the DMA transfer.
I don't believe a CPU can mark a cache line as dirty and write it back.  A CPU can
speculatively read a line if the TLB entry for it is installed and it permits move-in.
Only setting setting the T bit or setting access rights to zero inhibits move-in (see
do_alias macro).  The NO_CACHE bit is another option.

Currently, near all cache flushes are done by removing the TLB entry and then
the page is flushed using a tmpalias flush that inhibits movein. This ensures that the
CPU doesn't speculatively bring lines for the page back into the cache.
>
> If the CPU doesn't speculatively mark cache lines as dirty, then
> increasing ARCH_DMA_MINALIGN would be sufficient solution.
Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: ARCH_DMA_MINALIGN on PA-RISC

[James Bottomley]

On Wed, 2024-07-24 at 20:17 +0200, Mikulas Patocka wrote:
> Hi
> 
> Thanks for fixing the cache aliasing issues on PA-RISC in the commit 
> 72d95924ee35c8cd16ef52f912483ee938a34d49.
> 
> I think there is still one problem left - and that is
> ARCH_DMA_MINALIGN. Currently, it is 16, which is obviously wrong.

I don't think that's obvious, why is it wrong?

> 
> Some comments n the kernel say that PA8900 has L2 cache with 128-byte
> line size, so I think that ARCH_DMA_MINALIGN should be 128 as well.

The L2+ caches on PA88 and 89 systems are PIPT and fully coherent with
the PCI bus, so the L2+ line size doesn't matter that much (well,
except we could possibly get better performance with more judicious DMA
alignment).

All the parisc coherency protocols rely on the CPU L1 cache, which is
still VIPT.  Additionally, the CPU architects kept the minimum line
size for the L1 at 16, so even in the later CPUs which have larger
actual VIPT cache line sizes there's a splitting mechanism which means
they can operate coherency protocols at a line size of 16.  This was
done so the only spinlock primitive parisc has (LDCW) can still operate
correctly with only 16 bytes of alignment.

> The question is - can the CPU speculatively mark a cache line as
> dirty and write it back?

No, the CPU may only mark a line as dirty if something actually wrote
to it; it may not do it speculatively.  The L1 cache can speculatively
move in clean lines if a TLB exists for them and once a line is marked
dirty it's within the gift of the CPU to decide when to write it back
absent a flush.

>  If yes, we have a big problem - Linux assumes that a part of the
> page may be used for DMA transfer and another part of that page may
> be used for normal cacheable structures. If the PA-RISC CPU
> speculatively  prefetched and wrote back a cache line, it could
> corrupt the DMA transfer.

The L2 PIPT PCI coherence protocol ensures that DMA can't corrupt
memory adjacent objects on PA88 and 89.  Earlier CPUs, which were fully
VIPT, do suffer from this problem because they have no PCI coherence,
but they all operate at a line size of 16 anyway and so
ARCH_DMA_MINALIGN works for them.

> If the CPU doesn't speculatively mark cache lines as dirty, then 
> increasing ARCH_DMA_MINALIGN would be sufficient solution.

Well, it's relatively safe to try without exploding all our hashed
spinlocks because the LDCW alignment isn't tied to this (it's a
separate #define in ldcw.h) if you want to benchmark it.

James

Re: ARCH_DMA_MINALIGN on PA-RISC

[John David Anglin]

On 2024-07-24 3:25 p.m., James Bottomley wrote:
> On Wed, 2024-07-24 at 20:17 +0200, Mikulas Patocka wrote:
>> Hi
>>
>> Thanks for fixing the cache aliasing issues on PA-RISC in the commit
>> 72d95924ee35c8cd16ef52f912483ee938a34d49.
>>
>> I think there is still one problem left - and that is
>> ARCH_DMA_MINALIGN. Currently, it is 16, which is obviously wrong.
> I don't think that's obvious, why is it wrong?
I see this comment in arch/arm64/include/asm/cache.h:

/*
  * Memory returned by kmalloc() may be used for DMA, so we must make
  * sure that all such allocations are cache aligned. Otherwise,
  * unrelated code may cause parts of the buffer to be read into the
  * cache before the transfer is done, causing old data to be seen by
  * the CPU.
  */
#define ARCH_DMA_MINALIGN       (128)
#define ARCH_KMALLOC_MINALIGN   (8)

L1_CACHE_BYTES is 64 on arm64.

Possibly, the same can occur on parisc.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: ARCH_DMA_MINALIGN on PA-RISC

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 1958 bytes --]



On Thu, 25 Jul 2024, John David Anglin wrote:

> On 2024-07-24 3:25 p.m., James Bottomley wrote:
> > On Wed, 2024-07-24 at 20:17 +0200, Mikulas Patocka wrote:
> >> Hi
> >>
> >> Thanks for fixing the cache aliasing issues on PA-RISC in the commit
> >> 72d95924ee35c8cd16ef52f912483ee938a34d49.
> >>
> >> I think there is still one problem left - and that is
> >> ARCH_DMA_MINALIGN. Currently, it is 16, which is obviously wrong.
> > I don't think that's obvious, why is it wrong?
> I see this comment in arch/arm64/include/asm/cache.h:
> 
> /*
>  * Memory returned by kmalloc() may be used for DMA, so we must make
>  * sure that all such allocations are cache aligned. Otherwise,
>  * unrelated code may cause parts of the buffer to be read into the
>  * cache before the transfer is done, causing old data to be seen by
>  * the CPU.
>  */
> #define ARCH_DMA_MINALIGN       (128)
> #define ARCH_KMALLOC_MINALIGN   (8)
> 
> L1_CACHE_BYTES is 64 on arm64.
> 
> Possibly, the same can occur on parisc.
> 
> Dave

L1_CACHE_BYTES is a performance hint that is used to avoid cache line 
ping-pong when multiple CPUs modify nearby data.

ARCH_DMA_MINALIGN is the biggest possible cache line size to avoid DMA 
data corruption. As there are some arm64 machines with 128-byte cache 
line, arm64 has to define it to 128.

James said that the L2 cache on PA8800/8900 is coherent with PCI. So, I 
think that ARCH_DMA_MINALIGN should be 64 (is that the L1 cache line size 
on PA8800/8900?).

L1_CACHE_BYTES could be 128 to avoid ping-pong between sockets.

For ARCH_KMALLOC_MINALIGN, there is an arm64 commit 
9382bc44b5f58ccee375f08f518e53c0280051dc, it is an optimization, so that 
they can use cache line size probed at startup instead of 128. I think we 
don't have to do this optimization on PA-RISC, the PA-RISC machines with 
16-byte or 32-byte cache line size are old and slow - so that there is no 
point in trying to optimize kmalloc pools for them.

Mikulas

Re: ARCH_DMA_MINALIGN on PA-RISC

[James Bottomley]

On Thu, 2024-07-25 at 19:27 +0200, Mikulas Patocka wrote:
> 
> 
> On Thu, 25 Jul 2024, John David Anglin wrote:
> 
> > On 2024-07-24 3:25 p.m., James Bottomley wrote:
> > > On Wed, 2024-07-24 at 20:17 +0200, Mikulas Patocka wrote:
> > > > Hi
> > > > 
> > > > Thanks for fixing the cache aliasing issues on PA-RISC in the
> > > > commit 72d95924ee35c8cd16ef52f912483ee938a34d49.
> > > > 
> > > > I think there is still one problem left - and that is
> > > > ARCH_DMA_MINALIGN. Currently, it is 16, which is obviously
> > > > wrong.
> > > I don't think that's obvious, why is it wrong?
> > I see this comment in arch/arm64/include/asm/cache.h:
> > 
> > /*
> >  * Memory returned by kmalloc() may be used for DMA, so we must
> > make
> >  * sure that all such allocations are cache aligned. Otherwise,
> >  * unrelated code may cause parts of the buffer to be read into the
> >  * cache before the transfer is done, causing old data to be seen
> > by
> >  * the CPU.
> >  */

This comment is copied from the same file in arch/arm.  arm is mostly
VIVT caching and has even worse problems than the PA VIPT cache. 
aarch64 has variable cache policy (CTR_EL0 register), but I think most
of them are actually PIPT or VIPT.

> > #define ARCH_DMA_MINALIGN       (128)
> > #define ARCH_KMALLOC_MINALIGN   (8)
> > 
> > L1_CACHE_BYTES is 64 on arm64.
> > 
> > Possibly, the same can occur on parisc.
> > 
> > Dave
> 
> L1_CACHE_BYTES is a performance hint that is used to avoid cache line
> ping-pong when multiple CPUs modify nearby data.
> 
> ARCH_DMA_MINALIGN is the biggest possible cache line size to avoid
> DMA data corruption. As there are some arm64 machines with 128-byte
> cache line, arm64 has to define it to 128.
> 
> James said that the L2 cache on PA8800/8900 is coherent with PCI. So,
> I  think that ARCH_DMA_MINALIGN should be 64 (is that the L1 cache
> line size  on PA8800/8900?).

By default, if unset, ARCH_DMA_MINALIGN defaults to alignof(long long),
so it is already 64 on parisc.

James

> L1_CACHE_BYTES could be 128 to avoid ping-pong between sockets.
> 
> For ARCH_KMALLOC_MINALIGN, there is an arm64 commit 
> 9382bc44b5f58ccee375f08f518e53c0280051dc, it is an optimization, so
> that they can use cache line size probed at startup instead of 128. I
> think we don't have to do this optimization on PA-RISC, the PA-RISC
> machines with 16-byte or 32-byte cache line size are old and slow -
> so that there is no point in trying to optimize kmalloc pools for
> them.
> 
> Mikulas

Re: ARCH_DMA_MINALIGN on PA-RISC

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]



On Thu, 25 Jul 2024, James Bottomley wrote:

> > > #define ARCH_DMA_MINALIGN       (128)
> > > #define ARCH_KMALLOC_MINALIGN   (8)
> > > 
> > > L1_CACHE_BYTES is 64 on arm64.
> > > 
> > > Possibly, the same can occur on parisc.
> > > 
> > > Dave
> > 
> > L1_CACHE_BYTES is a performance hint that is used to avoid cache line
> > ping-pong when multiple CPUs modify nearby data.
> > 
> > ARCH_DMA_MINALIGN is the biggest possible cache line size to avoid
> > DMA data corruption. As there are some arm64 machines with 128-byte
> > cache line, arm64 has to define it to 128.
> > 
> > James said that the L2 cache on PA8800/8900 is coherent with PCI. So,
> > I  think that ARCH_DMA_MINALIGN should be 64 (is that the L1 cache
> > line size  on PA8800/8900?).
> 
> By default, if unset, ARCH_DMA_MINALIGN defaults to alignof(long long),
> so it is already 64 on parisc.
> 
> James

No, alignof(long long) is 8 :-)

Mikulas

Re: ARCH_DMA_MINALIGN on PA-RISC

[James Bottomley]

On Thu, 2024-07-25 at 19:46 +0200, Mikulas Patocka wrote:
> 
> 
> On Thu, 25 Jul 2024, James Bottomley wrote:
> 
> > > > #define ARCH_DMA_MINALIGN       (128)
> > > > #define ARCH_KMALLOC_MINALIGN   (8)
> > > > 
> > > > L1_CACHE_BYTES is 64 on arm64.
> > > > 
> > > > Possibly, the same can occur on parisc.
> > > > 
> > > > Dave
> > > 
> > > L1_CACHE_BYTES is a performance hint that is used to avoid cache
> > > line ping-pong when multiple CPUs modify nearby data.
> > > 
> > > ARCH_DMA_MINALIGN is the biggest possible cache line size to
> > > avoid DMA data corruption. As there are some arm64 machines with
> > > 128-byte cache line, arm64 has to define it to 128.
> > > 
> > > James said that the L2 cache on PA8800/8900 is coherent with PCI.
> > > So, I  think that ARCH_DMA_MINALIGN should be 64 (is that the L1
> > > cache line size  on PA8800/8900?).
> > 
> > By default, if unset, ARCH_DMA_MINALIGN defaults to alignof(long
> > long), so it is already 64 on parisc.
> > 
> > James
> 
> No, alignof(long long) is 8 :-)

Sorry, thinking in bits not bytes.  We definitely need that to be at
least 16 for all the pure VIPT systems.

James

Re: ARCH_DMA_MINALIGN on PA-RISC

[John David Anglin]

On 2024-07-25 1:27 p.m., Mikulas Patocka wrote:
>
> On Thu, 25 Jul 2024, John David Anglin wrote:
>
>> On 2024-07-24 3:25 p.m., James Bottomley wrote:
>>> On Wed, 2024-07-24 at 20:17 +0200, Mikulas Patocka wrote:
>>>> Hi
>>>>
>>>> Thanks for fixing the cache aliasing issues on PA-RISC in the commit
>>>> 72d95924ee35c8cd16ef52f912483ee938a34d49.
>>>>
>>>> I think there is still one problem left - and that is
>>>> ARCH_DMA_MINALIGN. Currently, it is 16, which is obviously wrong.
>>> I don't think that's obvious, why is it wrong?
>> I see this comment in arch/arm64/include/asm/cache.h:
>>
>> /*
>>   * Memory returned by kmalloc() may be used for DMA, so we must make
>>   * sure that all such allocations are cache aligned. Otherwise,
>>   * unrelated code may cause parts of the buffer to be read into the
>>   * cache before the transfer is done, causing old data to be seen by
>>   * the CPU.
>>   */
>> #define ARCH_DMA_MINALIGN       (128)
>> #define ARCH_KMALLOC_MINALIGN   (8)
>>
>> L1_CACHE_BYTES is 64 on arm64.
>>
>> Possibly, the same can occur on parisc.
>>
>> Dave
> L1_CACHE_BYTES is a performance hint that is used to avoid cache line
> ping-pong when multiple CPUs modify nearby data.
Our L1_CACHE_BYTES define is wrong.  PA7100 has a L1 length of 16 bytes.
PA7200 to PA7300LC have a length of 32 bytes.  PA8000 to PA8700 have a length of 64
bytes.  PA8800 and PA8900 have a L1 length of 128 bytes (this is from ERS D_Stride).

Thus, L1_CACHE_BYTES should be 128 for CONFIG_PA20 and 32 otherwise.
> ARCH_DMA_MINALIGN is the biggest possible cache line size to avoid DMA
> data corruption. As there are some arm64 machines with 128-byte cache
> line, arm64 has to define it to 128.
We can continue to define ARCH_DMA_MINALIGN to be L1_CACHE_BYTES.
>
> James said that the L2 cache on PA8800/8900 is coherent with PCI. So, I
> think that ARCH_DMA_MINALIGN should be 64 (is that the L1 cache line size
> on PA8800/8900?).
>
> L1_CACHE_BYTES could be 128 to avoid ping-pong between sockets.
>
> For ARCH_KMALLOC_MINALIGN, there is an arm64 commit
I believe we need to define ARCH_KMALLOC_MINALIGN.
> 9382bc44b5f58ccee375f08f518e53c0280051dc, it is an optimization, so that
> they can use cache line size probed at startup instead of 128. I think we
> don't have to do this optimization on PA-RISC, the PA-RISC machines with
> 16-byte or 32-byte cache line size are old and slow - so that there is no
> point in trying to optimize kmalloc pools for them.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: ARCH_DMA_MINALIGN on PA-RISC

[John David Anglin]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On 2024-07-25 3:13 p.m., John David Anglin wrote:
>> L1_CACHE_BYTES is a performance hint that is used to avoid cache line
>> ping-pong when multiple CPUs modify nearby data.
> Our L1_CACHE_BYTES define is wrong.  PA7100 has a L1 length of 16 bytes.
> PA7200 to PA7300LC have a length of 32 bytes.  PA8000 to PA8700 have a length of 64
> bytes.  PA8800 and PA8900 have a L1 length of 128 bytes (this is from ERS D_Stride).
>
The attached patch fixes the defines for L1_CACHE_SHIFT, L1_CACHE_BYTES and
ARCH_DMA_MINALIGN.  I left ARCH_DMA_MINALIGN at 16 as I believe this gives
128-byte alignment.

Testing.

Dave

-- 
John David Anglin  dave.anglin@bell.net

[-- Attachment #2: L1_CACHE_BYTES.d --]
[-- Type: text/plain, Size: 924 bytes --]

diff --git a/arch/parisc/include/asm/cache.h b/arch/parisc/include/asm/cache.h
index 2a60d7a72f1f..e9b0081546c9 100644
--- a/arch/parisc/include/asm/cache.h
+++ b/arch/parisc/include/asm/cache.h
@@ -9,18 +9,21 @@
 #include <asm/alternative.h>
 
 /*
- * PA 2.0 processors have 64 and 128-byte L2 cachelines; PA 1.1 processors
- * have 32-byte cachelines.  The L1 length appears to be 16 bytes but this
- * is not clearly documented.
+ * PA 2.0 processors have 64 and 128-byte cachelines; PA 1.1 processors
+ * have 32-byte cachelines.
  */
-#define L1_CACHE_BYTES 16
-#define L1_CACHE_SHIFT 4
+#ifdef CONFIG_PA20
+#define L1_CACHE_SHIFT 7
+#else
+#define L1_CACHE_SHIFT 5
+#endif
+#define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
 
 #ifndef __ASSEMBLY__
 
 #define SMP_CACHE_BYTES L1_CACHE_BYTES
 
-#define ARCH_DMA_MINALIGN	L1_CACHE_BYTES
+#define ARCH_DMA_MINALIGN	16
 
 #define __read_mostly __section(".data..read_mostly")
 

Re: ARCH_DMA_MINALIGN on PA-RISC

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]



On Thu, 25 Jul 2024, John David Anglin wrote:

> On 2024-07-25 3:13 p.m., John David Anglin wrote:
> >> L1_CACHE_BYTES is a performance hint that is used to avoid cache line
> >> ping-pong when multiple CPUs modify nearby data.
> > Our L1_CACHE_BYTES define is wrong.  PA7100 has a L1 length of 16 bytes.
> > PA7200 to PA7300LC have a length of 32 bytes.  PA8000 to PA8700 have a length of 64
> > bytes.  PA8800 and PA8900 have a L1 length of 128 bytes (this is from ERS D_Stride).
> >
> The attached patch fixes the defines for L1_CACHE_SHIFT, L1_CACHE_BYTES and
> ARCH_DMA_MINALIGN.  I left ARCH_DMA_MINALIGN at 16 as I believe this gives
> 128-byte alignment.
> 
> Testing.
> 
> Dave
> 
> -- 
> John David Anglin  dave.anglin@bell.net

How does ARCH_DMA_MINALIGN == 16 give 128-byte alignment? I think that 
ARCH_DMA_MINALIGN needs to be 128 to avoid the possibility of DMA transfer 
corruption.

L1_CACHE_SHIFT can be set to arbitrary value - setting it badly could 
degrade performance, but it shouldn't cause data corruption.

The commit d93277b9839b0bde06238a7a7f644114edb2ad4a says that setting 
L1_CACHE_SHIFT == 7 causes networking performance degradation on arm64. I 
don't know how much is this related to parisc.

Mikulas

Re: ARCH_DMA_MINALIGN on PA-RISC

[John David Anglin]

On 2024-07-25 4:36 p.m., Mikulas Patocka wrote:
>
> On Thu, 25 Jul 2024, John David Anglin wrote:
>
>> On 2024-07-25 3:13 p.m., John David Anglin wrote:
>>>> L1_CACHE_BYTES is a performance hint that is used to avoid cache line
>>>> ping-pong when multiple CPUs modify nearby data.
>>> Our L1_CACHE_BYTES define is wrong.  PA7100 has a L1 length of 16 bytes.
>>> PA7200 to PA7300LC have a length of 32 bytes.  PA8000 to PA8700 have a length of 64
>>> bytes.  PA8800 and PA8900 have a L1 length of 128 bytes (this is from ERS D_Stride).
>>>
>> The attached patch fixes the defines for L1_CACHE_SHIFT, L1_CACHE_BYTES and
>> ARCH_DMA_MINALIGN.  I left ARCH_DMA_MINALIGN at 16 as I believe this gives
>> 128-byte alignment.
>>
>> Testing.
>>
>> Dave
>>
>> -- 
>> John David Anglin  dave.anglin@bell.net
> How does ARCH_DMA_MINALIGN == 16 give 128-byte alignment? I think that
> ARCH_DMA_MINALIGN needs to be 128 to avoid the possibility of DMA transfer
> corruption.
You are right.  It should be 128.
>
> L1_CACHE_SHIFT can be set to arbitrary value - setting it badly could
> degrade performance, but it shouldn't cause data corruption.
If we set to an arbitrary value, we need to document why we do it. The naming
suggests that L1_CACHE_BYTES should be the L1 cache length.
>
> The commit d93277b9839b0bde06238a7a7f644114edb2ad4a says that setting
> L1_CACHE_SHIFT == 7 causes networking performance degradation on arm64. I
> don't know how much is this related to parisc.
Some other architectures have L1_CACHE_SHIFT == 7.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: ARCH_DMA_MINALIGN on PA-RISC

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]



On Thu, 25 Jul 2024, John David Anglin wrote:

> On 2024-07-25 4:36 p.m., Mikulas Patocka wrote:
> >
> > On Thu, 25 Jul 2024, John David Anglin wrote:
> >
> >> On 2024-07-25 3:13 p.m., John David Anglin wrote:
> >>>> L1_CACHE_BYTES is a performance hint that is used to avoid cache line
> >>>> ping-pong when multiple CPUs modify nearby data.
> >>> Our L1_CACHE_BYTES define is wrong.  PA7100 has a L1 length of 16 bytes.
> >>> PA7200 to PA7300LC have a length of 32 bytes.  PA8000 to PA8700 have a length of 64
> >>> bytes.  PA8800 and PA8900 have a L1 length of 128 bytes (this is from ERS D_Stride).
> >>>
> >> The attached patch fixes the defines for L1_CACHE_SHIFT, L1_CACHE_BYTES and
> >> ARCH_DMA_MINALIGN.  I left ARCH_DMA_MINALIGN at 16 as I believe this gives
> >> 128-byte alignment.
> >>
> >> Testing.
> >>
> >> Dave
> >>
> >> -- 
> >> John David Anglin  dave.anglin@bell.net
> > How does ARCH_DMA_MINALIGN == 16 give 128-byte alignment? I think that
> > ARCH_DMA_MINALIGN needs to be 128 to avoid the possibility of DMA transfer
> > corruption.
> You are right.  It should be 128.

I've sent a patch that fixes it - and it also uses dcache_stride to 
advertise the probed cache line size to the kernel, so that on machines 
older than PA8800 we can use slab caches smaller than 128 bytes.

> > L1_CACHE_SHIFT can be set to arbitrary value - setting it badly could
> > degrade performance, but it shouldn't cause data corruption.
> If we set to an arbitrary value, we need to document why we do it. The naming
> suggests that L1_CACHE_BYTES should be the L1 cache length.

It's hard to say what should we set it to, if we have different 
microarchitectures with different cache line size. ARM64 sets it to 64, 
despite the fact that there are some ARM64 machines with 128-byte cache 
line.

L1_CACHE_BYTES is a matter of performance. Do you have some benchmarks, so 
that you could try to tune it?

The commit a01fece2e4185ac173abd16d10304d73d47ebf00 says that setting 
L1_CACHE_BYTES == 16 improves performance.

> > The commit d93277b9839b0bde06238a7a7f644114edb2ad4a says that setting
> > L1_CACHE_SHIFT == 7 causes networking performance degradation on arm64. I
> > don't know how much is this related to parisc.
> Some other architectures have L1_CACHE_SHIFT == 7.
> 
> Dave

Mikulas

Re: ARCH_DMA_MINALIGN on PA-RISC

[John David Anglin]

On 2024-07-27 6:24 a.m., Mikulas Patocka wrote:
>>> L1_CACHE_SHIFT can be set to arbitrary value - setting it badly could
>>> degrade performance, but it shouldn't cause data corruption.
>> If we set to an arbitrary value, we need to document why we do it. The naming
>> suggests that L1_CACHE_BYTES should be the L1 cache length.
> It's hard to say what should we set it to, if we have different
> microarchitectures with different cache line size. ARM64 sets it to 64,
> despite the fact that there are some ARM64 machines with 128-byte cache
> line.
>
> L1_CACHE_BYTES is a matter of performance. Do you have some benchmarks, so
> that you could try to tune it?
>
> The commit a01fece2e4185ac173abd16d10304d73d47ebf00 says that setting
> L1_CACHE_BYTES == 16 improves performance.
I can't recall what tests were used at the time.  However, L1_CACHE_BYTES == 16 reduces
the kernel size and this probably helps performance.  16 bytes would be the minimum alignment
for ldcw locks.

The comment about the L1 line length being 16 bytes is wrong.  The notes we have from the
PA8800 ERS clearly state that the L1 stride is 128 bytes.  The notes show how to flush the L1
cache.  I think the L1 and L2 lengths have always been the same.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: ARCH_DMA_MINALIGN on PA-RISC

[Mikulas Patocka]

On Wed, 24 Jul 2024, James Bottomley wrote:

> On Wed, 2024-07-24 at 20:17 +0200, Mikulas Patocka wrote:
> > Hi
> > 
> > Thanks for fixing the cache aliasing issues on PA-RISC in the commit 
> > 72d95924ee35c8cd16ef52f912483ee938a34d49.
> > 
> > I think there is still one problem left - and that is
> > ARCH_DMA_MINALIGN. Currently, it is 16, which is obviously wrong.
> 
> I don't think that's obvious, why is it wrong?

Suppose that two unrelated kernel subsystems allocate a 16-byte region and 
the regions happen to be placed next to each other - in the same 
cacheline.

The first subsystem does DMA into its 16-byte region and the second 
subsystem writes to its region using cacheable write.

Because the regions share a cache line, the cacheable write may corrupt 
the data that were loaded using DMA. For example
1. load a cache line into CPU cache
2. DMA writes to the region that was loaded
3. the cache line is modified
4. the cache line is written back and it overwrites the result of the DMA 
operation

ARCH_DMA_MINALIGN prevents this situation from hapenning. See the 
explanation of ARCH_DMA_MINALIGN in 
Documentation/core-api/dma-api-howto.rst

> > Some comments n the kernel say that PA8900 has L2 cache with 128-byte
> > line size, so I think that ARCH_DMA_MINALIGN should be 128 as well.
> 
> The L2+ caches on PA88 and 89 systems are PIPT and fully coherent with
> the PCI bus, so the L2+ line size doesn't matter that much (well,
> except we could possibly get better performance with more judicious DMA
> alignment).

I didn't know that.

> All the parisc coherency protocols rely on the CPU L1 cache, which is
> still VIPT.  Additionally, the CPU architects kept the minimum line
> size for the L1 at 16, so even in the later CPUs which have larger
> actual VIPT cache line sizes there's a splitting mechanism which means
> they can operate coherency protocols at a line size of 16.  This was
> done so the only spinlock primitive parisc has (LDCW) can still operate
> correctly with only 16 bytes of alignment.

But you still need to flush L1 cache when doing DMA.

> > The question is - can the CPU speculatively mark a cache line as
> > dirty and write it back?
> 
> No, the CPU may only mark a line as dirty if something actually wrote
> to it; it may not do it speculatively.  The L1 cache can speculatively
> move in clean lines if a TLB exists for them and once a line is marked
> dirty it's within the gift of the CPU to decide when to write it back
> absent a flush.

That's good.

> >  If yes, we have a big problem - Linux assumes that a part of the
> > page may be used for DMA transfer and another part of that page may
> > be used for normal cacheable structures. If the PA-RISC CPU
> > speculatively  prefetched and wrote back a cache line, it could
> > corrupt the DMA transfer.
> 
> The L2 PIPT PCI coherence protocol ensures that DMA can't corrupt
> memory adjacent objects on PA88 and 89.  Earlier CPUs, which were fully
> VIPT, do suffer from this problem because they have no PCI coherence,
> but they all operate at a line size of 16 anyway and so
> ARCH_DMA_MINALIGN works for them.

I read somewhere on internet that PA7300LC has 32-byte cache line size. 
So, ARCH_DMA_MINALIGN 16 is too small for it.

> > If the CPU doesn't speculatively mark cache lines as dirty, then 
> > increasing ARCH_DMA_MINALIGN would be sufficient solution.
> 
> Well, it's relatively safe to try without exploding all our hashed
> spinlocks because the LDCW alignment isn't tied to this (it's a
> separate #define in ldcw.h) if you want to benchmark it.
> 
> James

Mikulas