| Message ID | |
|---|---|
| State | Accepted |
| Delegated to: | James Hogan |
| Headers | show |
| Series |
|
| Related | show |
Hi James, this hasn't appear in linux-next yet, or in any branch of git://git.kernel.org/pub/scm/linux/kernel/git/jhogan/mips.git Should I expect it to? Thanks, NeilBrown On Fri, Apr 27 2018, NeilBrown wrote: > When DMA will be performed to a MIPS32 1004K CPS, the > L1-cache for the range needs to be flushed and invalidated > first. > The code currently takes one of two approaches. > 1/ If the range is less than the size of the dcache, then > HIT type requests flush/invalidate cache lines for the > particular addresses. HIT-type requests a globalised > by the CPS so this is safe on SMP. > > 2/ If the range is larger than the size of dcache, then > INDEX type requests flush/invalidate the whole cache. > INDEX type requests affect the local cache only. CPS > does not propagate them in any way. So this invalidation > is not safe on SMP CPS systems. > > Data corruption due to '2' can quite easily be demonstrated by > repeatedly "echo 3 > /proc/sys/vm/drop_caches" and then sha1sum > a file that is several times the size of available memory. > Dropping caches means that large contiguous extents (large than > dcache) are more likely. > > This was not a problem before Linux-4.8 because option 2 was > never used if CONFIG_MIPS_CPS was defined. The commit > which removed that apparently didn't appreciate the full > consequence of the change. > > We could, in theory, globalize the INDEX based flush by sending an IPI > to other cores. These cache invalidation routines can be called with > interrupts disabled and synchronous IPI require interrupts to be > enabled. Asynchronous IPI may not trigger writeback soon enough. > So we cannot use IPI in practice. > > We can already test is IPI would be needed for an INDEX operation > with r4k_op_needs_ipi(R4K_INDEX). If this is True then we mustn't try > the INDEX approach as we cannot use IPI. If this is False (e.g. when > there is only one core and hence one L1 cache) then it is safe to > use the INDEX approach without IPI. > > This patch avoids options 2 if r4k_op_needs_ipi(R4K_INDEX), and so > eliminates the corruption. > > Fixes: c00ab4896ed5 ("MIPS: Remove cpu_has_safe_index_cacheops") > Cc: # v4.8+ > Signed-off-by: NeilBrown <> > --- > arch/mips/mm/c-r4k.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c > index 6f534b209971..e12dfa48b478 100644 > --- a/arch/mips/mm/c-r4k.c > +++ b/arch/mips/mm/c-r4k.c > @@ -851,9 +851,12 @@ static void r4k_dma_cache_wback_inv(unsigned long addr, unsigned long size) > /* > * Either no secondary cache or the available caches don't have the > * subset property so we have to flush the primary caches > - * explicitly > + * explicitly. > + * If we would need IPI to perform an INDEX-type operation, then > + * we have to use the HIT-type alternative as IPI cannot be used > + * here due to interrupts possibly being disabled. > */ > - if (size >= dcache_size) { > + if (!r4k_op_needs_ipi(R4K_INDEX) && size >= dcache_size) { > r4k_blast_dcache(); > } else { > R4600_HIT_CACHEOP_WAR_IMPL; > @@ -890,7 +893,7 @@ static void r4k_dma_cache_inv(unsigned long addr, unsigned long size) > return; > } > > - if (size >= dcache_size) { > + if (!r4k_op_needs_ipi(R4K_INDEX) && size >= dcache_size) { > r4k_blast_dcache(); > } else { > R4600_HIT_CACHEOP_WAR_IMPL; > -- > 2.14.0.rc0.dirty
On Mon, May 07, 2018 at 07:40:49AM +1000, NeilBrown wrote: > > Hi James, > this hasn't appear in linux-next yet, or in any branch > of > git://git.kernel.org/pub/scm/linux/kernel/git/jhogan/mips.git > > Should I expect it to? Sorry Neil, I haven't applied it yet. I'm planning to get a few fixes sorted this week, at which point it would land in the mips-fixes branch at the above repo. Cheers James
On Mon, May 07 2018, James Hogan wrote: > On Mon, May 07, 2018 at 07:40:49AM +1000, NeilBrown wrote: >> >> Hi James, >> this hasn't appear in linux-next yet, or in any branch >> of >> git://git.kernel.org/pub/scm/linux/kernel/git/jhogan/mips.git >> >> Should I expect it to? > > Sorry Neil, I haven't applied it yet. I'm planning to get a few fixes > sorted this week, at which point it would land in the mips-fixes branch > at the above repo. Cool, thanks. I just wanted to be sure it hadn't got lost somehow. Thanks, NeilBrown
On Tue, May 08, 2018 at 11:22:36AM +1000, NeilBrown wrote: > On Mon, May 07 2018, James Hogan wrote: > > > On Mon, May 07, 2018 at 07:40:49AM +1000, NeilBrown wrote: > >> > >> Hi James, > >> this hasn't appear in linux-next yet, or in any branch > >> of > >> git://git.kernel.org/pub/scm/linux/kernel/git/jhogan/mips.git > >> > >> Should I expect it to? > > > > Sorry Neil, I haven't applied it yet. I'm planning to get a few fixes > > sorted this week, at which point it would land in the mips-fixes branch > > at the above repo. > > Cool, thanks. I just wanted to be sure it hadn't got lost somehow. Now pushed. Thanks James
diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c index 6f534b209971..e12dfa48b478 100644 --- a/arch/mips/mm/c-r4k.c +++ b/arch/mips/mm/c-r4k.c @@ -851,9 +851,12 @@ static void r4k_dma_cache_wback_inv(unsigned long addr, unsigned long size) /* * Either no secondary cache or the available caches don't have the * subset property so we have to flush the primary caches - * explicitly + * explicitly. + * If we would need IPI to perform an INDEX-type operation, then + * we have to use the HIT-type alternative as IPI cannot be used + * here due to interrupts possibly being disabled. */ - if (size >= dcache_size) { + if (!r4k_op_needs_ipi(R4K_INDEX) && size >= dcache_size) { r4k_blast_dcache(); } else { R4600_HIT_CACHEOP_WAR_IMPL; @@ -890,7 +893,7 @@ static void r4k_dma_cache_inv(unsigned long addr, unsigned long size) return; } - if (size >= dcache_size) { + if (!r4k_op_needs_ipi(R4K_INDEX) && size >= dcache_size) { r4k_blast_dcache(); } else { R4600_HIT_CACHEOP_WAR_IMPL;
When DMA will be performed to a MIPS32 1004K CPS, the L1-cache for the range needs to be flushed and invalidated first. The code currently takes one of two approaches. 1/ If the range is less than the size of the dcache, then HIT type requests flush/invalidate cache lines for the particular addresses. HIT-type requests a globalised by the CPS so this is safe on SMP. 2/ If the range is larger than the size of dcache, then INDEX type requests flush/invalidate the whole cache. INDEX type requests affect the local cache only. CPS does not propagate them in any way. So this invalidation is not safe on SMP CPS systems. Data corruption due to '2' can quite easily be demonstrated by repeatedly "echo 3 > /proc/sys/vm/drop_caches" and then sha1sum a file that is several times the size of available memory. Dropping caches means that large contiguous extents (large than dcache) are more likely. This was not a problem before Linux-4.8 because option 2 was never used if CONFIG_MIPS_CPS was defined. The commit which removed that apparently didn't appreciate the full consequence of the change. We could, in theory, globalize the INDEX based flush by sending an IPI to other cores. These cache invalidation routines can be called with interrupts disabled and synchronous IPI require interrupts to be enabled. Asynchronous IPI may not trigger writeback soon enough. So we cannot use IPI in practice. We can already test is IPI would be needed for an INDEX operation with r4k_op_needs_ipi(R4K_INDEX). If this is True then we mustn't try the INDEX approach as we cannot use IPI. If this is False (e.g. when there is only one core and hence one L1 cache) then it is safe to use the INDEX approach without IPI. This patch avoids options 2 if r4k_op_needs_ipi(R4K_INDEX), and so eliminates the corruption. Fixes: c00ab4896ed5 ("MIPS: Remove cpu_has_safe_index_cacheops") Cc: # v4.8+ Signed-off-by: NeilBrown <> --- arch/mips/mm/c-r4k.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)