From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:06:28
Hi all,
this series switches the core block layer code and all users of the
existing bvec kmap helpers to use kmap_local_page. Drivers that
currently use open coded kmap_atomic calls will converted in a follow
on series.
Diffstat:
arch/mips/include/asm/mach-rc32434/rb.h | 2 -
block/bio-integrity.c | 14 ++++------
block/bio.c | 37 +++++++---------------------
block/blk-map.c | 2 -
block/bounce.c | 35 ++++++--------------------
block/t10-pi.c | 16 ++++--------
drivers/block/ps3disk.c | 19 ++------------
drivers/block/rbd.c | 15 +----------
drivers/md/dm-writecache.c | 5 +--
include/linux/bio.h | 42 --------------------------------
include/linux/bvec.h | 27 ++++++++++++++++++--
include/linux/highmem.h | 4 +--
12 files changed, 64 insertions(+), 154 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:06:32
There is no need to include genhd.h from a random arch header, and not
doing so prevents the possibility for nasty include loops.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/mips/include/asm/mach-rc32434/rb.h | 2 --
1 file changed, 2 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:06:34
Fix the include guards to match the file naming.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/bvec.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:06:37
Add a helper to call kmap_local_page on a bvec. There is no need for
an unmap helper given that kunmap_local accept any address in the mapped
page.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/bvec.h | 6 ++++++
1 file changed, 6 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:06:44
Add helpers to perform common memory operation on a bvec.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/bvec.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:06:53
Use memzero_bvec to zero each segment in the bio instead of manually
mapping and zeroing the data.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bio.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:07:36
There is no need to disable interrupts in bio_copy_block, and the local
only mappings helps to avoid any sort of problems with stray writes
into the bio data.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/md/dm-writecache.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:07:39
Use the bvec helpers instead of open coding the copy.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/block/ps3disk.c | 19 +++----------------
1 file changed, 3 insertions(+), 16 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:07:44
Use the proper helpers instead of open coding the copy.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bio.c | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)
@@ -5,7 +5,6 @@#ifndef __LINUX_BIO_H#define __LINUX_BIO_H-#include<linux/highmem.h>#include<linux/mempool.h>#include<linux/ioprio.h>/* struct bio, bio_vec and BIO_* flags are defined in blk_types.h */
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:08:27
Use memcpy_from_bvec instead of open coding the logic.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:08:30
Rewrite the actual bounce buffering loop in __blk_queue_bounce to that
the memcpy_to_bvec helper can be used to perform the data copies.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bounce.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:09:14
Using local kmaps slightly reduces the chances to stray writes, and
the bvec interface cleans up the code a little bit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bio-integrity.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:09:16
Using local kmaps slightly reduces the chances to stray writes, and
the bvec interface cleans up the code a little bit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/t10-pi.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
From: Bart Van Assche <bvanassche@acm.org> Date: 2021-06-08 16:23:55
On 6/8/21 9:05 AM, Christoph Hellwig wrote:
quoted hunk
There is no need to include genhd.h from a random arch header, and not
doing so prevents the possibility for nasty include loops.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/mips/include/asm/mach-rc32434/rb.h | 2 --
1 file changed, 2 deletions(-)
From one of the functions called by kunmap_local():
unsigned long addr = (unsigned long) vaddr & PAGE_MASK;
This won't work well if bvec->bv_offset >= PAGE_SIZE I assume?
Thanks,
Bart.
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-08 16:38:08
On Tue, Jun 08, 2021 at 09:30:56AM -0700, Bart Van Assche wrote:
quoted
From one of the functions called by kunmap_local():
unsigned long addr = (unsigned long) vaddr & PAGE_MASK;
This won't work well if bvec->bv_offset >= PAGE_SIZE I assume?
It won't indeed. Both the existing and new helpers operate on single
page bvecs only, and all callers only use those. I should have
probably mentioned that in the cover letter and documented the
assumptions in the code, though.
Add a helper to call kmap_local_page on a bvec. There is no need for
an unmap helper given that kunmap_local accept any address in the mapped
page.
Signed-off-by: Christoph Hellwig <hch@lst.de>
From: Ira Weiny <hidden> Date: 2021-06-09 01:48:31
On Tue, Jun 08, 2021 at 06:05:56PM +0200, Christoph Hellwig wrote:
quoted hunk
rq_for_each_segment(bvec, req, iter) {- unsigned long flags;- dev_dbg(&dev->sbd.core, "%s:%u: bio %u: %u sectors from %llu\n",- __func__, __LINE__, i, bio_sectors(iter.bio),- iter.bio->bi_iter.bi_sector);-- size = bvec.bv_len;- buf = bvec_kmap_irq(&bvec, &flags); if (gather)- memcpy(dev->bounce_buf+offset, buf, size);+ memcpy_from_bvec(dev->bounce_buf + offset, &bvec); else- memcpy(buf, dev->bounce_buf+offset, size);- offset += size;- flush_kernel_dcache_page(bvec.bv_page);
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.
Ira
From: Ira Weiny <hidden> Date: 2021-06-09 01:58:22
On Tue, Jun 08, 2021 at 06:06:01PM +0200, Christoph Hellwig wrote:
quoted hunk
Rewrite the actual bounce buffering loop in __blk_queue_bounce to that
the memcpy_to_bvec helper can be used to perform the data copies.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/bounce.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
NIT: the fact that the copy is from 'to' makes my head hurt... But I don't
see a good way to change that without declaring unnecessary variables... :-(
The logic seems right.
Ira
From: Ira Weiny <hidden> Date: 2021-06-09 01:59:47
On Tue, Jun 08, 2021 at 06:05:47PM +0200, Christoph Hellwig wrote:
Hi all,
this series switches the core block layer code and all users of the
existing bvec kmap helpers to use kmap_local_page. Drivers that
currently use open coded kmap_atomic calls will converted in a follow
on series.
Other than the missing flush_dcache's.
For the series.
Reviewed-by: Ira Weiny <redacted>
Ira already brought up the fact that this conversion drops
flush_dcache_page() calls throughout. Other than that:
Acked-by: Ilya Dryomov <idryomov@gmail.com>
Thanks,
Ilya
On Tue, Jun 8, 2021 at 6:06 PM Christoph Hellwig [off-list ref] wrote:
quoted hunk
Add a helper to call kmap_local_page on a bvec. There is no need for
an unmap helper given that kunmap_local accept any address in the mapped
page.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/bvec.h | 6 ++++++
1 file changed, 6 insertions(+)
Might be useful to add the second sentence of the commit message as
a comment for bvec_kmap_local(). It could be expanded to mention the
single-page bvec caveat too.
Thanks,
Ilya
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-11 06:53:46
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?
From: Ira Weiny <hidden> Date: 2021-06-12 04:07:57
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:
quoted
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 [off-list ref]
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 <redacted>
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-kernel@lists.infradead.org
Cc: linux-csky@vger.kernel.org
Cc: linux-mips@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-mmc@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Ira Weiny <redacted>
---
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 ---
include/linux/highmem.h | 3 ---
lib/scatterlist.c | 4 ----
12 files changed, 30 insertions(+), 30 deletions(-)
@@ -351,19 +351,6 @@ maps this page at its virtual address. architectures). For incoherent architectures, it should flush the cache of the page at vmaddr.-``void flush_kernel_dcache_page(struct page *page)``-- When the kernel needs to modify a user page is has obtained- with kmap, it calls this function after all modifications are- complete (but before kunmapping it) to bring the underlying- page up to date. It is assumed here that the user has no- incoherent cached copies (i.e. the original page was obtained- from a mechanism like get_user_pages()). The default- implementation is a nop and should remain so on all coherent- architectures. On incoherent architectures, this should flush- the kernel cache for page (using page_address(page)).--``void flush_icache_range(unsigned long start, unsigned long end)`` When the kernel stores into addresses that it will execute
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2021-06-15 05:04:22
On Fri, Jun 11, 2021 at 09:07:43PM -0700, Ira Weiny wrote:
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.
Probably because the generic Crypto API has the same check. This
all goes back to
commit 4f3e797ad07d52d34983354a77b365dfcd48c1b4
Author: Herbert Xu [off-list ref]
Date: Mon Feb 9 14:22:14 2009 +1100
crypto: scatterwalk - Avoid flush_dcache_page on slab pages
It's illegal to call flush_dcache_page on slab pages on a number
of architectures. So this patch avoids doing so if PageSlab is
true.
In future we can move the flush_dcache_page call to those page
cache users that actually need it.
Reported-by: David S. Miller [off-list ref]
Signed-off-by: Herbert Xu [off-list ref]
But I can't find any emails discussing this so let me ask Dave
directly and see if he can tell us what the issue was or might
have been.
Thanks,
--
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt