dm-writecache patches

I use memcpy_flushcache in my persistent memory driver for metadata updates and it turns out that the overhead of memcpy_flushcache causes 2% performance degradation compared to "movnti" instruction explicitly coded using inline assembler.

This patch recognizes memcpy_flushcache calls with constant short length and turns them into inline assembler - so that I don't have to use inline assembler in the driver.

Signed-off-by Mikulas Patocka <[email protected]>

---
 arch/x86/include/asm/string_64.h |   20 +++++++++++++++++++-
 arch/x86/lib/usercopy_64.c       |    4 ++--
 2 files changed, 21 insertions(+), 3 deletions(-)

Index: linux-2.6/arch/x86/include/asm/string_64.h
===================================================================

arch/x86/include/asm/string_64.h 2018-05-18 21:21:15.000000000 +0200 renamed from arch/x86/include/asm/string_64.h 2018-05-18 21:21:15.000000000 +0200 +19 -1

@@ -147,7 +147,25 @@ memcpy_mcsafe(void *dst, const void *src
 
 #ifdef CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE
 #define __HAVE_ARCH_MEMCPY_FLUSHCACHE 1
-void memcpy_flushcache(void *dst, const void *src, size_t cnt);
+void __memcpy_flushcache(void *dst, const void *src, size_t cnt);
+static __always_inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
+{
+       if (__builtin_constant_p(cnt)) {
+               switch (cnt) {
+                       case 4:
+                               asm ("movntil %1, %0" : "=m"(*(u32 *)dst) : "r"(*(u32 *)src));
+                               return;
+                       case 8:
+                               asm ("movntiq %1, %0" : "=m"(*(u64 *)dst) : "r"(*(u64 *)src));
+                               return;
+                       case 16:
+                               asm ("movntiq %1, %0" : "=m"(*(u64 *)dst) : "r"(*(u64 *)src));
+                               asm ("movntiq %1, %0" : "=m"(*(u64 *)(dst + 8)) : "r"(*(u64 *)(src + 8)));
+                               return;
+               }
+       }
+       __memcpy_flushcache(dst, src, cnt);
+}
 #endif
 
 #endif /* __KERNEL__ */

Index: linux-2.6/arch/x86/lib/usercopy_64.c
===================================================================

arch/x86/lib/usercopy_64.c 2018-05-18 22:09:49.000000000 +0200 renamed from arch/x86/lib/usercopy_64.c 2018-05-18 21:21:15.000000000 +0200 +2 -2

@@ -133,7 +133,7 @@ long __copy_user_flushcache(void *dst, c
        return rc;
 }
 
-void memcpy_flushcache(void *_dst, const void *_src, size_t size)
+void __memcpy_flushcache(void *_dst, const void *_src, size_t size)
 {
        unsigned long dest = (unsigned long) _dst;
        unsigned long source = (unsigned long) _src;

@@ -196,7 +196,7 @@ void memcpy_flushcache(void *_dst, const
                clean_cache_range((void *) dest, size);
        }
 }
-EXPORT_SYMBOL_GPL(memcpy_flushcache);
+EXPORT_SYMBOL_GPL(__memcpy_flushcache);
 
 void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
                size_t len)

From: Mikulas Patocka <[email protected]>
Subject: [PATCH v2] x86: optimize memcpy_flushcache

In the context of constant short length stores to persistent memory, memcpy_flushcache suffers from a 2% performance degradation compared to explicitly using the "movnti" instruction.

Optimize 4, 8, and 16 byte memcpy_flushcache calls to explicitly use the movnti instruction with inline assembler.

Quoted 9 lines · Mike Snitzer

From: Mikulas Patocka <[email protected]>
Subject: [PATCH v2] x86: optimize memcpy_flushcache

In the context of constant short length stores to persistent memory, memcpy_flushcache suffers from a 2% performance degradation compared to explicitly using the "movnti" instruction.

Optimize 4, 8, and 16 byte memcpy_flushcache calls to explicitly use the movnti instruction with inline assembler.

Linus requested asm optimizations to include actual benchmarks, so it would be nice to describe how this was tested, on what hardware, and what the before/after numbers are.

Thanks,

Ingo

Quoted 19 lines · Ingo Molnar

Quoted 9 lines · Mike Snitzer

From: Mikulas Patocka <[email protected]>
Subject: [PATCH v2] x86: optimize memcpy_flushcache

In the context of constant short length stores to persistent memory, memcpy_flushcache suffers from a 2% performance degradation compared to explicitly using the "movnti" instruction.

Optimize 4, 8, and 16 byte memcpy_flushcache calls to explicitly use the movnti instruction with inline assembler.

Linus requested asm optimizations to include actual benchmarks, so it would be nice to describe how this was tested, on what hardware, and what the before/after numbers are.

Thanks,

Quoted 1 line

Ingo

It was tested on 4-core skylake machine with persistent memory being emulated using the memmap kernel option. The dm-writecache target used the emulated persistent memory as a cache and sata SSD as a backing device. The patch results in 2% improved throughput when writing data using dd.

I don't have access to the machine anymore.

Quoted 21 lines · Mikulas Patocka

Quoted 19 lines · Ingo Molnar

Quoted 9 lines · Mike Snitzer

From: Mikulas Patocka <[email protected]>
Subject: [PATCH v2] x86: optimize memcpy_flushcache

In the context of constant short length stores to persistent memory, memcpy_flushcache suffers from a 2% performance degradation compared to explicitly using the "movnti" instruction.

Optimize 4, 8, and 16 byte memcpy_flushcache calls to explicitly use the movnti instruction with inline assembler.

Linus requested asm optimizations to include actual benchmarks, so it would be nice to describe how this was tested, on what hardware, and what the before/after numbers are.

Thanks,

Quoted 8 lines

Quoted 1 line

Ingo

It was tested on 4-core skylake machine with persistent memory being emulated using the memmap kernel option. The dm-writecache target used the emulated persistent memory as a cache and sata SSD as a backing device. The patch results in 2% improved throughput when writing data using dd.

I don't have access to the machine anymore.

I think this information is enough, but do we know how well memmap emulation represents true persistent memory speed and cache management characteristics? It might be representative - but I don't know for sure, nor probably most readers of the changelog.

So could you please put all this into an updated changelog, and also add a short description that outlines exactly which codepaths end up using this method in a typical persistent memory setup? All filesystem ops - or only reads, etc?

Thanks,

Ingo

Here I resend it:

From: Mikulas Patocka <[email protected]>
Subject: [PATCH] x86: optimize memcpy_flushcache

I use memcpy_flushcache in my persistent memory driver for metadata updates, there are many 8-byte and 16-byte updates and it turns out that the overhead of memcpy_flushcache causes 2% performance degradation compared to "movnti" instruction explicitly coded using inline assembler.

The tests were done on a Skylake processor with persistent memory emulated using the "memmap" kernel parameter. dd was used to copy data to the dm-writecache target.

This patch recognizes memcpy_flushcache calls with constant short length and turns them into inline assembler - so that I don't have to use inline assembler in the driver.

Signed-off-by Mikulas Patocka <[email protected]>

---
 arch/x86/include/asm/string_64.h |   20 +++++++++++++++++++-
 arch/x86/lib/usercopy_64.c       |    4 ++--
 2 files changed, 21 insertions(+), 3 deletions(-)

In order to reduce locking overhead, I use the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, I need to use unlocked functions __prepare_to_swait and __finish_swait. These functions are declared in the file include/linux/swait.h, but they are not exported, and so they are not useable from kernel modules.

Signed-off-by Mikulas Patocka <[email protected]>

---
 kernel/sched/swait.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/kernel/sched/swait.c
===================================================================

kernel/sched/swait.c +2 -1

@@ -75,6 +75,7 @@ void __prepare_to_swait(struct swait_que
 	if (list_empty(&wait->task_list))
 		list_add(&wait->task_list, &q->task_list);
 }
+EXPORT_SYMBOL(__prepare_to_swait);
 
 void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state)
 {

@@ -104,6 +105,7 @@ void __finish_swait(struct swait_queue_h
 	if (!list_empty(&wait->task_list))
 		list_del_init(&wait->task_list);
 }
+EXPORT_SYMBOL(__finish_swait);
 
 void finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 {
 
--

dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Quoted 6 lines · Mikulas Patocka

In order to reduce locking overhead, I use the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, I need to use unlocked functions __prepare_to_swait and __finish_swait. These functions are declared in the file include/linux/swait.h, but they are not exported, and so they are not useable from kernel modules.

Please CC the author and maintainers of the swait code.

My impression is that this is the wrong thing to do. The swait code is supposed to be simple and self contained, and if you want to do anything else use normal waitqueues.

Quoted 2 lines · Christoph Hellwig

Please CC the author and maintainers of the swait code.

Quoted 3 lines

My impression is that this is the wrong thing to do. The swait code is supposed to be simple and self contained, and if you want to do anything else use normal waitqueues.

You said the same thing last time around. I've since cc'd Peter and Thomas and haven't heard back, see:
https://www.redhat.com/archives/dm-devel/2018-May/msg00048.html

The entire point of exporting these symbols is to allow use of the "simple waitqueue" code to optimize -- without resorting to using normal waitqueues.

Quoted 9 lines · Mike Snitzer

Quoted 5 lines · Christoph Hellwig

Please CC the author and maintainers of the swait code.

My impression is that this is the wrong thing to do. The swait code is supposed to be simple and self contained, and if you want to do anything else use normal waitqueues.

Quoted 3 lines

You said the same thing last time around. I've since cc'd Peter and Thomas and haven't heard back, see:
https://www.redhat.com/archives/dm-devel/2018-May/msg00048.html

Yeah, sorry, got lost :/

Quoted 3 lines

The entire point of exporting these symbols is to allow use of the "simple waitqueue" code to optimize -- without resorting to using normal waitqueues.

So I don't immediately object to exporting them; however I do share some of hch's concerns. The reason swait exists is to be deterministic (for RT) -- something that regular wait code cannot be.

And by (ab)using / exporting the wait internal lock you risk loosing that. So I don't think the proposed usage is bad, it is possible to create badness.

So if we're going to export them; someone needs to keep an eye on things and ensure the lock isn't abused.

[Peter, in this v2 I switched to using _GPL for the exports and updated the patch header. As covered in previous mail, please let me know if you're OK with me staging this change for 4.18 via linux-dm.git with your Ack, thanks]

From: Mikulas Patocka <[email protected]>
Subject: [PATCH] swait: export symbols __prepare_to_swait and __finish_swait

__prepare_to_swait and __finish_swait are declared in include/linux/swait.h but they are not exported; so they are not useable from kernel modules.

A new consumer of swait (in dm-writecache) reduces its locking overhead by using the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, this swait consuming kernel module needs to use these unlocked functions.

Peter Zijlstra explained:
"The reason swait exists is to be deterministic (for RT) -- something that regular wait code cannot be.
And by (ab)using / exporting the wait internal lock you risk losing that. So I don't think the proposed [dm-writecache] usage is bad, it is possible to create badness.
So if we're going to export them; someone needs to keep an eye on things and ensure the lock isn't abused."

So while this new use of the wait internal lock doesn't jeopardize the realtime requirements of swait, these exports do open swait's internal locking up to being abused. As such, EXPORT_SYMBOL_GPL is used because any future consumers of __prepare_to_swait and __finish_swait must always be thoroughly scrutinized.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Mikulas Patocka <[email protected]>

Signed-off-by Mike Snitzer <[email protected]>

---
 kernel/sched/swait.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/sched/swait.c +2 -0

index b6fb2c3b3ff7..5d891b65ada5 100644
@@ -75,6 +75,7 @@ void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait)
 	if (list_empty(&wait->task_list))
 		list_add(&wait->task_list, &q->task_list);
 }
+EXPORT_SYMBOL_GPL(__prepare_to_swait);
 
 void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state)
 {

@@ -104,6 +105,7 @@ void __finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 	if (!list_empty(&wait->task_list))
 		list_del_init(&wait->task_list);
 }
+EXPORT_SYMBOL_GPL(__finish_swait);
 
 void finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 {

Signature · 1 line

2.15.0

Quoted 61 lines · Mike Snitzer

[Peter, in this v2 I switched to using _GPL for the exports and updated the patch header. As covered in previous mail, please let me know if you're OK with me staging this change for 4.18 via linux-dm.git with your Ack, thanks]

From: Mikulas Patocka <[email protected]>
Subject: [PATCH] swait: export symbols __prepare_to_swait and __finish_swait

__prepare_to_swait and __finish_swait are declared in include/linux/swait.h but they are not exported; so they are not useable from kernel modules.

A new consumer of swait (in dm-writecache) reduces its locking overhead by using the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, this swait consuming kernel module needs to use these unlocked functions.

Peter Zijlstra explained:
"The reason swait exists is to be deterministic (for RT) -- something that regular wait code cannot be.
And by (ab)using / exporting the wait internal lock you risk losing that. So I don't think the proposed [dm-writecache] usage is bad, it is possible to create badness.
So if we're going to export them; someone needs to keep an eye on things and ensure the lock isn't abused."

So while this new use of the wait internal lock doesn't jeopardize the realtime requirements of swait, these exports do open swait's internal locking up to being abused. As such, EXPORT_SYMBOL_GPL is used because any future consumers of __prepare_to_swait and __finish_swait must always be thoroughly scrutinized.

Cc Peter Zijlstra <[email protected]>

Signed-off-by Mikulas Patocka <[email protected]>

Signed-off-by Mike Snitzer <[email protected]>

---
 kernel/sched/swait.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/sched/swait.c +2 -0

index b6fb2c3b3ff7..5d891b65ada5 100644
@@ -75,6 +75,7 @@ void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait)
 	if (list_empty(&wait->task_list))
 		list_add(&wait->task_list, &q->task_list);
 }
+EXPORT_SYMBOL_GPL(__prepare_to_swait);
 
 void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state)
 {

@@ -104,6 +105,7 @@ void __finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 	if (!list_empty(&wait->task_list))
 		list_del_init(&wait->task_list);
 }
+EXPORT_SYMBOL_GPL(__finish_swait);
 
 void finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 {

Signature · 1 line

2.15.0

Then - you should export swake_up_locked with EXPORT_SYMBOL_GPL too.

Because swake_up_locked is unusable without __prepare_to_swait and __finish_swait.

[Peter, in this v2 I switched to using _GPL for the exports and updated the patch header. As covered in previous mail, please let me know if you're OK with me staging this change for 4.18 via linux-dm.git with your Ack, thanks]

From: Mikulas Patocka <[email protected]>
Subject: [PATCH] swait: export symbols __prepare_to_swait and __finish_swait

__prepare_to_swait and __finish_swait are declared in include/linux/swait.h but they are not exported; so they are not useable from kernel modules.

A new consumer of swait (in dm-writecache) reduces its locking overhead by using the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, this swait consuming kernel module needs to use these unlocked functions.

Peter Zijlstra explained:
"The reason swait exists is to be deterministic (for RT) -- something that regular wait code cannot be.
And by (ab)using / exporting the wait internal lock you risk losing that. So I don't think the proposed [dm-writecache] usage is bad, it is possible to create badness.
So if we're going to export them; someone needs to keep an eye on things and ensure the lock isn't abused."

So while this new use of the wait internal lock doesn't jeopardize the realtime requirements of swait, these exports do open swait's internal locking up to being abused. As such, EXPORT_SYMBOL_GPL is used because any future consumers of __prepare_to_swait and __finish_swait must always be thoroughly scrutinized.

Signed-off-by Mikulas Patocka <[email protected]>

Signed-off-by Mike Snitzer <[email protected]>

---
 kernel/sched/swait.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/sched/swait.c +2 -0

index b6fb2c3b3ff7..5d891b65ada5 100644
@@ -75,6 +75,7 @@ void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait)
 	if (list_empty(&wait->task_list))
 		list_add(&wait->task_list, &q->task_list);
 }
+EXPORT_SYMBOL_GPL(__prepare_to_swait);
 
 void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state)
 {

@@ -104,6 +105,7 @@ void __finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 	if (!list_empty(&wait->task_list))
 		list_del_init(&wait->task_list);
 }
+EXPORT_SYMBOL_GPL(__finish_swait);
 
 void finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
 {

Signature · 1 line

2.15.0

Quoted 26 lines · Mike Snitzer

[Peter, in this v2 I switched to using _GPL for the exports and updated the patch header. As covered in previous mail, please let me know if you're OK with me staging this change for 4.18 via linux-dm.git with your Ack, thanks]

From: Mikulas Patocka <[email protected]>
Subject: [PATCH] swait: export symbols __prepare_to_swait and __finish_swait

__prepare_to_swait and __finish_swait are declared in include/linux/swait.h but they are not exported; so they are not useable from kernel modules.

A new consumer of swait (in dm-writecache) reduces its locking overhead by using the spinlock in swait_queue_head to protect not only the wait queue, but also the list of events. Consequently, this swait consuming kernel module needs to use these unlocked functions.

Peter Zijlstra explained:
"The reason swait exists is to be deterministic (for RT) -- something that regular wait code cannot be.
And by (ab)using / exporting the wait internal lock you risk losing that. So I don't think the proposed [dm-writecache] usage is bad, it is possible to create badness.
So if we're going to export them; someone needs to keep an eye on things and ensure the lock isn't abused."

Quoted 6 lines

So while this new use of the wait internal lock doesn't jeopardize the realtime requirements of swait, these exports do open swait's internal locking up to being abused. As such, EXPORT_SYMBOL_GPL is used because any future consumers of __prepare_to_swait and __finish_swait must always be thoroughly scrutinized.

Acked-by Peter Zijlstra (Intel) <[email protected]>

The dm-writecache target.

Signed-off-by Mikulas Patocka <[email protected]>

drivers/md/dm-writecache.c new file +4 -0

+/*
+ * On X86, non-temporal stores are more efficient than cache flushing.
+ * On ARM64, cache flushing is more efficient.
+ */

Use new API for flushing persistent memory.

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

Quoted 1 line · Mikulas Patocka

Use new API for flushing persistent memory.

The sentence doesnt make much sense. 'A new API', 'A better abstraction' maybe?

Quoted 16 lines

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

Quoted 2 lines

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

All these should be provided by the pmem layer, and be properly documented. And be sorted before adding your new target that uses them.

Quoted 22 lines · Christoph Hellwig

Quoted 1 line · Mikulas Patocka

Use new API for flushing persistent memory.

The sentence doesnt make much sense. 'A new API', 'A better abstraction' maybe?

Quoted 16 lines

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

Quoted 6 lines

Quoted 2 lines

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

All these should be provided by the pmem layer, and be properly documented. And be sorted before adding your new target that uses them.

I don't see that as a hard requirement. Mikulas did the work to figure out what is more optimal on x86_64 vs amd64. It makes a difference for his target and that is sufficient to carry it locally until/when it is either elevated to pmem.

We cannot even get x86 and swait maintainers to reply to repeat requests for review. Stacking up further deps on pmem isn't high on my list.

Quoted 37 lines · Mike Snitzer

Quoted 22 lines · Christoph Hellwig

Quoted 1 line · Mikulas Patocka

Use new API for flushing persistent memory.

The sentence doesnt make much sense. 'A new API', 'A better abstraction' maybe?

Quoted 16 lines

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

Quoted 6 lines

Quoted 2 lines

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

All these should be provided by the pmem layer, and be properly documented. And be sorted before adding your new target that uses them.

I don't see that as a hard requirement. Mikulas did the work to figure out what is more optimal on x86_64 vs amd64. It makes a difference for his target and that is sufficient to carry it locally until/when it is either elevated to pmem.

Quoted 3 lines

We cannot even get x86 and swait maintainers to reply to repeat requests for review. Stacking up further deps on pmem isn't high on my list.

Except I'm being responsive.

I agree with Christoph that we should
build pmem helpers at an architecture level and not per-driver. Let's make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch up to x86 in this space. We already have PowerPC enabling PMEM API, so I don't see an unreasonable barrier to ask the same of ARM. This patch is not even cc'd to linux-arm-kernel. Has the subject been broached with them?

Quoted 38 lines · Dan Williams

Quoted 37 lines · Mike Snitzer

Quoted 22 lines · Christoph Hellwig

Quoted 1 line · Mikulas Patocka

Use new API for flushing persistent memory.

The sentence doesnt make much sense. 'A new API', 'A better abstraction' maybe?

Quoted 16 lines

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

Quoted 6 lines

Quoted 2 lines

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

All these should be provided by the pmem layer, and be properly documented. And be sorted before adding your new target that uses them.

I don't see that as a hard requirement. Mikulas did the work to figure out what is more optimal on x86_64 vs amd64. It makes a difference for his target and that is sufficient to carry it locally until/when it is either elevated to pmem.

Quoted 5 lines

Quoted 3 lines

We cannot even get x86 and swait maintainers to reply to repeat requests for review. Stacking up further deps on pmem isn't high on my list.

Except I'm being responsive.

Except you're looking to immediately punt to linux-arm-kernel ;)

Quoted 8 lines

I agree with Christoph that we should
build pmem helpers at an architecture level and not per-driver. Let's make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch up to x86 in this space. We already have PowerPC enabling PMEM API, so I don't see an unreasonable barrier to ask the same of ARM. This patch is not even cc'd to linux-arm-kernel. Has the subject been broached with them?

No idea. Not by me.

The thing is, I'm no expert in pmem. You are. Coordinating the change with ARM et al feels unnecessarily limiting and quicky moves outside my control.

Serious question: Why can't this code land in this dm-writecache target and then be lifted (or obsoleted)?

But if you think it worthwhile to force ARM to step up then fine. That does limit the availability of using writecache on ARM while they get the PMEM API together.

I'll do whatever you want.. just put the smack down and tell me how it is ;)

Quoted 41 lines · Mike Snitzer

Quoted 38 lines · Dan Williams

Quoted 37 lines · Mike Snitzer

Quoted 22 lines · Christoph Hellwig

Quoted 1 line · Mikulas Patocka

Use new API for flushing persistent memory.

The sentence doesnt make much sense. 'A new API', 'A better abstraction' maybe?

Quoted 16 lines

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

Quoted 6 lines

Quoted 2 lines

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

All these should be provided by the pmem layer, and be properly documented. And be sorted before adding your new target that uses them.

I don't see that as a hard requirement. Mikulas did the work to figure out what is more optimal on x86_64 vs amd64. It makes a difference for his target and that is sufficient to carry it locally until/when it is either elevated to pmem.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

We cannot even get x86 and swait maintainers to reply to repeat requests for review. Stacking up further deps on pmem isn't high on my list.

Except I'm being responsive.

Except you're looking to immediately punt to linux-arm-kernel ;)

Well, I'm not, not really. I'm saying drop ARM support, it's not ready.

Quoted 9 lines

Quoted 8 lines

I agree with Christoph that we should
build pmem helpers at an architecture level and not per-driver. Let's make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch up to x86 in this space. We already have PowerPC enabling PMEM API, so I don't see an unreasonable barrier to ask the same of ARM. This patch is not even cc'd to linux-arm-kernel. Has the subject been broached with them?

Quoted 8 lines

No idea. Not by me.

The thing is, I'm no expert in pmem. You are. Coordinating the change with ARM et al feels unnecessarily limiting and quicky moves outside my control.

Serious question: Why can't this code land in this dm-writecache target and then be lifted (or obsoleted)?

Because we already have an API, and we don't want to promote local solutions to global problems, or carry unnecessary technical debt.

Quoted 5 lines

But if you think it worthwhile to force ARM to step up then fine. That does limit the availability of using writecache on ARM while they get the PMEM API together.

Quoted 2 lines

I'll do whatever you want.. just put the smack down and tell me how it is ;)

I'd say just control the variables you can control. Drop the ARM support if you want to move forward and propose extensions / updates to the pmem api for x86 and I'll help push those since I was involved in pushing the x86 pmem api in the first instance. That way you don't need to touch this driver as new archs add their pmem api enabling.

Looking at Mikulas' wrapper API that you and hch are calling into question:

For ARM it is using arch/arm64/mm/flush.c:arch_wb_cache_pmem(). (And ARM does seem to be providing CONFIG_ARCH_HAS_PMEM_API.)

Whereas x86_64 is using memcpy_flushcache() as provided by CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE.
(Yet ARM does provide arch/arm64/lib/uaccess_flushcache.c:memcpy_flushcache)

Just seems this isn't purely about ARM lacking on an API level (given on x86_64 Mikulas isn't only using CONFIG_ARCH_HAS_PMEM_API).

Seems this is more to do with x86_64 having efficient Non-temporal stores?

Anyway, I'm still trying to appreciate the details here before I can make any forward progress.

Hi, Mike,

Quoted 10 lines · Mike Snitzer

Looking at Mikulas' wrapper API that you and hch are calling into question:

For ARM it is using arch/arm64/mm/flush.c:arch_wb_cache_pmem(). (And ARM does seem to be providing CONFIG_ARCH_HAS_PMEM_API.)

Whereas x86_64 is using memcpy_flushcache() as provided by CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE.
(Yet ARM does provide arch/arm64/lib/uaccess_flushcache.c:memcpy_flushcache)

Quoted 5 lines

Just seems this isn't purely about ARM lacking on an API level (given on x86_64 Mikulas isn't only using CONFIG_ARCH_HAS_PMEM_API).

Seems this is more to do with x86_64 having efficient Non-temporal stores?

Yeah, I think you've got that all right.

Quoted 2 lines

Anyway, I'm still trying to appreciate the details here before I can make any forward progress.

Making data persistent on x64 requires 3 steps:
1) copy the data into pmem (store instructions)
2) flush the cache lines associated with the data (clflush, clflush_opt, clwb)
3) wait on the flush to complete (sfence)

I'm not sure if other architectures require step 3. Mikulas' implementation seems to imply that arm64 doesn't require the fence.

The current pmem api provides:

memcpy* -- step 1
memcpy_flushcache -- this combines steps 1 and 2
dax_flush -- step 2
wmb* -- step 3

* not strictly part of the pmem api

So, if you didn't care about performance, you could write generic code that only used memcpy, dax_flush, and wmb (assuming other arches actually need the wmb). What Mikulas did was to abstract out an API that could be called by generic code that would work optimally on all architectures.

This looks like a worth-while addition to the PMEM API, to me. Mikulas, what do you think about refactoring the code as Christoph suggested?

Cheers,
Jeff

Quoted 3 lines

Quoted 1 line

Except I'm being responsive.

Except you're looking to immediately punt to linux-arm-kernel ;)

Well, I'm not, not really. I'm saying drop ARM support, it's not ready.

Quoted 8 lines

Quoted 7 lines

I agree with Christoph that we should
build pmem helpers at an architecture level and not per-driver. Let's make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch up to x86 in this space. We already have PowerPC enabling PMEM API, so I don't see an unreasonable barrier to ask the same of ARM. This patch is not even cc'd to linux-arm-kernel. Has the subject been broached with them?

Quoted 8 lines

No idea. Not by me.

The thing is, I'm no expert in pmem. You are. Coordinating the change with ARM et al feels unnecessarily limiting and quicky moves outside my control.

Serious question: Why can't this code land in this dm-writecache target and then be lifted (or obsoleted)?

Because we already have an API, and we don't want to promote local solutions to global problems, or carry unnecessary technical debt.

Quoted 5 lines

But if you think it worthwhile to force ARM to step up then fine. That does limit the availability of using writecache on ARM while they get the PMEM API together.

Quoted 2 lines

I'll do whatever you want.. just put the smack down and tell me how it is ;)

I'd say just control the variables you can control. Drop the ARM support if you want to move forward and propose extensions / updates

to the pmem api for x86 and I'll help push those since I was involved in pushing the x86 pmem api in the first instance. That way you don't need to touch this driver as new archs add their pmem api enabling.

Quoted 5 lines · Dan Williams

Quoted 3 lines

Quoted 1 line

Except I'm being responsive.

Except you're looking to immediately punt to linux-arm-kernel ;)

Well, I'm not, not really. I'm saying drop ARM support, it's not ready.

This is the worst thing to do - because once late cache flushing is dropped from the dm-writecache target, it could hardly be reintroduced again.

Quoted 25 lines

Quoted 8 lines

Quoted 7 lines

I agree with Christoph that we should
build pmem helpers at an architecture level and not per-driver. Let's make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch up to x86 in this space. We already have PowerPC enabling PMEM API, so I don't see an unreasonable barrier to ask the same of ARM. This patch is not even cc'd to linux-arm-kernel. Has the subject been broached with them?

Quoted 8 lines

No idea. Not by me.

The thing is, I'm no expert in pmem. You are. Coordinating the change with ARM et al feels unnecessarily limiting and quicky moves outside my control.

Serious question: Why can't this code land in this dm-writecache target and then be lifted (or obsoleted)?

Because we already have an API, and we don't want to promote local solutions to global problems, or carry unnecessary technical debt.

Quoted 5 lines

But if you think it worthwhile to force ARM to step up then fine. That does limit the availability of using writecache on ARM while they get the PMEM API together.

Quoted 5 lines

Quoted 2 lines

I'll do whatever you want.. just put the smack down and tell me how it is ;)

I'd say just control the variables you can control. Drop the ARM support if you want to move forward and propose extensions / updates

What do we gain by dropping it?

Quoted 3 lines

to the pmem api for x86 and I'll help push those since I was involved in pushing the x86 pmem api in the first instance. That way you don't need to touch this driver as new archs add their pmem api enabling.

The pmem API is x86-centric - that the problem.

When I read your patch I came away with the impression that ARM had not added memcpy_flushcache() yet and you were working around that fact. Now that I look, ARM *does* define memcpy_flushcache() and you're avoiding it. You use memcpy+arch_wb_pmem where arch_wb_pmem on ARM64 is defined as __clean_dcache_area_pop(dst, cnt). The ARM memcpy_flushcache() implementation is:

    memcpy(dst, src, cnt);
    __clean_dcache_area_pop(dst, cnt);

So, I do not see how what you're doing is any less work unless you are flushing less than you copy?

If memcpy_flushcache() is slower than memcpy + arch_wb_pmem then the ARM implementation is broken and that needs to be addressed not worked around in a driver.

Quoted 10 lines · Dan Williams

When I read your patch I came away with the impression that ARM had not added memcpy_flushcache() yet and you were working around that fact. Now that I look, ARM *does* define memcpy_flushcache() and you're avoiding it. You use memcpy+arch_wb_pmem where arch_wb_pmem on ARM64 is defined as __clean_dcache_area_pop(dst, cnt). The ARM memcpy_flushcache() implementation is:

    memcpy(dst, src, cnt);
    __clean_dcache_area_pop(dst, cnt);

Quoted 6 lines

So, I do not see how what you're doing is any less work unless you are flushing less than you copy?

If memcpy_flushcache() is slower than memcpy + arch_wb_pmem then the ARM implementation is broken and that needs to be addressed not worked around in a driver.

I think Mikulas wanted to batch up multiple copies and flush at the end. According to his commit message, that batching gained him 2% performance.

-Jeff

Quoted 13 lines · Dan Williams

When I read your patch I came away with the impression that ARM had not added memcpy_flushcache() yet and you were working around that fact. Now that I look, ARM *does* define memcpy_flushcache() and you're avoiding it. You use memcpy+arch_wb_pmem where arch_wb_pmem on ARM64 is defined as __clean_dcache_area_pop(dst, cnt). The ARM memcpy_flushcache() implementation is:

    memcpy(dst, src, cnt);
    __clean_dcache_area_pop(dst, cnt);

So, I do not see how what you're doing is any less work unless you are flushing less than you copy?

Quoted 3 lines

If memcpy_flushcache() is slower than memcpy + arch_wb_pmem then the ARM implementation is broken and that needs to be addressed not worked around in a driver.

I think Mikulas wanted to batch up multiple copies and flush at the end. According to his commit message, that batching gained him 2% performance.

Quoted 32 lines · Mike Snitzer

Quoted 22 lines · Christoph Hellwig

Quoted 1 line · Mikulas Patocka

Use new API for flushing persistent memory.

The sentence doesnt make much sense. 'A new API', 'A better abstraction' maybe?

Quoted 16 lines

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

Quoted 6 lines

Quoted 2 lines

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

All these should be provided by the pmem layer, and be properly documented. And be sorted before adding your new target that uses them.

Quoted 8 lines

I don't see that as a hard requirement. Mikulas did the work to figure out what is more optimal on x86_64 vs amd64. It makes a difference for his target and that is sufficient to carry it locally until/when it is either elevated to pmem.

We cannot even get x86 and swait maintainers to reply to repeat requests for review. Stacking up further deps on pmem isn't high on my list.

Except I'm being responsive. I agree with Christoph that we should build pmem helpers at an architecture level and not per-driver. Let's make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch up to x86 in this space. We already have PowerPC enabling PMEM API, so I don't see an unreasonable barrier to ask the same of ARM. This patch is not even cc'd to linux-arm-kernel. Has the subject been broached with them?

Use new API for flushing persistent memory.

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

Quoted 11 lines · Mikulas Patocka

Use new API for flushing persistent memory.

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

Quoted 8 lines

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 21 lines · Dan Williams

Quoted 11 lines · Mikulas Patocka

Use new API for flushing persistent memory.

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

Quoted 8 lines

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

Quoted 9 lines

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

But memcpy_flushcache() on ARM64 is slow.

Quoted 35 lines · Mikulas Patocka

Quoted 21 lines · Dan Williams

Quoted 11 lines · Mikulas Patocka

Use new API for flushing persistent memory.

The problem is this:
* on X86-64, non-temporal stores have the best performance
* ARM64 doesn't have non-temporal stores, so we must flush cache. We should flush cache as late as possible, because it performs better this way.

We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

Quoted 8 lines

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

Quoted 9 lines

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Yes, Dan can you please take some time to fully appreciate what this small wrapper API is providing (maybe you've done that, but your recent reply is mixed-message). Seems you're keeping the innovation it provides at arms-length. Basically the PMEM APIs you've helped construct are lacking, forcing DM developers to own fixing them is an inversion that only serves to stonewall.

Please, less time on stonewalling and more time lifting this wrapper API; otherwise the dm-writecache local wrapper API is a near-term means to an end.

I revised the dm-writecache patch header yesterday, please review:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.18&id=2105231db61b08752bc4247d2fe7838657700b0d
(the last paragraph in particular, I intend to move forward with this wrapper unless someone on the PMEM side of the house steps up and lifts it up between now and when the 4.18 merge window opens)

dm: add writecache target
The writecache target caches writes on persistent memory or SSD. It is intended for databases or other programs that need extremely low commit latency.

The writecache target doesn't cache reads because reads are supposed to be cached in page cache in normal RAM.

The following describes the approach used to provide the most efficient flushing of persistent memory on X86_64 vs ARM64:

* On X86_64 non-temporal stores (i.e. memcpy_flushcache) are faster than cached writes and cache flushing.

* The ARM64 architecture doesn't have non-temporal stores. So, memcpy_flushcache on ARM does memcpy (that writes data to the cache) and then flushes the cache. But this eager cache flushig is slower than late cache flushing.

The optimal code sequence on ARM to write to persistent memory is to call memcpy, then do something else, and then call arch_wb_cache_pmem as late as possible. And this ARM-optimized code sequence is just horribly slow on X86.

This issue can't be "fixed" in ARM-specific source code. The ARM processor have characteristics such that eager cache flushing is slower than late cache flushing - and that's it - you can't change processor behavior.

We introduce a wrapper API for flushing persistent memory with functions pmem_memcpy, pmem_flush and pmem_commit. To commit data persistently, all three functions must be called.

The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign (unlike pmem_memcpy) guarantees that 8-byte values are written atomically.

On X86_64, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and pmem_commit is wmb.

On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and pmem_commit is empty.

It is clear that this wrapper API for flushing persistent memory needs to be elevated out of this dm-writecache driver. But that can happen later without requiring DM developers to blaze new trails on pmem specific implementation details/quirks (pmem developers need to clean up their APIs given they are already spread across CONFIG_ARCH_HAS_PMEM_API and CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE and they absolutely don't take into account the duality of the different programming models needed to achieve optimal cross-architecture use of persistent memory).

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

Quoted 16 lines · Dan Williams

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 6 lines

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

memcpy_flushcache on ARM64 is generally wrong thing to do, because it is slower than memcpy and explicit cache flush.

Suppose that you want to write data to a block device and make it persistent. So you send a WRITE bio and then a FLUSH bio.

Now - how to implement these two bios on persistent memory:

On X86, the WRITE bio does memcpy_flushcache() and the FLUSH bio does wmb() - this is the optimal implementation.

But on ARM64, memcpy_flushcache() is suboptimal. On ARM64, the optimal implementation is that the WRITE bio does just memcpy() and the FLUSH bio does arch_wb_cache_pmem() on the affected range.

Why is memcpy_flushcache() is suboptimal on ARM? The ARM architecture doesn't have non-temporal stores. So, memcpy_flushcache() is implemented as memcpy() followed by a cache flush.

Now - if you flush the cache immediatelly after memcpy, the cache is full of dirty lines and the cache-flushing code has to write these lines back and that is slow.

If you flush the cache some time after memcpy (i.e. when the FLUSH bio is received), the processor already flushed some part of the cache on its own, so the cache-flushing function has less work to do and it is faster.

So the conclusion is - don't use memcpy_flushcache on ARM. This problem cannot be fixed by a better implementation of memcpy_flushcache.

Quoted 54 lines · Mikulas Patocka

Quoted 16 lines · Dan Williams

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 6 lines

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

memcpy_flushcache on ARM64 is generally wrong thing to do, because it is slower than memcpy and explicit cache flush.

Suppose that you want to write data to a block device and make it persistent. So you send a WRITE bio and then a FLUSH bio.

Now - how to implement these two bios on persistent memory:

On X86, the WRITE bio does memcpy_flushcache() and the FLUSH bio does wmb() - this is the optimal implementation.

But on ARM64, memcpy_flushcache() is suboptimal. On ARM64, the optimal implementation is that the WRITE bio does just memcpy() and the FLUSH bio does arch_wb_cache_pmem() on the affected range.

Why is memcpy_flushcache() is suboptimal on ARM? The ARM architecture doesn't have non-temporal stores. So, memcpy_flushcache() is implemented as memcpy() followed by a cache flush.

Now - if you flush the cache immediatelly after memcpy, the cache is full of dirty lines and the cache-flushing code has to write these lines back and that is slow.

If you flush the cache some time after memcpy (i.e. when the FLUSH bio is received), the processor already flushed some part of the cache on its own, so the cache-flushing function has less work to do and it is faster.

Quoted 2 lines

So the conclusion is - don't use memcpy_flushcache on ARM. This problem cannot be fixed by a better implementation of memcpy_flushcache.

It sounds like ARM might be better off with mapping its pmem as write-through rather than write-back, and skip the explicit cache

management altogether. You speak of "optimal" and "sub-optimal", but what would be more clear is fio measurements of the relative IOPs and latency profiles of the different approaches. The reason I am continuing to push here is that reducing the operation space from 'copy-flush-commit' to just 'copy' or 'copy-commit' simplifies the maintenance long term.

Quoted 55 lines · Dan Williams

Quoted 54 lines · Mikulas Patocka

Quoted 16 lines · Dan Williams

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 6 lines

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

memcpy_flushcache on ARM64 is generally wrong thing to do, because it is slower than memcpy and explicit cache flush.

Suppose that you want to write data to a block device and make it persistent. So you send a WRITE bio and then a FLUSH bio.

Now - how to implement these two bios on persistent memory:

On X86, the WRITE bio does memcpy_flushcache() and the FLUSH bio does wmb() - this is the optimal implementation.

But on ARM64, memcpy_flushcache() is suboptimal. On ARM64, the optimal implementation is that the WRITE bio does just memcpy() and the FLUSH bio does arch_wb_cache_pmem() on the affected range.

Why is memcpy_flushcache() is suboptimal on ARM? The ARM architecture doesn't have non-temporal stores. So, memcpy_flushcache() is implemented as memcpy() followed by a cache flush.

Now - if you flush the cache immediatelly after memcpy, the cache is full of dirty lines and the cache-flushing code has to write these lines back and that is slow.

If you flush the cache some time after memcpy (i.e. when the FLUSH bio is received), the processor already flushed some part of the cache on its own, so the cache-flushing function has less work to do and it is faster.

Quoted 5 lines

Quoted 2 lines

So the conclusion is - don't use memcpy_flushcache on ARM. This problem cannot be fixed by a better implementation of memcpy_flushcache.

It sounds like ARM might be better off with mapping its pmem as write-through rather than write-back, and skip the explicit cache

I doubt it would perform well - write combining combines the writes into a larger segments - and write-through doesn't.

Quoted 6 lines

management altogether. You speak of "optimal" and "sub-optimal", but what would be more clear is fio measurements of the relative IOPs and latency profiles of the different approaches. The reason I am continuing to push here is that reducing the operation space from 'copy-flush-commit' to just 'copy' or 'copy-commit' simplifies the maintenance long term.

I measured it (with nvme backing store) and late cache flushing has 12% better performance than eager flushing with memcpy_flushcache().

131836 4k iops - vs - 117016.

Quoted 59 lines · Mikulas Patocka

Quoted 55 lines · Dan Williams

Quoted 54 lines · Mikulas Patocka

Quoted 16 lines · Dan Williams

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 6 lines

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

memcpy_flushcache on ARM64 is generally wrong thing to do, because it is slower than memcpy and explicit cache flush.

Suppose that you want to write data to a block device and make it persistent. So you send a WRITE bio and then a FLUSH bio.

Now - how to implement these two bios on persistent memory:

On X86, the WRITE bio does memcpy_flushcache() and the FLUSH bio does wmb() - this is the optimal implementation.

But on ARM64, memcpy_flushcache() is suboptimal. On ARM64, the optimal implementation is that the WRITE bio does just memcpy() and the FLUSH bio does arch_wb_cache_pmem() on the affected range.

Why is memcpy_flushcache() is suboptimal on ARM? The ARM architecture doesn't have non-temporal stores. So, memcpy_flushcache() is implemented as memcpy() followed by a cache flush.

Now - if you flush the cache immediatelly after memcpy, the cache is full of dirty lines and the cache-flushing code has to write these lines back and that is slow.

If you flush the cache some time after memcpy (i.e. when the FLUSH bio is received), the processor already flushed some part of the cache on its own, so the cache-flushing function has less work to do and it is faster.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

So the conclusion is - don't use memcpy_flushcache on ARM. This problem cannot be fixed by a better implementation of memcpy_flushcache.

It sounds like ARM might be better off with mapping its pmem as write-through rather than write-back, and skip the explicit cache

I doubt it would perform well - write combining combines the writes into a larger segments - and write-through doesn't.

Last I checked write-through caching does not disable write combining

Quoted 9 lines

Quoted 6 lines

management altogether. You speak of "optimal" and "sub-optimal", but what would be more clear is fio measurements of the relative IOPs and latency profiles of the different approaches. The reason I am continuing to push here is that reducing the operation space from 'copy-flush-commit' to just 'copy' or 'copy-commit' simplifies the maintenance long term.

I measured it (with nvme backing store) and late cache flushing has 12% better performance than eager flushing with memcpy_flushcache().

I assume what you're seeing is ARM64 over-flushing the amount of dirty data so it becomes more efficient to do an amortized flush at the end? However, that effectively makes memcpy_flushcache() unusable in the way it can be used on x86. You claimed that ARM does not support non-temporal stores, but it does, see the STNP instruction. I do not want to see arch specific optimizations in drivers, so either write-through mappings is a potential answer to remove the need to explicitly manage flushing, or just implement STNP hacks in memcpy_flushcache() like you did with MOVNT on x86.

Quoted 1 line

131836 4k iops - vs - 117016.

To be clear this is memcpy_flushcache() vs memcpy + flush?

Quoted 15 lines · Dan Williams

Quoted 3 lines · Mikulas Patocka

I measured it (with nvme backing store) and late cache flushing has 12% better performance than eager flushing with memcpy_flushcache().

I assume what you're seeing is ARM64 over-flushing the amount of dirty data so it becomes more efficient to do an amortized flush at the end? However, that effectively makes memcpy_flushcache() unusable in the way it can be used on x86. You claimed that ARM does not support non-temporal stores, but it does, see the STNP instruction. I do not want to see arch specific optimizations in drivers, so either write-through mappings is a potential answer to remove the need to explicitly manage flushing, or just implement STNP hacks in memcpy_flushcache() like you did with MOVNT on x86.

Quoted 3 lines

Quoted 1 line

131836 4k iops - vs - 117016.

To be clear this is memcpy_flushcache() vs memcpy + flush?

I found out what caused the difference. I used dax_flush on the version of dm-writecache that I had on the ARM machine (with the kernel 4.14, because it is the last version where dax on ramdisk works) - and I thought that dax_flush flushes the cache, but it doesn't.

When I replaced dax_flush with arch_wb_cache_pmem, the performance difference between early flushing and late flushing disappeared.

So I think we can remove this per-architecture switch from dm-writecache.

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Thanks,
Mike

Quoted 3 lines · Mike Snitzer

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Quoted 2 lines

Thanks,
Mike

I've sent the current version that I have. I fixed the bugs that were reported here (missing DAX, dm_bufio_client_create, __branch_check__ long->int truncation).

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Mike Snitzer

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Quoted 6 lines

Quoted 2 lines

Thanks,
Mike

I've sent the current version that I have. I fixed the bugs that were reported here (missing DAX, dm_bufio_client_create, __branch_check__ long->int truncation).

OK, but a monolithic dm-writecache.c is no longer useful to me. I can drop Arnd's gcc warning fix (with the idea that Ingo or Steve will take your __branch_check__ patch). Not sure what the dm_bufio_client_create fix is... must've missed a report about that.

ANyway, point is we're on too a different phase of dm-writecache.c's development. I've picked it up and am trying to get it ready for the 4.18 merge window (likely opening Sunday). Therefore it needs to be in a git tree, and incremental changes overlayed. I cannot be rebasing at this late stage in the 4.18 development window.

Thanks,
Mike

Quoted 28 lines · Mike Snitzer

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Mike Snitzer

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Quoted 6 lines

Quoted 2 lines

Thanks,
Mike

I've sent the current version that I have. I fixed the bugs that were reported here (missing DAX, dm_bufio_client_create, __branch_check__ long->int truncation).

OK, but a monolithic dm-writecache.c is no longer useful to me. I can drop Arnd's gcc warning fix (with the idea that Ingo or Steve will take your __branch_check__ patch). Not sure what the dm_bufio_client_create fix is... must've missed a report about that.

ANyway, point is we're on too a different phase of dm-writecache.c's development. I've picked it up and am trying to get it ready for the 4.18 merge window (likely opening Sunday). Therefore it needs to be in a git tree, and incremental changes overlayed. I cannot be rebasing at this late stage in the 4.18 development window.

Quoted 2 lines

Thanks,
Mike

I downloaded dm-writecache from your git repository some times ago - but you changed a lot of useless things (i.e. reordering the fields in the structure) since that time - so, you'll have to merge the changes.

You dropped the latency measuring code - why? We still would like to benchmark the driver.

Quoted 35 lines · Mikulas Patocka

Quoted 30 lines · Mike Snitzer

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Mike Snitzer

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Quoted 6 lines

Quoted 2 lines

Thanks,
Mike

I've sent the current version that I have. I fixed the bugs that were reported here (missing DAX, dm_bufio_client_create, __branch_check__ long->int truncation).

OK, but a monolithic dm-writecache.c is no longer useful to me. I can drop Arnd's gcc warning fix (with the idea that Ingo or Steve will take your __branch_check__ patch). Not sure what the dm_bufio_client_create fix is... must've missed a report about that.

ANyway, point is we're on too a different phase of dm-writecache.c's development. I've picked it up and am trying to get it ready for the 4.18 merge window (likely opening Sunday). Therefore it needs to be in a git tree, and incremental changes overlayed. I cannot be rebasing at this late stage in the 4.18 development window.

Thanks,
Mike

Quoted 3 lines

I downloaded dm-writecache from your git repository some times ago - but you changed a lot of useless things (i.e. reordering the fields in the structure) since that time - so, you'll have to merge the changes.

Fine I'll deal with it. reordering the fields eliminated holes in the structure and reduced struct members spanning cache lines.

Quoted 38 lines · Mike Snitzer

Quoted 35 lines · Mikulas Patocka

Quoted 30 lines · Mike Snitzer

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Mike Snitzer

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Quoted 6 lines

Quoted 2 lines

Thanks,
Mike

I've sent the current version that I have. I fixed the bugs that were reported here (missing DAX, dm_bufio_client_create, __branch_check__ long->int truncation).

OK, but a monolithic dm-writecache.c is no longer useful to me. I can drop Arnd's gcc warning fix (with the idea that Ingo or Steve will take your __branch_check__ patch). Not sure what the dm_bufio_client_create fix is... must've missed a report about that.

ANyway, point is we're on too a different phase of dm-writecache.c's development. I've picked it up and am trying to get it ready for the 4.18 merge window (likely opening Sunday). Therefore it needs to be in a git tree, and incremental changes overlayed. I cannot be rebasing at this late stage in the 4.18 development window.

Thanks,
Mike

Quoted 6 lines

Quoted 3 lines

I downloaded dm-writecache from your git repository some times ago - but you changed a lot of useless things (i.e. reordering the fields in the structure) since that time - so, you'll have to merge the changes.

Fine I'll deal with it. reordering the fields eliminated holes in the structure and reduced struct members spanning cache lines.

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

Quoted 56 lines · Mikulas Patocka

Quoted 38 lines · Mike Snitzer

Quoted 35 lines · Mikulas Patocka

Quoted 30 lines · Mike Snitzer

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Mike Snitzer

That is really great news, can you submit an incremental patch that layers ontop of the linux-dm.git 'dm-4.18' branch?

Quoted 6 lines

Quoted 2 lines

Thanks,
Mike

I've sent the current version that I have. I fixed the bugs that were reported here (missing DAX, dm_bufio_client_create, __branch_check__ long->int truncation).

OK, but a monolithic dm-writecache.c is no longer useful to me. I can drop Arnd's gcc warning fix (with the idea that Ingo or Steve will take your __branch_check__ patch). Not sure what the dm_bufio_client_create fix is... must've missed a report about that.

ANyway, point is we're on too a different phase of dm-writecache.c's development. I've picked it up and am trying to get it ready for the 4.18 merge window (likely opening Sunday). Therefore it needs to be in a git tree, and incremental changes overlayed. I cannot be rebasing at this late stage in the 4.18 development window.

Thanks,
Mike

Quoted 6 lines

Quoted 3 lines

I downloaded dm-writecache from your git repository some times ago - but you changed a lot of useless things (i.e. reordering the fields in the structure) since that time - so, you'll have to merge the changes.

Fine I'll deal with it. reordering the fields eliminated holes in the structure and reduced struct members spanning cache lines.

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Quoted 7 lines

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

This level of microoptimization can be backfilled. But as it was, there were too many #defines. And I'm really not concerned with eliminating unused static functions for this case.

Quoted 18 lines · Mike Snitzer

Quoted 10 lines

Quoted 2 lines

Fine I'll deal with it. reordering the fields eliminated holes in the structure and reduced struct members spanning cache lines.

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Quoted 7 lines

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

Quoted 3 lines

This level of microoptimization can be backfilled. But as it was, there were too many #defines. And I'm really not concerned with eliminating unused static functions for this case.

I don't see why "too many defines" would be a problem.

If I compile it with and without pmem support, the difference is 15kB-vs-12kB. If we look at just one function (writecache_map), the difference is 1595 bytes - vs - 1280 bytes. So, it produces real savings in code size.

The problem with performance is not caused a condition that always jumps the same way (that is predicted by the CPU and it causes no delays in the pipeline) - the problem is that a bigger function consumes more i-cache. There is no reason to include code that can't be executed.

Quoted 33 lines · Mikulas Patocka

Quoted 18 lines · Mike Snitzer

Quoted 10 lines

Quoted 2 lines

Fine I'll deal with it. reordering the fields eliminated holes in the structure and reduced struct members spanning cache lines.

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Quoted 7 lines

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

Quoted 3 lines

This level of microoptimization can be backfilled. But as it was, there were too many #defines. And I'm really not concerned with eliminating unused static functions for this case.

I don't see why "too many defines" would be a problem.

If I compile it with and without pmem support, the difference is 15kB-vs-12kB. If we look at just one function (writecache_map), the difference is 1595 bytes - vs - 1280 bytes. So, it produces real savings in code size.

Quoted 4 lines

The problem with performance is not caused a condition that always jumps the same way (that is predicted by the CPU and it causes no delays in the pipeline) - the problem is that a bigger function consumes more i-cache. There is no reason to include code that can't be executed.

Please double check you see the reduced code size you're expecting using the latest dm-writecache.c in linux-dm.git's dm-4.18 branch.

Thanks,
Mike

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

Quoted 7 lines · Mikulas Patocka

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Quoted 7 lines

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

Here is the patch that I just folded into the rebased version of dm-writecache now in the dm-4.18 branch.

(I rebased ontop of Jens' latest block tree for 4.18 that now includes the mempool_init changes, etc.)

---
 drivers/md/dm-writecache.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

drivers/md/dm-writecache.c +15 -2

index fcbfaf7c27ec..691b5ffb799f 100644
@@ -34,13 +34,21 @@
 #define BITMAP_GRANULARITY	PAGE_SIZE
 #endif
 
+#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
+#define DM_WRITECACHE_HAS_PMEM
+#endif
+
+#ifdef DM_WRITECACHE_HAS_PMEM
 #define pmem_assign(dest, src)					\
 do {								\
 	typeof(dest) uniq = (src);				\
 	memcpy_flushcache(&(dest), &uniq, sizeof(dest));	\
 } while (0)
+#else
+#define pmem_assign(dest, src)	((dest)) = (src))
+#endif
 
-#if defined(__HAVE_ARCH_MEMCPY_MCSAFE) && defined(CONFIG_ARCH_HAS_PMEM_API)
+#if defined(__HAVE_ARCH_MEMCPY_MCSAFE) && defined(DM_WRITECACHE_HAS_PMEM)
 #define DM_WRITECACHE_HANDLE_HARDWARE_ERRORS
 #endif
 

@@ -87,8 +95,13 @@ struct wc_entry {
 #endif
 };
 
+#ifdef DM_WRITECACHE_HAS_PMEM
 #define WC_MODE_PMEM(wc)			((wc)->pmem_mode)
 #define WC_MODE_FUA(wc)				((wc)->writeback_fua)
+#else
+#define WC_MODE_PMEM(wc)			false
+#define WC_MODE_FUA(wc)				false
+#endif
 #define WC_MODE_SORT_FREELIST(wc)		(!WC_MODE_PMEM(wc))
 
 struct dm_writecache {

@@ -1857,7 +1870,7 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (!strcasecmp(string, "s")) {
 		wc->pmem_mode = false;
 	} else if (!strcasecmp(string, "p")) {
-#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
+#ifdef DM_WRITECACHE_HAS_PMEM
 		wc->pmem_mode = true;
 		wc->writeback_fua = true;
 #else

Signature · 1 line

2.15.0

Quoted 80 lines · Mike Snitzer

Quoted 7 lines · Mikulas Patocka

And what about this?
#define WC_MODE_PMEM(wc) ((wc)->pmem_mode)

The code that I had just allowed the compiler to optimize out persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you deleted it.

Quoted 7 lines

Most architectures don't have persistent memory and the dm-writecache driver could work in ssd-only mode on them. On these architectures, I define
#define WC_MODE_PMEM(wc) false
- and the compiler will just automatically remove the tests for that condition and the unused branch. It does also eliminate unused static functions.

Here is the patch that I just folded into the rebased version of dm-writecache now in the dm-4.18 branch.

(I rebased ontop of Jens' latest block tree for 4.18 that now includes the mempool_init changes, etc.)

---
 drivers/md/dm-writecache.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

drivers/md/dm-writecache.c +15 -2

index fcbfaf7c27ec..691b5ffb799f 100644
@@ -34,13 +34,21 @@
 #define BITMAP_GRANULARITY	PAGE_SIZE
 #endif
 
+#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
+#define DM_WRITECACHE_HAS_PMEM
+#endif
+
+#ifdef DM_WRITECACHE_HAS_PMEM
 #define pmem_assign(dest, src)					\
 do {								\
 	typeof(dest) uniq = (src);				\
 	memcpy_flushcache(&(dest), &uniq, sizeof(dest));	\
 } while (0)
+#else
+#define pmem_assign(dest, src)	((dest)) = (src))
+#endif
 
-#if defined(__HAVE_ARCH_MEMCPY_MCSAFE) && defined(CONFIG_ARCH_HAS_PMEM_API)
+#if defined(__HAVE_ARCH_MEMCPY_MCSAFE) && defined(DM_WRITECACHE_HAS_PMEM)
 #define DM_WRITECACHE_HANDLE_HARDWARE_ERRORS
 #endif
 

@@ -87,8 +95,13 @@ struct wc_entry {
 #endif
 };
 
+#ifdef DM_WRITECACHE_HAS_PMEM
 #define WC_MODE_PMEM(wc)			((wc)->pmem_mode)
 #define WC_MODE_FUA(wc)				((wc)->writeback_fua)
+#else
+#define WC_MODE_PMEM(wc)			false
+#define WC_MODE_FUA(wc)				false
+#endif
 #define WC_MODE_SORT_FREELIST(wc)		(!WC_MODE_PMEM(wc))
 
 struct dm_writecache {

@@ -1857,7 +1870,7 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	if (!strcasecmp(string, "s")) {
 		wc->pmem_mode = false;
 	} else if (!strcasecmp(string, "p")) {
-#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
+#ifdef DM_WRITECACHE_HAS_PMEM
 		wc->pmem_mode = true;
 		wc->writeback_fua = true;
 #else

Signature · 1 line

2.15.0

OK.

I think that persistent_memory_claim should also be conditioned based on DM_WRITECACHE_HAS_PMEM - i.e. if we have DM_WRITECACHE_HAS_PMEM, we don't need to use other ifdefs.

Is there some difference between "#if IS_ENABLED(CONFIG_OPTION)" and "#if defined(CONFIG_OPTION)"?

Quoted 92 lines · Dan Williams

Quoted 59 lines · Mikulas Patocka

Quoted 55 lines · Dan Williams

Quoted 54 lines · Mikulas Patocka

Quoted 16 lines · Dan Williams

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 6 lines

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

memcpy_flushcache on ARM64 is generally wrong thing to do, because it is slower than memcpy and explicit cache flush.

Suppose that you want to write data to a block device and make it persistent. So you send a WRITE bio and then a FLUSH bio.

Now - how to implement these two bios on persistent memory:

On X86, the WRITE bio does memcpy_flushcache() and the FLUSH bio does wmb() - this is the optimal implementation.

But on ARM64, memcpy_flushcache() is suboptimal. On ARM64, the optimal implementation is that the WRITE bio does just memcpy() and the FLUSH bio does arch_wb_cache_pmem() on the affected range.

Why is memcpy_flushcache() is suboptimal on ARM? The ARM architecture doesn't have non-temporal stores. So, memcpy_flushcache() is implemented as memcpy() followed by a cache flush.

Now - if you flush the cache immediatelly after memcpy, the cache is full of dirty lines and the cache-flushing code has to write these lines back and that is slow.

If you flush the cache some time after memcpy (i.e. when the FLUSH bio is received), the processor already flushed some part of the cache on its own, so the cache-flushing function has less work to do and it is faster.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

So the conclusion is - don't use memcpy_flushcache on ARM. This problem cannot be fixed by a better implementation of memcpy_flushcache.

It sounds like ARM might be better off with mapping its pmem as write-through rather than write-back, and skip the explicit cache

I doubt it would perform well - write combining combines the writes into a larger segments - and write-through doesn't.

Last I checked write-through caching does not disable write combining

Quoted 9 lines

Quoted 6 lines

management altogether. You speak of "optimal" and "sub-optimal", but what would be more clear is fio measurements of the relative IOPs and latency profiles of the different approaches. The reason I am continuing to push here is that reducing the operation space from 'copy-flush-commit' to just 'copy' or 'copy-commit' simplifies the maintenance long term.

I measured it (with nvme backing store) and late cache flushing has 12% better performance than eager flushing with memcpy_flushcache().

I assume what you're seeing is ARM64 over-flushing the amount of dirty data so it becomes more efficient to do an amortized flush at the end? However, that effectively makes memcpy_flushcache() unusable in the way it can be used on x86. You claimed that ARM does not support non-temporal stores, but it does, see the STNP instruction. I do not want to see arch specific optimizations in drivers, so either write-through mappings is a potential answer to remove the need to explicitly manage flushing, or just implement STNP hacks in memcpy_flushcache() like you did with MOVNT on x86.

Quoted 3 lines

Quoted 1 line

131836 4k iops - vs - 117016.

To be clear this is memcpy_flushcache() vs memcpy + flush?

I found out what caused the difference. I used dax_flush on the version of dm-writecache that I had on the ARM machine (with the kernel 4.14, because it is the last version where dax on ramdisk works) - and I thought that dax_flush flushes the cache, but it doesn't.

When I replaced dax_flush with arch_wb_cache_pmem, the performance difference between early flushing and late flushing disappeared.

So I think we can remove this per-architecture switch from dm-writecache.

Quoted 108 lines · Mikulas Patocka

Quoted 92 lines · Dan Williams

Quoted 59 lines · Mikulas Patocka

Quoted 55 lines · Dan Williams

Quoted 54 lines · Mikulas Patocka

Quoted 16 lines · Dan Williams

Quoted 15 lines · Mike Snitzer

Quoted 12 lines · Mikulas Patocka

Quoted 9 lines · Dan Williams

I don't want to grow driver-local wrappers for pmem. You should use memcpy_flushcache directly() and if an architecture does not define memcpy_flushcache() then don't allow building dm-writecache, i.e. this driver should 'depends on CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE'. I don't see a need to add a standalone flush operation if all relevant archs provide memcpy_flushcache(). As for commit, I'd say just use wmb() directly since all archs define it. Alternatively we could introduce memcpy_flushcache_relaxed() to be the un-ordered version of the copy routine and memcpy_flushcache() would imply a wmb().

Quoted 6 lines

Quoted 1 line

Quoted 1 line

But memcpy_flushcache() on ARM64 is slow.

Right, so again, what is wrong with memcpy_flushcache_relaxed() + wmb() or otherwise making memcpy_flushcache() ordered. I do not see that as a trailblazing requirement, I see that as typical review and a reduction of the operation space that you are proposing.

memcpy_flushcache on ARM64 is generally wrong thing to do, because it is slower than memcpy and explicit cache flush.

Suppose that you want to write data to a block device and make it persistent. So you send a WRITE bio and then a FLUSH bio.

Now - how to implement these two bios on persistent memory:

On X86, the WRITE bio does memcpy_flushcache() and the FLUSH bio does wmb() - this is the optimal implementation.

But on ARM64, memcpy_flushcache() is suboptimal. On ARM64, the optimal implementation is that the WRITE bio does just memcpy() and the FLUSH bio does arch_wb_cache_pmem() on the affected range.

Why is memcpy_flushcache() is suboptimal on ARM? The ARM architecture doesn't have non-temporal stores. So, memcpy_flushcache() is implemented as memcpy() followed by a cache flush.

Now - if you flush the cache immediatelly after memcpy, the cache is full of dirty lines and the cache-flushing code has to write these lines back and that is slow.

If you flush the cache some time after memcpy (i.e. when the FLUSH bio is received), the processor already flushed some part of the cache on its own, so the cache-flushing function has less work to do and it is faster.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

So the conclusion is - don't use memcpy_flushcache on ARM. This problem cannot be fixed by a better implementation of memcpy_flushcache.

It sounds like ARM might be better off with mapping its pmem as write-through rather than write-back, and skip the explicit cache

I doubt it would perform well - write combining combines the writes into a larger segments - and write-through doesn't.

Last I checked write-through caching does not disable write combining

Quoted 9 lines

Quoted 6 lines

management altogether. You speak of "optimal" and "sub-optimal", but what would be more clear is fio measurements of the relative IOPs and latency profiles of the different approaches. The reason I am continuing to push here is that reducing the operation space from 'copy-flush-commit' to just 'copy' or 'copy-commit' simplifies the maintenance long term.

I measured it (with nvme backing store) and late cache flushing has 12% better performance than eager flushing with memcpy_flushcache().

I assume what you're seeing is ARM64 over-flushing the amount of dirty data so it becomes more efficient to do an amortized flush at the end? However, that effectively makes memcpy_flushcache() unusable in the way it can be used on x86. You claimed that ARM does not support non-temporal stores, but it does, see the STNP instruction. I do not want to see arch specific optimizations in drivers, so either write-through mappings is a potential answer to remove the need to explicitly manage flushing, or just implement STNP hacks in memcpy_flushcache() like you did with MOVNT on x86.

Quoted 3 lines

Quoted 1 line

131836 4k iops - vs - 117016.

To be clear this is memcpy_flushcache() vs memcpy + flush?

I found out what caused the difference. I used dax_flush on the version of dm-writecache that I had on the ARM machine (with the kernel 4.14, because it is the last version where dax on ramdisk works) - and I thought that dax_flush flushes the cache, but it doesn't.

When I replaced dax_flush with arch_wb_cache_pmem, the performance difference between early flushing and late flushing disappeared.

Quoted 1 line

So I think we can remove this per-architecture switch from dm-writecache.

Great find! Thanks for the due diligence. Feel free to add:

Acked-by Dan Williams <[email protected]>

...on the reworks to unify ARM and x86.

Quoted 5 lines

Great find! Thanks for the due diligence. Feel free to add:

Acked-by Dan Williams <[email protected]>

...on the reworks to unify ARM and x86.

One more note. The side effect of not using dax_flush() is that you may end up flushing caches on systems where the platform has asserted it will take responsibility for flushing caches at power loss. If / when those systems become more prevalent we may want to think of a way to combine the non-temporal optimization and the cache-flush-bypass optimizations. However that is something that can wait for a later change beyond 4.18.

Quoted 6 lines · Dan Williams

Quoted 5 lines

Great find! Thanks for the due diligence. Feel free to add:

Acked-by Dan Williams <[email protected]>

...on the reworks to unify ARM and x86.

Quoted 7 lines

One more note. The side effect of not using dax_flush() is that you may end up flushing caches on systems where the platform has asserted it will take responsibility for flushing caches at power loss. If / when those systems become more prevalent we may want to think of a way to combine the non-temporal optimization and the cache-flush-bypass optimizations. However that is something that can wait for a later change beyond 4.18.

We could define memcpy_flushpmem, that falls back to memcpy or memcpy_flushcache, depending on whether the platform flushes the caches at power loss or not.

Quoted 18 lines · Mikulas Patocka

Quoted 13 lines · Dan Williams

Quoted 5 lines

Great find! Thanks for the due diligence. Feel free to add:

Acked-by Dan Williams <[email protected]>

...on the reworks to unify ARM and x86.

One more note. The side effect of not using dax_flush() is that you may end up flushing caches on systems where the platform has asserted it will take responsibility for flushing caches at power loss. If / when those systems become more prevalent we may want to think of a way to combine the non-temporal optimization and the cache-flush-bypass optimizations. However that is something that can wait for a later change beyond 4.18.

Quoted 3 lines

We could define memcpy_flushpmem, that falls back to memcpy or memcpy_flushcache, depending on whether the platform flushes the caches at power loss or not.

The problem is that some platforms only power fail protect a subset of the physical address range,

but yes, if the platform makes a global
assertion we can globally replace memcpy_flushpmem() with plain memcpy.

Quoted 23 lines · Dan Williams

Quoted 21 lines · Mikulas Patocka

Quoted 6 lines · Dan Williams

Quoted 5 lines

Great find! Thanks for the due diligence. Feel free to add:

Acked-by Dan Williams <[email protected]>

...on the reworks to unify ARM and x86.

Quoted 7 lines

One more note. The side effect of not using dax_flush() is that you may end up flushing caches on systems where the platform has asserted it will take responsibility for flushing caches at power loss. If / when those systems become more prevalent we may want to think of a way to combine the non-temporal optimization and the cache-flush-bypass optimizations. However that is something that can wait for a later change beyond 4.18.

We could define memcpy_flushpmem, that falls back to memcpy or memcpy_flushcache, depending on whether the platform flushes the caches at power loss or not.

Quoted 2 lines

The problem is that some platforms only power fail protect a subset of the physical address range,

How can this be? A psysical address may be cached on any CPU, so either there is enough power to flush all the CPUs' caches or there isn't.

How does the CPU design that protects only a part of physical addresses look like?