From: NeilBrown <neil@brown.name>
To: James Hogan <jhogan@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>,
Paul Burton <paul.burton@mips.com>,
linux-mips@linux-mips.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] MIPS: c-r4k: fix data corruption related to cache coherence.
Date: Mon, 07 May 2018 07:40:49 +1000 [thread overview]
Message-ID: <87lgcwcvj2.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <87vacdlf8d.fsf@notabene.neil.brown.name>
[-- Attachment #1: Type: text/plain, Size: 3574 bytes --]
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: stable@vger.kernel.org # v4.8+
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
next prev parent reply other threads:[~2018-05-06 21:41 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-25 4:08 [PATCH] MIPS: c-r4k: fix data corruption related to cache coherence NeilBrown
2018-04-25 21:46 ` James Hogan
2018-04-25 22:00 ` NeilBrown
2018-04-25 22:08 ` James Hogan
2018-04-26 23:28 ` [PATCH v2] " NeilBrown
2018-05-06 21:40 ` NeilBrown [this message]
2018-05-07 20:16 ` James Hogan
2018-05-08 1:22 ` NeilBrown
2018-05-11 21:56 ` James Hogan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87lgcwcvj2.fsf@notabene.neil.brown.name \
--to=neil@brown.name \
--cc=jhogan@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@linux-mips.org \
--cc=paul.burton@mips.com \
--cc=ralf@linux-mips.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).