On Fri, Jun 11, 2021 at 08:53:38AM +0200, Christoph Hellwig wrote:
> On Tue, Jun 08, 2021 at 06:48:22PM -0700, Ira Weiny wrote:
> > I'm still not 100% sure that these flushes are needed but the are not
> > no-ops on
> > every arch. Would it be best to preserve them after the
> > memcpy_to/from_bvec()?
> >
> > Same thing in patch 11 and 14.
>
> To me it seems kunmap_local should basically always call the equivalent
> of flush_kernel_dcache_page. parisc does this through
> kunmap_flush_on_unmap, but none of the other architectures with VIVT
> caches or other coherency issues does.
>
> Does anyone have a history or other insights here?
I went digging into the current callers of flush_kernel_dcache_page() other
than this one. To see if adding kunmap_flush_on_unmap() to the other arch's
would cause any problems.
In particular this call site stood out because it is not always called?!?!?!?
void sg_miter_stop(struct sg_mapping_iter *miter)
{
...
if ((miter->__flags & SG_MITER_TO_SG) &&
!PageSlab(miter->page))
flush_kernel_dcache_page(miter->page);
...
}
Looking at
3d77b50c5874 lib/scatterlist.c: don't flush_kernel_dcache_page on slab page[1]
It seems the restrictions they are quoting for the page are completely out of
date. I don't see any current way for a VM_BUG_ON() to be triggered. So is
this code really necessary?
More recently this was added:
7e34e0bbc644 crypto: omap-crypto - fix userspace copied buffer access
I'm CC'ing Tero and Herbert to see why they added the SLAB check.
Then we have interesting comments like this...
...
/* This can go away once MIPS implements
* flush_kernel_dcache_page */
flush_dcache_page(miter->page);
...
And some users optimizing.
...
/* discard mappings */
if (direction == DMA_FROM_DEVICE)
flush_kernel_dcache_page(sg_page(sg));
...
The uses in fs/exec.c are the most straight forward and can simply rely on the
kunmap() code to replace the call.
In conclusion I don't see a lot of reason to not define kunmap_flush_on_unmap()
on arm, csky, mips, nds32, and sh... Then remove all the
flush_kernel_dcache_page() call sites and the documentation...
Something like [2] below... Completely untested of course...
Ira
[1] commit 3d77b50c5874b7e923be946ba793644f82336b75
Author: Ming Lei
Date: Thu Oct 31 16:34:17 2013 -0700
lib/scatterlist.c: don't flush_kernel_dcache_page on slab page
Commit b1adaf65ba03 ("[SCSI] block: add sg buffer copy helper
functions") introduces two sg buffer copy helpers, and calls
flush_kernel_dcache_page() on pages in SG list after these pages are
written to.
Unfortunately, the commit may introduce a potential bug:
- Before sending some SCSI commands, kmalloc() buffer may be passed to
block layper, so flush_kernel_dcache_page() can see a slab page
finally
- According to cachetlb.txt, flush_kernel_dcache_page() is only called
on "a user page", which surely can't be a slab page.
- ARCH's implementation of flush_kernel_dcache_page() may use page
mapping information to do optimization so page_mapping() will see the
slab page, then VM_BUG_ON() is triggered.
Aaro Koskinen reported the bug on ARM/kirkwood when DEBUG_VM is enabled,
and this patch fixes the bug by adding test of '!PageSlab(miter->page)'
before calling flush_kernel_dcache_page().
[2]
>From 70b537c31d16c2a5e4e92c35895e8c59303bcbef Mon Sep 17 00:00:00 2001
From: Ira Weiny
Date: Fri, 11 Jun 2021 18:24:27 -0700
Subject: [PATCH] COMPLETELY UNTESTED: highmem: Remove direct calls to
flush_kernel_dcache_page
When to call flush_kernel_dcache_page() is confusing and inconsistent. For
architectures which may need to do something the core kmap code should be
leveraged to handle this when direct kernel access is needed.
Like parisc define kunmap_flush_on_unmap() to be called when pages are
unmapped on arm, csky, mpis, nds32, and sh.
Remove all direct calls to flush_kernel_dcache_page() and let the
kunmap() code do this for the users.
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-c...@vger.kernel.org
Cc: linux-m...@vger.kernel.org
Cc: linux...@vger.kernel.org
Cc: linux-cry...@vger.kernel.org
Cc: linux-...@vger.kernel.org
Cc: linux-s...@vger.kernel.org
Cc: linux-fsde...@vger.kernel.org
Signed-off-by: Ira Weiny
---
Documentation/core-api/cachetlb.rst | 13 -
arch/arm/include/asm/cacheflush.h| 6 ++
arch/csky/abiv1/inc/abi/cacheflush.h | 6 ++
arch/mips/include/asm/cacheflush.h | 6 ++
arch/nds32/include/asm/cacheflush.h | 6 ++
arch/sh/include/asm/cacheflush.h | 6 ++
drivers/crypto/omap-crypto.c | 3 ---
drivers/mmc/host/mmc_spi.c | 3 ---
drivers/scsi/aacraid/aachba.c| 1 -
fs/exec.c| 3 -
