Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1258449 > unrolled thread

[PATCH 0/2] "big hammer" for DAX msync/fsync correctness

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] "big hammer" for DAX msync/fsync correctness Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-10-28 23:10 +0100
    [PATCH 1/2] pmem: add wb_cache_pmem() to the PMEM API Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-10-28 23:20 +0100
    [PATCH 2/2] pmem: Add simple and slow fsync/msync support Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-10-28 23:20 +0100
      Re: [PATCH 2/2] pmem: Add simple and slow fsync/msync support Dan Williams <dan.j.williams@intel.com> - 2015-10-29 00:10 +0100
    Re: [PATCH 0/2] "big hammer" for DAX msync/fsync correctness Jeff Moyer <jmoyer@redhat.com> - 2015-10-28 23:30 +0100
      Re: [PATCH 0/2] "big hammer" for DAX msync/fsync correctness Dan Williams <dan.j.williams@intel.com> - 2015-10-29 00:00 +0100
      Re: [PATCH 0/2] "big hammer" for DAX msync/fsync correctness Ross Zwisler <ross.zwisler@linux.intel.com> - 2015-10-29 00:00 +0100

#1258449 — [PATCH 0/2] "big hammer" for DAX msync/fsync correctness

This series implements the very slow but correct handling for
blkdev_issue_flush() with DAX mappings, as discussed here:

https://lkml.org/lkml/2015/10/26/116

I don't think that we can actually do the

    on_each_cpu(sync_cache, ...);

...where sync_cache is something like:

    cache_disable();
    wbinvd();
    pcommit();
    cache_enable();

solution as proposed by Dan because WBINVD + PCOMMIT doesn't guarantee that
your writes actually make it durably onto the DIMMs.  I believe you really do
need to loop through the cache lines, flush them with CLWB, then fence and
PCOMMIT.

I do worry that the cost of blindly flushing the entire PMEM namespace on each
fsync or msync will be prohibitively expensive, and that we'll by very
incentivized to move to the radix tree based dirty page tracking as soon as
possible. :)

Ross Zwisler (2):
  pmem: add wb_cache_pmem() to the PMEM API
  pmem: Add simple and slow fsync/msync support

 arch/x86/include/asm/pmem.h | 11 ++++++-----
 drivers/nvdimm/pmem.c       | 10 +++++++++-
 include/linux/pmem.h        | 22 +++++++++++++++++++++-
 3 files changed, 36 insertions(+), 7 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1258450 — [PATCH 1/2] pmem: add wb_cache_pmem() to the PMEM API

The function __arch_wb_cache_pmem() was already an internal implementation
detail of the x86 PMEM API, but this functionality needs to be exported as
part of the general PMEM API to handle the fsync/msync case for DAX mmaps.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 arch/x86/include/asm/pmem.h | 11 ++++++-----
 include/linux/pmem.h        | 22 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/pmem.h b/arch/x86/include/asm/pmem.h
index d8ce3ec..6c7ade0 100644
--- a/arch/x86/include/asm/pmem.h
+++ b/arch/x86/include/asm/pmem.h
@@ -67,18 +67,19 @@ static inline void arch_wmb_pmem(void)
 }
 
 /**
- * __arch_wb_cache_pmem - write back a cache range with CLWB
+ * arch_wb_cache_pmem - write back a cache range with CLWB
  * @vaddr:	virtual start address
  * @size:	number of bytes to write back
  *
  * Write back a cache range using the CLWB (cache line write back)
  * instruction.  This function requires explicit ordering with an
- * arch_wmb_pmem() call.  This API is internal to the x86 PMEM implementation.
+ * arch_wmb_pmem() call.
  */
-static inline void __arch_wb_cache_pmem(void *vaddr, size_t size)
+static inline void arch_wb_cache_pmem(void __pmem *addr, size_t size)
 {
 	u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
 	unsigned long clflush_mask = x86_clflush_size - 1;
+	void *vaddr = (void __force *)addr;
 	void *vend = vaddr + size;
 	void *p;
 
@@ -115,7 +116,7 @@ static inline size_t arch_copy_from_iter_pmem(void __pmem *addr, size_t bytes,
 	len = copy_from_iter_nocache(vaddr, bytes, i);
 
 	if (__iter_needs_pmem_wb(i))
-		__arch_wb_cache_pmem(vaddr, bytes);
+		arch_wb_cache_pmem(addr, bytes);
 
 	return len;
 }
@@ -138,7 +139,7 @@ static inline void arch_clear_pmem(void __pmem *addr, size_t size)
 	else
 		memset(vaddr, 0, size);
 
-	__arch_wb_cache_pmem(vaddr, size);
+	arch_wb_cache_pmem(addr, size);
 }
 
 static inline bool __arch_has_wmb_pmem(void)
diff --git a/include/linux/pmem.h b/include/linux/pmem.h
index 85f810b3..2cd5003 100644
--- a/include/linux/pmem.h
+++ b/include/linux/pmem.h
@@ -53,12 +53,18 @@ static inline void arch_clear_pmem(void __pmem *addr, size_t size)
 {
 	BUG();
 }
+
+static inline void arch_wb_cache_pmem(void __pmem *addr, size_t size)
+{
+	BUG();
+}
 #endif
 
 /*
  * Architectures that define ARCH_HAS_PMEM_API must provide
  * implementations for arch_memcpy_to_pmem(), arch_wmb_pmem(),
- * arch_copy_from_iter_pmem(), arch_clear_pmem() and arch_has_wmb_pmem().
+ * arch_copy_from_iter_pmem(), arch_clear_pmem(), arch_wb_cache_pmem()
+ * and arch_has_wmb_pmem().
  */
 static inline void memcpy_from_pmem(void *dst, void __pmem const *src, size_t size)
 {
@@ -202,4 +208,18 @@ static inline void clear_pmem(void __pmem *addr, size_t size)
 	else
 		default_clear_pmem(addr, size);
 }
+
+/**
+ * wb_cache_pmem - write back processor cache for PMEM memory range
+ * @addr:	virtual start address
+ * @size:	number of bytes to write back
+ *
+ * Write back the processor cache range starting at 'addr' for 'size' bytes.
+ * This function requires explicit ordering with a wmb_pmem() call.
+ */
+static inline void wb_cache_pmem(void __pmem *addr, size_t size)
+{
+	if (arch_has_pmem_api())
+		arch_wb_cache_pmem(addr, size);
+}
 #endif /* __PMEM_H__ */
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258454 — [PATCH 2/2] pmem: Add simple and slow fsync/msync support

Make blkdev_issue_flush() behave correctly according to its required
semantics - all volatile cached data is flushed to stable storage.

Eventually this needs to be replaced with something much more precise by
tracking dirty DAX entries via the radix tree in struct address_space, but
for now this gives us correctness even if the performance is quite bad.

Userspace applications looking to avoid the fsync/msync penalty should
consider more fine-grained flushing via the NVML library:

https://github.com/pmem/nvml

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 drivers/nvdimm/pmem.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 0ba6a97..eea7997 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -80,7 +80,14 @@ static void pmem_make_request(struct request_queue *q, struct bio *bio)
 	if (do_acct)
 		nd_iostat_end(bio, start);
 
-	if (bio_data_dir(bio))
+	if (bio->bi_rw & REQ_FLUSH) {
+		void __pmem *addr = pmem->virt_addr + pmem->data_offset;
+		size_t size = pmem->size - pmem->data_offset;
+
+		wb_cache_pmem(addr, size);
+	}
+
+	if (bio_data_dir(bio) || (bio->bi_rw & REQ_FLUSH))
 		wmb_pmem();
 
 	bio_endio(bio);
@@ -189,6 +196,7 @@ static int pmem_attach_disk(struct device *dev,
 	blk_queue_physical_block_size(pmem->pmem_queue, PAGE_SIZE);
 	blk_queue_max_hw_sectors(pmem->pmem_queue, UINT_MAX);
 	blk_queue_bounce_limit(pmem->pmem_queue, BLK_BOUNCE_ANY);
+	blk_queue_flush(pmem->pmem_queue, REQ_FLUSH);
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, pmem->pmem_queue);
 
 	disk = alloc_disk(0);
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258475 — Re: [PATCH 2/2] pmem: Add simple and slow fsync/msync support

On Thu, Oct 29, 2015 at 7:09 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> Make blkdev_issue_flush() behave correctly according to its required
> semantics - all volatile cached data is flushed to stable storage.
>
> Eventually this needs to be replaced with something much more precise by
> tracking dirty DAX entries via the radix tree in struct address_space, but
> for now this gives us correctness even if the performance is quite bad.
>
> Userspace applications looking to avoid the fsync/msync penalty should
> consider more fine-grained flushing via the NVML library:
>
> https://github.com/pmem/nvml
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  drivers/nvdimm/pmem.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 0ba6a97..eea7997 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -80,7 +80,14 @@ static void pmem_make_request(struct request_queue *q, struct bio *bio)
>         if (do_acct)
>                 nd_iostat_end(bio, start);
>
> -       if (bio_data_dir(bio))
> +       if (bio->bi_rw & REQ_FLUSH) {
> +               void __pmem *addr = pmem->virt_addr + pmem->data_offset;
> +               size_t size = pmem->size - pmem->data_offset;
> +
> +               wb_cache_pmem(addr, size);
> +       }
> +

So I think this will be too expensive to run synchronously in the
submission path for very large pmem ranges and should be farmed out to
an async thread. Then, as long as we're farming it out, might as well
farm it out to more than one cpu.  I'll take a stab at this on the
flight back from KS.

Another optimization is that we can make the flush a nop up until
pmem_direct_access() is first called, because we know there is nothing
to flush when all the i/o is coming through the driver.  That at least
helps the "pmem as a fast SSD" use case avoid the overhead.

Bikeshed alert... wb_cache_pmem() should probably become
mmio_wb_cache() and live next to mmio_flush_cache() since it is not
specific to persistent memory.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258455

Ross Zwisler <ross.zwisler@linux.intel.com> writes:

> This series implements the very slow but correct handling for
> blkdev_issue_flush() with DAX mappings, as discussed here:
>
> https://lkml.org/lkml/2015/10/26/116
>
> I don't think that we can actually do the
>
>     on_each_cpu(sync_cache, ...);
>
> ...where sync_cache is something like:
>
>     cache_disable();
>     wbinvd();
>     pcommit();
>     cache_enable();
>
> solution as proposed by Dan because WBINVD + PCOMMIT doesn't guarantee that
> your writes actually make it durably onto the DIMMs.  I believe you really do
> need to loop through the cache lines, flush them with CLWB, then fence and
> PCOMMIT.

*blink*
*blink*

So much for not violating the principal of least surprise.  I suppose
you've asked the hardware folks, and they've sent you down this path?

> I do worry that the cost of blindly flushing the entire PMEM namespace on each
> fsync or msync will be prohibitively expensive, and that we'll by very
> incentivized to move to the radix tree based dirty page tracking as soon as
> possible. :)

Sure, but wbinvd would be quite costly as well.  Either way I think a
better solution will be required in the near term.

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258463

On Thu, Oct 29, 2015 at 7:24 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Ross Zwisler <ross.zwisler@linux.intel.com> writes:
>
>> This series implements the very slow but correct handling for
>> blkdev_issue_flush() with DAX mappings, as discussed here:
>>
>> https://lkml.org/lkml/2015/10/26/116
>>
>> I don't think that we can actually do the
>>
>>     on_each_cpu(sync_cache, ...);
>>
>> ...where sync_cache is something like:
>>
>>     cache_disable();
>>     wbinvd();
>>     pcommit();
>>     cache_enable();
>>
>> solution as proposed by Dan because WBINVD + PCOMMIT doesn't guarantee that
>> your writes actually make it durably onto the DIMMs.  I believe you really do
>> need to loop through the cache lines, flush them with CLWB, then fence and
>> PCOMMIT.
>
> *blink*
> *blink*
>
> So much for not violating the principal of least surprise.  I suppose
> you've asked the hardware folks, and they've sent you down this path?

The SDM states that wbinvd only asynchronously "signals" L3 to flush.

>> I do worry that the cost of blindly flushing the entire PMEM namespace on each
>> fsync or msync will be prohibitively expensive, and that we'll by very
>> incentivized to move to the radix tree based dirty page tracking as soon as
>> possible. :)
>
> Sure, but wbinvd would be quite costly as well.  Either way I think a
> better solution will be required in the near term.
>

As Peter points out the irqoff latency that wbinvd introduces also
makes it not optimal.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258466

On Wed, Oct 28, 2015 at 06:24:29PM -0400, Jeff Moyer wrote:
> Ross Zwisler <ross.zwisler@linux.intel.com> writes:
> 
> > This series implements the very slow but correct handling for
> > blkdev_issue_flush() with DAX mappings, as discussed here:
> >
> > https://lkml.org/lkml/2015/10/26/116
> >
> > I don't think that we can actually do the
> >
> >     on_each_cpu(sync_cache, ...);
> >
> > ...where sync_cache is something like:
> >
> >     cache_disable();
> >     wbinvd();
> >     pcommit();
> >     cache_enable();
> >
> > solution as proposed by Dan because WBINVD + PCOMMIT doesn't guarantee that
> > your writes actually make it durably onto the DIMMs.  I believe you really do
> > need to loop through the cache lines, flush them with CLWB, then fence and
> > PCOMMIT.
> 
> *blink*
> *blink*
> 
> So much for not violating the principal of least surprise.  I suppose
> you've asked the hardware folks, and they've sent you down this path?

Sadly, yes, this was the guidance from the hardware folks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web