diff mbox

[v3,08/14] x86, dax, libnvdimm: move wb_cache_pmem() to libnvdimm

Message ID 149703986971.20620.10303247412197996310.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
State Not Applicable, archived
Delegated to: Mike Snitzer
Headers show

Commit Message

Dan Williams June 9, 2017, 8:24 p.m. UTC
With all calls to this routine re-directed through the pmem driver, we can kill
the pmem api indirection. arch_wb_cache_pmem() is now optionally supplied by
the arch specific asm/pmem.h.  Same as before, pmem flushing is only defined
for x86_64, but it is straightforward to add other archs in the future.

Cc: <x86@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Jeff Moyer <jmoyer@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Matthew Wilcox <mawilcox@microsoft.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/include/asm/pmem.h       |   18 +-----------------
 arch/x86/include/asm/uaccess_64.h |    1 +
 arch/x86/lib/usercopy_64.c        |    3 ++-
 drivers/nvdimm/pmem.c             |    2 +-
 drivers/nvdimm/pmem.h             |    7 +++++++
 include/linux/pmem.h              |   19 -------------------
 6 files changed, 12 insertions(+), 38 deletions(-)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Jan Kara June 14, 2017, 10:54 a.m. UTC | #1
On Fri 09-06-17 13:24:29, Dan Williams wrote:
> With all calls to this routine re-directed through the pmem driver, we can kill
> the pmem api indirection. arch_wb_cache_pmem() is now optionally supplied by
> the arch specific asm/pmem.h.  Same as before, pmem flushing is only defined
> for x86_64, but it is straightforward to add other archs in the future.
> 
> Cc: <x86@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Oliver O'Halloran <oohall@gmail.com>
> Cc: Matthew Wilcox <mawilcox@microsoft.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Looks good to me. Just one question below...

> -/**
> - * arch_wb_cache_pmem - write back a cache range with CLWB
> - * @vaddr:	virtual start address
> - * @size:	number of bytes to write back
> - *
> - * Write back a cache range using the CLWB (cache line write back)
> - * 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)
>  {
> -	u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
> -	unsigned long clflush_mask = x86_clflush_size - 1;
> -	void *vend = addr + size;
> -	void *p;
> -
> -	for (p = (void *)((unsigned long)addr & ~clflush_mask);
> -	     p < vend; p += x86_clflush_size)
> -		clwb(p);
> +	clean_cache_range(addr,size);
>  }

So this will make compilation break on 32-bit x86 as it does not define
clean_cache_range(). Do we somewhere force we are on x86_64 when pmem is
enabled?

								Honza
Dan Williams June 14, 2017, 4:49 p.m. UTC | #2
On Wed, Jun 14, 2017 at 3:54 AM, Jan Kara <jack@suse.cz> wrote:
> On Fri 09-06-17 13:24:29, Dan Williams wrote:
>> With all calls to this routine re-directed through the pmem driver, we can kill
>> the pmem api indirection. arch_wb_cache_pmem() is now optionally supplied by
>> the arch specific asm/pmem.h.  Same as before, pmem flushing is only defined
>> for x86_64, but it is straightforward to add other archs in the future.
>>
>> Cc: <x86@kernel.org>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Jeff Moyer <jmoyer@redhat.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Oliver O'Halloran <oohall@gmail.com>
>> Cc: Matthew Wilcox <mawilcox@microsoft.com>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Looks good to me. Just one question below...
>
>> -/**
>> - * arch_wb_cache_pmem - write back a cache range with CLWB
>> - * @vaddr:   virtual start address
>> - * @size:    number of bytes to write back
>> - *
>> - * Write back a cache range using the CLWB (cache line write back)
>> - * 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)
>>  {
>> -     u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
>> -     unsigned long clflush_mask = x86_clflush_size - 1;
>> -     void *vend = addr + size;
>> -     void *p;
>> -
>> -     for (p = (void *)((unsigned long)addr & ~clflush_mask);
>> -          p < vend; p += x86_clflush_size)
>> -             clwb(p);
>> +     clean_cache_range(addr,size);
>>  }
>
> So this will make compilation break on 32-bit x86 as it does not define
> clean_cache_range(). Do we somewhere force we are on x86_64 when pmem is
> enabled?

Yes, this is enforced by:

    select ARCH_HAS_PMEM_API if X86_64

...in arch/x86/Kconfig. We fallback to a dummy arch_wb_cache_pmem()
implementation and emit this warning for !ARCH_HAS_PMEM_API archs:

    "nd_pmem namespace0.0: unable to guarantee persistence of writes"

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Jan Kara June 15, 2017, 8:11 a.m. UTC | #3
On Wed 14-06-17 09:49:29, Dan Williams wrote:
> On Wed, Jun 14, 2017 at 3:54 AM, Jan Kara <jack@suse.cz> wrote:
> >> -/**
> >> - * arch_wb_cache_pmem - write back a cache range with CLWB
> >> - * @vaddr:   virtual start address
> >> - * @size:    number of bytes to write back
> >> - *
> >> - * Write back a cache range using the CLWB (cache line write back)
> >> - * 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)
> >>  {
> >> -     u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
> >> -     unsigned long clflush_mask = x86_clflush_size - 1;
> >> -     void *vend = addr + size;
> >> -     void *p;
> >> -
> >> -     for (p = (void *)((unsigned long)addr & ~clflush_mask);
> >> -          p < vend; p += x86_clflush_size)
> >> -             clwb(p);
> >> +     clean_cache_range(addr,size);
> >>  }
> >
> > So this will make compilation break on 32-bit x86 as it does not define
> > clean_cache_range(). Do we somewhere force we are on x86_64 when pmem is
> > enabled?
> 
> Yes, this is enforced by:
> 
>     select ARCH_HAS_PMEM_API if X86_64
> 
> ...in arch/x86/Kconfig. We fallback to a dummy arch_wb_cache_pmem()
> implementation and emit this warning for !ARCH_HAS_PMEM_API archs:
> 
>     "nd_pmem namespace0.0: unable to guarantee persistence of writes"

Aha, right. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

							Honza
Christoph Hellwig June 18, 2017, 8:40 a.m. UTC | #4
> +void clean_cache_range(void *addr, size_t size);
>  
>  static inline int
>  __copy_from_user_inatomic_nocache(void *dst, const void __user *src,
> diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
> index f42d2fd86ca3..baa80ff29da8 100644
> --- a/arch/x86/lib/usercopy_64.c
> +++ b/arch/x86/lib/usercopy_64.c
> @@ -85,7 +85,7 @@ copy_user_handle_tail(char *to, char *from, unsigned len)
>   * instruction. Note that @size is internally rounded up to be cache
>   * line size aligned.
>   */
> -static void clean_cache_range(void *addr, size_t size)
> +void clean_cache_range(void *addr, size_t size)

Can you keep clean_cache_range private please?  Just add
arch_wb_cache_pmem to usercopy_64.c just behind it so that the
compiler can tail-call and export that instead.

> --- a/drivers/nvdimm/pmem.h
> +++ b/drivers/nvdimm/pmem.h
> @@ -4,6 +4,13 @@
>  #include <linux/types.h>
>  #include <linux/pfn_t.h>
>  #include <linux/fs.h>
> +#include <asm/pmem.h>
> +
> +#ifndef CONFIG_ARCH_HAS_PMEM_API
> +static inline void arch_wb_cache_pmem(void *addr, size_t size)
> +{
> +}
> +#endif

And our normal Linux style would be to have this linux linux/pmem.h,
which should always be included for the asm version.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Dan Williams June 19, 2017, 2:06 a.m. UTC | #5
On Sun, Jun 18, 2017 at 1:40 AM, Christoph Hellwig <hch@lst.de> wrote:
>> +void clean_cache_range(void *addr, size_t size);
>>
>>  static inline int
>>  __copy_from_user_inatomic_nocache(void *dst, const void __user *src,
>> diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
>> index f42d2fd86ca3..baa80ff29da8 100644
>> --- a/arch/x86/lib/usercopy_64.c
>> +++ b/arch/x86/lib/usercopy_64.c
>> @@ -85,7 +85,7 @@ copy_user_handle_tail(char *to, char *from, unsigned len)
>>   * instruction. Note that @size is internally rounded up to be cache
>>   * line size aligned.
>>   */
>> -static void clean_cache_range(void *addr, size_t size)
>> +void clean_cache_range(void *addr, size_t size)
>
> Can you keep clean_cache_range private please?  Just add
> arch_wb_cache_pmem to usercopy_64.c just behind it so that the
> compiler can tail-call and export that instead.
>
>> --- a/drivers/nvdimm/pmem.h
>> +++ b/drivers/nvdimm/pmem.h
>> @@ -4,6 +4,13 @@
>>  #include <linux/types.h>
>>  #include <linux/pfn_t.h>
>>  #include <linux/fs.h>
>> +#include <asm/pmem.h>
>> +
>> +#ifndef CONFIG_ARCH_HAS_PMEM_API
>> +static inline void arch_wb_cache_pmem(void *addr, size_t size)
>> +{
>> +}
>> +#endif
>
> And our normal Linux style would be to have this linux linux/pmem.h,
> which should always be included for the asm version.

Ok, will do.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
index f4c119d253f3..862be3a9275c 100644
--- a/arch/x86/include/asm/pmem.h
+++ b/arch/x86/include/asm/pmem.h
@@ -44,25 +44,9 @@  static inline void arch_memcpy_to_pmem(void *dst, const void *src, size_t n)
 		BUG();
 }
 
-/**
- * arch_wb_cache_pmem - write back a cache range with CLWB
- * @vaddr:	virtual start address
- * @size:	number of bytes to write back
- *
- * Write back a cache range using the CLWB (cache line write back)
- * 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)
 {
-	u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
-	unsigned long clflush_mask = x86_clflush_size - 1;
-	void *vend = addr + size;
-	void *p;
-
-	for (p = (void *)((unsigned long)addr & ~clflush_mask);
-	     p < vend; p += x86_clflush_size)
-		clwb(p);
+	clean_cache_range(addr,size);
 }
 
 static inline void arch_invalidate_pmem(void *addr, size_t size)
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index b16f6a1d8b26..bdc4a2761525 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -174,6 +174,7 @@  extern long __copy_user_nocache(void *dst, const void __user *src,
 extern long __copy_user_flushcache(void *dst, const void __user *src, unsigned size);
 extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
 			   size_t len);
+void clean_cache_range(void *addr, size_t size);
 
 static inline int
 __copy_from_user_inatomic_nocache(void *dst, const void __user *src,
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index f42d2fd86ca3..baa80ff29da8 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -85,7 +85,7 @@  copy_user_handle_tail(char *to, char *from, unsigned len)
  * instruction. Note that @size is internally rounded up to be cache
  * line size aligned.
  */
-static void clean_cache_range(void *addr, size_t size)
+void clean_cache_range(void *addr, size_t size)
 {
 	u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
 	unsigned long clflush_mask = x86_clflush_size - 1;
@@ -96,6 +96,7 @@  static void clean_cache_range(void *addr, size_t size)
 	     p < vend; p += x86_clflush_size)
 		clwb(p);
 }
+EXPORT_SYMBOL(clean_cache_range);
 
 long __copy_user_flushcache(void *dst, const void __user *src, unsigned size)
 {
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 823b07774244..3b87702d46bb 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -245,7 +245,7 @@  static size_t pmem_copy_from_iter(struct dax_device *dax_dev, pgoff_t pgoff,
 static void pmem_dax_flush(struct dax_device *dax_dev, pgoff_t pgoff,
 		void *addr, size_t size)
 {
-	wb_cache_pmem(addr, size);
+	arch_wb_cache_pmem(addr, size);
 }
 
 static const struct dax_operations pmem_dax_ops = {
diff --git a/drivers/nvdimm/pmem.h b/drivers/nvdimm/pmem.h
index 7f4dbd72a90a..9137ec80b85f 100644
--- a/drivers/nvdimm/pmem.h
+++ b/drivers/nvdimm/pmem.h
@@ -4,6 +4,13 @@ 
 #include <linux/types.h>
 #include <linux/pfn_t.h>
 #include <linux/fs.h>
+#include <asm/pmem.h>
+
+#ifndef CONFIG_ARCH_HAS_PMEM_API
+static inline void arch_wb_cache_pmem(void *addr, size_t size)
+{
+}
+#endif
 
 /* this definition is in it's own header for tools/testing/nvdimm to consume */
 struct pmem_device {
diff --git a/include/linux/pmem.h b/include/linux/pmem.h
index 772bd02a5b52..33ae761f010a 100644
--- a/include/linux/pmem.h
+++ b/include/linux/pmem.h
@@ -31,11 +31,6 @@  static inline void arch_memcpy_to_pmem(void *dst, const void *src, size_t n)
 	BUG();
 }
 
-static inline void arch_wb_cache_pmem(void *addr, size_t size)
-{
-	BUG();
-}
-
 static inline void arch_invalidate_pmem(void *addr, size_t size)
 {
 	BUG();
@@ -80,18 +75,4 @@  static inline void invalidate_pmem(void *addr, size_t size)
 	if (arch_has_pmem_api())
 		arch_invalidate_pmem(addr, size);
 }
-
-/**
- * wb_cache_pmem - write back processor cache for PMEM memory range
- * @addr:	virtual start address
- * @size:	number of bytes to write back
- *
- * Write back the processor cache range starting at 'addr' for 'size' bytes.
- * See blkdev_issue_flush() note for memcpy_to_pmem().
- */
-static inline void wb_cache_pmem(void *addr, size_t size)
-{
-	if (arch_has_pmem_api())
-		arch_wb_cache_pmem(addr, size);
-}
 #endif /* __PMEM_H__ */