Before we rework the "pmem api" to stop abusing __copy_user_nocache()
for memcpy_to_pmem() we need to fix cases where we may strand dirty data
in the cpu cache. The problem occurs when copy_from_iter_pmem() is used
for arbitrary data transfers from userspace. There is no guarantee that
these transfers, performed by dax_iomap_actor(), will have aligned
destinations or aligned transfer lengths. Backstop the usage
__copy_user_nocache() with explicit cache management in these unaligned
cases.
Yes, copy_from_iter_pmem() is now too big for an inline, but addressing
that is saved for a later patch that moves the entirety of the "pmem
api" into the pmem driver directly.
Fixes: 5de490daec8b ("pmem: add copy_from_iter_pmem() and clear_pmem()")
Cc: <[email protected]>
Cc: <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Jeff Moyer <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Ross Zwisler <[email protected]>
Signed-off-by: Toshi Kani <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Changes in v3:
* match the implementation to the notes at the top of
__copy_user_nocache (Toshi)
* Switch to using the IS_ALIGNED() macro to make alignment checks easier
to read and harder to get wrong like they were in v2. (Toshi, Dan)
arch/x86/include/asm/pmem.h | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
index 2c1ebeb4d737..529bb4a6487a 100644
--- a/arch/x86/include/asm/pmem.h
+++ b/arch/x86/include/asm/pmem.h
@@ -55,7 +55,8 @@ static inline int arch_memcpy_from_pmem(void *dst, const void *src, size_t n)
* @size: number of bytes to write back
*
* Write back a cache range using the CLWB (cache line write back)
- * instruction.
+ * instruction. Note that @size is internally rounded up to be cache
+ * line size aligned.
*/
static inline void arch_wb_cache_pmem(void *addr, size_t size)
{
@@ -69,15 +70,6 @@ static inline void arch_wb_cache_pmem(void *addr, size_t size)
clwb(p);
}
-/*
- * copy_from_iter_nocache() on x86 only uses non-temporal stores for iovec
- * iterators, so for other types (bvec & kvec) we must do a cache write-back.
- */
-static inline bool __iter_needs_pmem_wb(struct iov_iter *i)
-{
- return iter_is_iovec(i) == false;
-}
-
/**
* arch_copy_from_iter_pmem - copy data from an iterator to PMEM
* @addr: PMEM destination address
@@ -94,7 +86,35 @@ static inline size_t arch_copy_from_iter_pmem(void *addr, size_t bytes,
/* TODO: skip the write-back by always using non-temporal stores */
len = copy_from_iter_nocache(addr, bytes, i);
- if (__iter_needs_pmem_wb(i))
+ /*
+ * In the iovec case on x86_64 copy_from_iter_nocache() uses
+ * non-temporal stores for the bulk of the transfer, but we need
+ * to manually flush if the transfer is unaligned. A cached
+ * memory copy is used when destination or size is not naturally
+ * aligned. That is:
+ * - Require 8-byte alignment when size is 8 bytes or larger.
+ * - Require 4-byte alignment when size is 4 bytes.
+ *
+ * In the non-iovec case the entire destination needs to be
+ * flushed.
+ */
+ if (iter_is_iovec(i)) {
+ unsigned long flushed, dest = (unsigned long) addr;
+
+ if (bytes < 8) {
+ if (!IS_ALIGNED(dest, 4) || (bytes != 4))
+ arch_wb_cache_pmem(addr, 1);
+ } else {
+ if (!IS_ALIGNED(dest, 8)) {
+ dest = ALIGN(dest, boot_cpu_data.x86_clflush_size);
+ arch_wb_cache_pmem(addr, 1);
+ }
+
+ flushed = dest - (unsigned long) addr;
+ if (bytes > flushed && !IS_ALIGNED(bytes - flushed, 8))
+ arch_wb_cache_pmem(addr + bytes - 1, 1);
+ }
+ } else
arch_wb_cache_pmem(addr, bytes);
return len;
On Mon, 2017-04-10 at 17:35 -0700, Dan Williams wrote:
> Before we rework the "pmem api" to stop abusing __copy_user_nocache()
> for memcpy_to_pmem() we need to fix cases where we may strand dirty
> data in the cpu cache. The problem occurs when copy_from_iter_pmem()
> is used for arbitrary data transfers from userspace. There is no
> guarantee that these transfers, performed by dax_iomap_actor(), will
> have aligned destinations or aligned transfer lengths. Backstop the
> usage __copy_user_nocache() with explicit cache management in these
> unaligned cases.
>
> Yes, copy_from_iter_pmem() is now too big for an inline, but
> addressing that is saved for a later patch that moves the entirety of
> the "pmem api" into the pmem driver directly.
>
> Fixes: 5de490daec8b ("pmem: add copy_from_iter_pmem() and
> clear_pmem()")
> Cc: <[email protected]>
> Cc: <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Jeff Moyer <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Ross Zwisler <[email protected]>
> Signed-off-by: Toshi Kani <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Changes in v3:
> * match the implementation to the notes at the top of
> __copy_user_nocache (Toshi)
>
> * Switch to using the IS_ALIGNED() macro to make alignment checks
> easier to read and harder to get wrong like they were in v2. (Toshi,
> Dan)
Thanks Dan! It looks good.
-Toshi
On Mon, Apr 10, 2017 at 05:35:02PM -0700, Dan Williams wrote:
> Before we rework the "pmem api" to stop abusing __copy_user_nocache()
> for memcpy_to_pmem() we need to fix cases where we may strand dirty data
> in the cpu cache. The problem occurs when copy_from_iter_pmem() is used
> for arbitrary data transfers from userspace. There is no guarantee that
> these transfers, performed by dax_iomap_actor(), will have aligned
> destinations or aligned transfer lengths. Backstop the usage
> __copy_user_nocache() with explicit cache management in these unaligned
> cases.
>
> Yes, copy_from_iter_pmem() is now too big for an inline, but addressing
> that is saved for a later patch that moves the entirety of the "pmem
> api" into the pmem driver directly.
>
> Fixes: 5de490daec8b ("pmem: add copy_from_iter_pmem() and clear_pmem()")
> Cc: <[email protected]>
> Cc: <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Jeff Moyer <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Ross Zwisler <[email protected]>
> Signed-off-by: Toshi Kani <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
This looks good to me, thanks for fixing this.
Reviewed-by: Ross Zwisler <[email protected]>