another pmem variant V2

From: Ross Zwisler <[email protected]>

PMEM is a new driver that presents a reserved range of memory as a block device. This is useful for developing with NV-DIMMs, and can be used with volatile memory as a development platform.

Signed-off-by Ross Zwisler <[email protected]>

[hch: convert to use a platform_device for discovery, fix partition
support]

Signed-off-by Christoph Hellwig <[email protected]>

Tested-by Ross Zwisler <[email protected]>

---
 MAINTAINERS            |   6 +
 drivers/block/Kconfig  |  13 ++
 drivers/block/Makefile |   1 +
 drivers/block/pmem.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 393 insertions(+)
 create mode 100644 drivers/block/pmem.c

MAINTAINERS +6 -0

index 358eb01..efacf2b 100644
@@ -8063,6 +8063,12 @@ S:       Maintained
 F:     Documentation/blockdev/ramdisk.txt
 F:     drivers/block/brd.c
 
+PERSISTENT MEMORY DRIVER
+M:     Ross Zwisler <[email protected]>
+L:     [email protected]
+S:     Supported
+F:     drivers/block/pmem.c
+
 RANDOM NUMBER DRIVER
 M:     "Theodore Ts'o" <[email protected]>
 S:     Maintained

drivers/block/Kconfig +13 -0

index 1b8094d..9284aaf 100644
@@ -404,6 +404,19 @@ config BLK_DEV_RAM_DAX
          and will prevent RAM block device backing store memory from being
          allocated from highmem (only a problem for highmem systems).
 
+config BLK_DEV_PMEM
+       tristate "Persistent memory block device support"
+       help
+         Saying Y here will allow you to use a contiguous range of reserved
+         memory as one or more block devices.  Memory for PMEM should be
+         reserved using the "memmap" kernel parameter.
+
+         To compile this driver as a module, choose M here: the module will be
+         called pmem.
+
+         Most normal users won't need this functionality, and can thus say N
+         here.
+
 config CDROM_PKTCDVD
        tristate "Packet writing on CD/DVD media"
        depends on !UML

drivers/block/Makefile +1 -0

index 02b688d..9cc6c18 100644
@@ -14,6 +14,7 @@ obj-$(CONFIG_PS3_VRAM)                += ps3vram.o
 obj-$(CONFIG_ATARI_FLOPPY)     += ataflop.o
 obj-$(CONFIG_AMIGA_Z2RAM)      += z2ram.o
 obj-$(CONFIG_BLK_DEV_RAM)      += brd.o
+obj-$(CONFIG_BLK_DEV_PMEM)     += pmem.o
 obj-$(CONFIG_BLK_DEV_LOOP)     += loop.o
 obj-$(CONFIG_BLK_CPQ_DA)       += cpqarray.o
 obj-$(CONFIG_BLK_CPQ_CISS_DA)  += cciss.o

drivers/block/pmem.c new file +219 -0

new file mode 100644
index 0000000..545b13b
@@ -0,0 +1,373 @@
+/*
+ * Persistent Memory Driver
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This driver is heavily based on drivers/block/brd.c.
+ * Copyright (C) 2007 Nick Piggin
+ * Copyright (C) 2007 Novell Inc.
+ */
+
+#include <asm/cacheflush.h>
+#include <linux/blkdev.h>
+#include <linux/hdreg.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+
+#define SECTOR_SHIFT           9
+#define PAGE_SECTORS_SHIFT     (PAGE_SHIFT - SECTOR_SHIFT)
+#define PAGE_SECTORS           (1 << PAGE_SECTORS_SHIFT)
+
+#define PMEM_MINORS            16
+
+struct pmem_device {
+       struct request_queue    *pmem_queue;
+       struct gendisk          *pmem_disk;
+
+       /* One contiguous memory region per device */
+       phys_addr_t             phys_addr;
+       void                    *virt_addr;
+       size_t                  size;
+};
+
+static int pmem_major;
+static atomic_t pmem_index;
+
+static int pmem_getgeo(struct block_device *bd, struct hd_geometry *geo)
+{
+       /* some standard values */
+       geo->heads = 1 << 6;
+       geo->sectors = 1 << 5;
+       geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+       return 0;
+}
+
+/*
+ * direct translation from (pmem,sector) => void*
+ * We do not require that sector be page aligned.
+ * The return value will point to the beginning of the page containing the
+ * given sector, not to the sector itself.
+ */
+static void *pmem_lookup_pg_addr(struct pmem_device *pmem, sector_t sector)
+{
+       size_t page_offset = sector >> PAGE_SECTORS_SHIFT;
+       size_t offset = page_offset << PAGE_SHIFT;
+
+       BUG_ON(offset >= pmem->size);
+       return pmem->virt_addr + offset;
+}
+
+/* sector must be page aligned */
+static unsigned long pmem_lookup_pfn(struct pmem_device *pmem, sector_t sector)
+{
+       size_t page_offset = sector >> PAGE_SECTORS_SHIFT;
+
+       BUG_ON(sector & (PAGE_SECTORS - 1));
+       return (pmem->phys_addr >> PAGE_SHIFT) + page_offset;
+}
+
+/*
+ * sector is not required to be page aligned.
+ * n is at most a single page, but could be less.
+ */
+static void copy_to_pmem(struct pmem_device *pmem, const void *src,
+                       sector_t sector, size_t n)
+{
+       void *dst;
+       unsigned int offset = (sector & (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
+       size_t copy;
+
+       BUG_ON(n > PAGE_SIZE);
+
+       copy = min_t(size_t, n, PAGE_SIZE - offset);
+       dst = pmem_lookup_pg_addr(pmem, sector);
+       memcpy(dst + offset, src, copy);
+
+       if (copy < n) {
+               src += copy;
+               sector += copy >> SECTOR_SHIFT;
+               copy = n - copy;
+               dst = pmem_lookup_pg_addr(pmem, sector);
+               memcpy(dst, src, copy);
+       }
+}
+
+/*
+ * sector is not required to be page aligned.
+ * n is at most a single page, but could be less.
+ */
+static void copy_from_pmem(void *dst, struct pmem_device *pmem,
+                         sector_t sector, size_t n)
+{
+       void *src;
+       unsigned int offset = (sector & (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
+       size_t copy;
+
+       BUG_ON(n > PAGE_SIZE);
+
+       copy = min_t(size_t, n, PAGE_SIZE - offset);
+       src = pmem_lookup_pg_addr(pmem, sector);
+
+       memcpy(dst, src + offset, copy);
+
+       if (copy < n) {
+               dst += copy;
+               sector += copy >> SECTOR_SHIFT;
+               copy = n - copy;
+               src = pmem_lookup_pg_addr(pmem, sector);
+               memcpy(dst, src, copy);
+       }
+}
+
+static void pmem_do_bvec(struct pmem_device *pmem, struct page *page,
+                       unsigned int len, unsigned int off, int rw,
+                       sector_t sector)
+{
+       void *mem = kmap_atomic(page);
+
+       if (rw == READ) {
+               copy_from_pmem(mem + off, pmem, sector, len);
+               flush_dcache_page(page);
+       } else {
+               /*
+                * FIXME: Need more involved flushing to ensure that writes to
+                * NVDIMMs are actually durable before returning.
+                */
+               flush_dcache_page(page);
+               copy_to_pmem(pmem, mem + off, sector, len);
+       }
+
+       kunmap_atomic(mem);
+}
+
+static void pmem_make_request(struct request_queue *q, struct bio *bio)
+{
+       struct block_device *bdev = bio->bi_bdev;
+       struct pmem_device *pmem = bdev->bd_disk->private_data;
+       int rw;
+       struct bio_vec bvec;
+       sector_t sector;
+       struct bvec_iter iter;
+       int err = 0;
+
+       sector = bio->bi_iter.bi_sector;
+       if (bio_end_sector(bio) > get_capacity(bdev->bd_disk)) {
+               err = -EIO;
+               goto out;
+       }
+
+       BUG_ON(bio->bi_rw & REQ_DISCARD);
+
+       rw = bio_rw(bio);
+       if (rw == READA)
+               rw = READ;
+
+       bio_for_each_segment(bvec, bio, iter) {
+               unsigned int len = bvec.bv_len;
+
+               BUG_ON(len > PAGE_SIZE);
+               pmem_do_bvec(pmem, bvec.bv_page, len,
+                           bvec.bv_offset, rw, sector);
+               sector += len >> SECTOR_SHIFT;
+       }
+
+out:
+       bio_endio(bio, err);
+}
+
+static int pmem_rw_page(struct block_device *bdev, sector_t sector,
+                      struct page *page, int rw)
+{
+       struct pmem_device *pmem = bdev->bd_disk->private_data;
+
+       pmem_do_bvec(pmem, page, PAGE_CACHE_SIZE, 0, rw, sector);
+       page_endio(page, rw & WRITE, 0);
+       return 0;
+}
+
+static long pmem_direct_access(struct block_device *bdev, sector_t sector,
+                             void **kaddr, unsigned long *pfn, long size)
+{
+       struct pmem_device *pmem = bdev->bd_disk->private_data;
+
+       if (!pmem)
+               return -ENODEV;
+
+       *kaddr = pmem_lookup_pg_addr(pmem, sector);
+       *pfn = pmem_lookup_pfn(pmem, sector);
+
+       return pmem->size - (sector * 512);
+}
+
+static const struct block_device_operations pmem_fops = {
+       .owner =                THIS_MODULE,
+       .rw_page =              pmem_rw_page,
+       .direct_access =        pmem_direct_access,
+       .getgeo =               pmem_getgeo,
+};
+

drivers/block/pmem.c line 229

Quoted 5 lines

drivers/block/pmem.c new file +5 -0

+       struct resource *res_mem;
+       int err;
+
+       res_mem = request_mem_region_exclusive(pmem->phys_addr, pmem->size,
+                                              "pmem");

Isn't request_mem_region() enough? i.e. it seems request_mem_region_exclusive() assumes no DAX, at least in theory?

This is fine for now, but I think we're going to end up with a continuum of solutions to this problem based on the platform and the device. Some ADR platforms have firmware that takes actions like flushing caches on a "power going away" signal. Other platforms have cache management instructions that we can use on either a per-i/o or per REQ_FUA/FLUSH request. Hmm, with this being in the memory map by default I think this poses a challenge for VIVT caches and aliased accesses? We can revisit this when arm support shows up.

Quoted 8 lines · Dan Williams

drivers/block/pmem.c line 229

Quoted 5 lines

drivers/block/pmem.c new file +5 -0

+       struct resource *res_mem;
+       int err;
+
+       res_mem = request_mem_region_exclusive(pmem->phys_addr, pmem->size,
+                                              "pmem");

Isn't request_mem_region() enough? i.e. it seems request_mem_region_exclusive() assumes no DAX, at least in theory?

This is 1:1 from the patch Ross sent, but I've been wondering why request_mem_region_exclusive is used here. All it does is setting the IORESOURCE_EXCLUSIVE flag, which prevents /dev/mem and sysfs from accessing the memory while the driver claims it. Besides pmem only a watchdog driver and e1000 make use of this flag, and there's various function related to it that are entirely unused. It's a weird beast.

Quoted 2 lines · Christoph Hellwig

From: Ross Zwisler <[email protected]>

Quoted 7 lines

PMEM is a new driver that presents a reserved range of memory as a block device. This is useful for developing with NV-DIMMs, and can be used with volatile memory as a development platform.

Signed-off-by Ross Zwisler <[email protected]>

[hch: convert to use a platform_device for discovery, fix partition
support]

Quoted 3 lines · Dan Williams

Quoted 2 lines · Christoph Hellwig

From: Ross Zwisler <[email protected]>

Dan something is Broken with you mailer program it keeps dropping the CC when sending replies.

For example Both me and Ross who were on CC got dropped, Jens Axboe though got add back.

Its not only this email, it is all the emails in this series, please check what is going on.

This will allow to deal with persistent memory which needs to be treated like ram in many, but not all cases.

Based on an earlier patch from Dave Jiang <[email protected]>.

Signed-off-by Christoph Hellwig <[email protected]>

Tested-by Ross Zwisler <[email protected]>

---
 arch/x86/kernel/e820.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

arch/x86/kernel/e820.c +8 -4

index 46201de..de088e3 100644
@@ -48,6 +48,11 @@ unsigned long pci_mem_start = 0xaeedbabe;
 EXPORT_SYMBOL(pci_mem_start);
 #endif
 
+static inline bool is_e820_ram(__u32 type)
+{
+	return type == E820_RAM;
+}
+
 /*
  * This function checks if any part of the range <start,end> is mapped
  * with type.

@@ -688,7 +693,7 @@ void __init e820_mark_nosave_regions(unsigned long limit_pfn)
 			register_nosave_region(pfn, PFN_UP(ei->addr));
 
 		pfn = PFN_DOWN(ei->addr + ei->size);
-		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
+		if (!is_e820_ram(ei->type) && ei->type != E820_RESERVED_KERN)
 			register_nosave_region(PFN_UP(ei->addr), pfn);
 
 		if (pfn >= limit_pfn)

@@ -748,7 +753,7 @@ u64 __init early_reserve_e820(u64 size, u64 align)
 /*
  * Find the highest page frame number we have available
  */
-static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
+static unsigned long __init e820_end_pfn(unsigned long limit_pfn)
 {
 	int i;
 	unsigned long last_pfn = 0;

@@ -759,7 +764,7 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 		unsigned long start_pfn;
 		unsigned long end_pfn;
 
-		if (ei->type != type)
+		if (!is_e820_ram(ei->type))
 			continue;
 
 		start_pfn = ei->addr >> PAGE_SHIFT;

@@ -784,12 +789,12 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 }
 unsigned long __init e820_end_of_ram_pfn(void)
 {
-	return e820_end_pfn(MAX_ARCH_PFN, E820_RAM);

This is_e820_ram() factoring out becomes really messy in patch #3.

So you left out a bunch of places making comparisons with E820_RAM, notably e820_reserve_resources_late() and memblock_x86_fill() - and of course those have to be left out, otherwise NVRAM might be registered and used as real kernel RAM!

And this shows the weakness and confusion caused by the factoring out of is_e820_ram() and then adding E820_PMEM to its definition...

I'd rather you add explicit checks to E820_PMEM (why not E820_PRAM, to keep in line with the E820_RAM name?), and not lie about is_e820_ram(). It should result in the exact same end result, with less confusion.

I have no fundamental objections to the driver otherwise.

From 4a6fdc8433559d649d7bf707f72aafa4488f2d23 Mon Sep 17 00:00:00 2001

Quoted 10 lines · Ingo Molnar

This is_e820_ram() factoring out becomes really messy in patch #3.

So you left out a bunch of places making comparisons with E820_RAM, notably e820_reserve_resources_late() and memblock_x86_fill() - and of course those have to be left out, otherwise NVRAM might be registered and used as real kernel RAM!

And this shows the weakness and confusion caused by the factoring out of is_e820_ram() and then adding E820_PMEM to its definition...

Quoted 6 lines

I'd rather you add explicit checks to E820_PMEM (why not E820_PRAM, to keep in line with the E820_RAM name?), and not lie about is_e820_ram(). It should result in the exact same end result, with less confusion.

I have no fundamental objections to the driver otherwise.

Does this patch (replaces patches 2 and 3) look better to you?

Yeah, the code is much clearer now:

Acked-by Ingo Molnar <[email protected]>

What tree is this intended for? Should I pick up the x86 bits?

Quoted 5 lines · Ingo Molnar

Yeah, the code is much clearer now:

Acked-by Ingo Molnar <[email protected]>

What tree is this intended for? Should I pick up the x86 bits?

The x86 bits really need to go through the x86 tree. The pmem driver itself would normally go through the block tree, but if Jens is fine with it sending it through the x86 tree as well would allow us to have a single complete tree for testing.

btw., there's half a dozen block drivers in arch/* platform code, so in theory even the block driver could be merged there - but I agree that it's much cleaner and more generic in drivers/block/.

From: Christoph Hellwig <[email protected]>
Date: Wed, 25 Mar 2015 12:24:11 +0100
Subject: x86: add support for the non-standard protected e820 type

Various recent BIOSes support NVDIMMs or ADR using a non-standard e820 memory type, and Intel supplied reference Linux code using this type to various vendors.

Wire this e820 table type up to export platform devices for the pmem driver so that we can use it in Linux, and also provide a memmap= argument to manually tag memory as protected, which can be used if the BIOSs doesn't use the standard nonstandard interface, or we just want to test the pmem driver with regular memory.

Based on an earlier patch from Dave Jiang <[email protected]>

Signed-off-by Christoph Hellwig <[email protected]>

Tested-by Ross Zwisler <[email protected]>

---
 Documentation/kernel-parameters.txt |  6 ++++
 arch/x86/Kconfig                    | 10 ++++++
 arch/x86/include/asm/setup.h        |  6 ++++
 arch/x86/include/uapi/asm/e820.h    | 10 ++++++
 arch/x86/kernel/Makefile            |  1 +
 arch/x86/kernel/e820.c              | 31 ++++++++++++----
 arch/x86/kernel/pmem.c              | 70 +++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/setup.c             |  2 ++
 8 files changed, 130 insertions(+), 6 deletions(-)
 create mode 100644 arch/x86/kernel/pmem.c

Documentation/kernel-parameters.txt +0 -0

index bfcb1a6..c87122d 100644

Documentation/kernel-parameters.txt +0 -0

 	memory_corruption_check=0/1 [X86]
 			Some BIOSes seem to corrupt the first 64k of
 			memory when doing things like suspend/resume.

arch/x86/Kconfig +10 -0

index b7d31ca..c0e8ee3 100644
@@ -1430,6 +1430,16 @@ config ILLEGAL_POINTER_VALUE
 
 source "mm/Kconfig"
 
+config X86_PMEM_LEGACY
+	bool "Support non-standard NVDIMMs and ADR protected memory"
+	help
+	  Treat memory marked using the non-standard e820 type of 12 as used
+	  by the Intel Sandy Bridge-EP reference BIOS as protected memory.
+	  The kernel will offer these regions to the pmem driver so
+	  they can be used for persistent storage.
+
+	  Say Y if unsure.
+
 config HIGHPTE
 	bool "Allocate 3rd-level pagetables from highmem"
 	depends on HIGHMEM

arch/x86/include/asm/setup.h +6 -0

index ff4e7b2..2352fde 100644
@@ -57,6 +57,12 @@ extern void x86_ce4100_early_setup(void);
 static inline void x86_ce4100_early_setup(void) { }
 #endif
 
+#ifdef CONFIG_X86_PMEM_LEGACY
+void reserve_pmem(void);
+#else
+static inline void reserve_pmem(void) { }
+#endif
+
 #ifndef _SETUP
 
 #include <asm/espfix.h>

arch/x86/include/uapi/asm/e820.h +0 -0

index d993e33..ce0d0bf 100644
@@ -33,6 +33,16 @@
 #define E820_NVS	4
 #define E820_UNUSABLE	5
 

arch/x86/include/uapi/asm/e820.h +0 -0

 /*
  * reserved RAM used by kernel itself

arch/x86/kernel/Makefile +1 -0

index cdb1b70..971f18c 100644
@@ -94,6 +94,7 @@ obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvmclock.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o
 obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= paravirt-spinlocks.o
 obj-$(CONFIG_PARAVIRT_CLOCK)	+= pvclock.o
+obj-$(CONFIG_X86_PMEM_LEGACY)	+= pmem.o
 
 obj-$(CONFIG_PCSPKR_PLATFORM)	+= pcspeaker.o
 

arch/x86/kernel/e820.c +0 -0

index 46201de..4bd525a 100644

arch/x86/kernel/e820.c +20 -5

+		break;
 	default:
 		printk(KERN_CONT "type %u", type);
 		break;

@@ -688,8 +691,15 @@ void __init e820_mark_nosave_regions(unsigned long limit_pfn)
 			register_nosave_region(pfn, PFN_UP(ei->addr));
 
 		pfn = PFN_DOWN(ei->addr + ei->size);
-		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
+
+		switch (ei->type) {
+		case E820_RAM:
+		case E820_PRAM:
+		case E820_RESERVED_KERN:
+			break;
+		default:
 			register_nosave_region(PFN_UP(ei->addr), pfn);
+		}
 
 		if (pfn >= limit_pfn)
 			break;

@@ -748,7 +758,7 @@ u64 __init early_reserve_e820(u64 size, u64 align)
 /*
  * Find the highest page frame number we have available
  */
-static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
+static unsigned long __init e820_end_pfn(unsigned long limit_pfn)
 {
 	int i;
 	unsigned long last_pfn = 0;

@@ -759,7 +769,11 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 		unsigned long start_pfn;
 		unsigned long end_pfn;
 
-		if (ei->type != type)
+		/*
+		 * Persistent memory is accounted as ram for purposes of
+		 * establishing max_pfn and mem_map.
+		 */
+		if (ei->type != E820_RAM && ei->type != E820_PRAM)
 			continue;
 
 		start_pfn = ei->addr >> PAGE_SHIFT;

@@ -784,12 +798,12 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 }
 unsigned long __init e820_end_of_ram_pfn(void)
 {
-	return e820_end_pfn(MAX_ARCH_PFN, E820_RAM);
+	return e820_end_pfn(MAX_ARCH_PFN);
 }
 
 unsigned long __init e820_end_of_low_ram_pfn(void)
 {
-	return e820_end_pfn(1UL<<(32 - PAGE_SHIFT), E820_RAM);
+	return e820_end_pfn(1UL<<(32 - PAGE_SHIFT));
 }
 
 static void early_panic(char *msg)

@@ -866,6 +880,9 @@ static int __init parse_memmap_one(char *p)
 	} else if (*p == '$') {
 		start_at = memparse(p+1, &p);
 		e820_add_region(start_at, mem_size, E820_RESERVED);
+	} else if (*p == '!') {
+		start_at = memparse(p+1, &p);
+		e820_add_region(start_at, mem_size, E820_PRAM);
 	} else
 		e820_remove_range(mem_size, ULLONG_MAX - mem_size, E820_RAM, 1);
 

arch/x86/kernel/e820.c +2 -1

 	default:	return "reserved";
 	}
 }

@@ -941,7 +959,8 @@ void __init e820_reserve_resources(void)
 		 * pcibios_resource_survey()
 		 */
 		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20)) {
-			res->flags |= IORESOURCE_BUSY;
+			if (e820.map[i].type != E820_PRAM)
+				res->flags |= IORESOURCE_BUSY;
 			insert_resource(&iomem_resource, res);
 		}
 		res++;

arch/x86/kernel/pmem.c new file +48 -0

new file mode 100644
index 0000000..f970048
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2009, Intel Corporation.
+ * Copyright (c) 2015, Christoph Hellwig.
+ */
+#include <linux/memblock.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <asm/e820.h>
+#include <asm/page_types.h>
+#include <asm/setup.h>
+
+void __init reserve_pmem(void)
+{
+	int i;
+
+	for (i = 0; i < e820.nr_map; i++) {
+		struct e820entry *ei = &e820.map[i];
+
+		if (ei->type != E820_PRAM)
+			continue;
+
+		memblock_reserve(ei->addr, ei->addr + ei->size);
+		max_pfn_mapped = init_memory_mapping(
+				ei->addr < 1UL << 32 ? 1UL << 32 : ei->addr,
+				ei->addr + ei->size);
+	}
+}
+
+static __init void register_pmem_device(struct resource *res)
+{
+	struct platform_device *pdev;
+	int error;
+
+	pdev = platform_device_alloc("pmem", PLATFORM_DEVID_AUTO);
+	if (!pdev)
+		return;
+
+	error = platform_device_add_resources(pdev, res, 1);
+	if (error)
+		goto out_put_pdev;
+
+	error = platform_device_add(pdev);
+	if (error)
+		goto out_put_pdev;
+	return;
+out_put_pdev:
+	dev_warn(&pdev->dev, "failed to add pmem device!\n");
+	platform_device_put(pdev);

arch/x86/kernel/pmem.c new file +12 -0

+			struct resource res = {
+				.flags	= IORESOURCE_MEM,
+				.start	= ei->addr,
+				.end	= ei->addr + ei->size - 1,
+			};
+			register_pmem_device(&res);
+		}
+	}
+
+	return 0;
+}
+device_initcall(register_pmem_devices);

arch/x86/kernel/setup.c +2 -0

index 0a2421c..f2bed2b 100644
@@ -1158,6 +1158,8 @@ void __init setup_arch(char **cmdline_p)
 
 	early_acpi_boot_init();
 
+	reserve_pmem();
+
 	initmem_init();
 	dma_contiguous_reserve(max_pfn_mapped << PAGE_SHIFT);
 

arch/x86/include/uapi/asm/e820.h line 45

Quoted 10 lines · Christoph Hellwig

arch/x86/include/uapi/asm/e820.h +10 -0

+/*
+ * This is a non-standardized way to represent ADR or NVDIMM regions that
+ * persist over a reboot.  The kernel will ignore their special capabilities
+ * unless the CONFIG_X86_PMEM_LEGACY option is set.
+ *
+ * Note that older platforms also used 6 for the same type of memory,
+ * but newer versions switched to 12 as 6 was assigned differently.  Some
+ * time they will learn..
+ */
+#define E820_PRAM    12

Why the PRAM Name. For one 2/3 of this patch say PMEM the Kconfig to enable is _PMEM_, the driver stack that gets loaded is pmem, so PRAM is unexpected.

Also I do believe PRAM is not the correct name. Yes NvDIMMs are RAM, but there are other not RAM technologies that can be supported exactly the same way.
MEM is a more general name meaning "on the memory bus". I think.

I would love the consistency.

Quoted 21 lines · Boaz Harrosh

arch/x86/include/uapi/asm/e820.h line 45

Quoted 10 lines · Christoph Hellwig

arch/x86/include/uapi/asm/e820.h +10 -0

+/*
+ * This is a non-standardized way to represent ADR or NVDIMM regions that
+ * persist over a reboot.  The kernel will ignore their special capabilities
+ * unless the CONFIG_X86_PMEM_LEGACY option is set.
+ *
+ * Note that older platforms also used 6 for the same type of memory,
+ * but newer versions switched to 12 as 6 was assigned differently.  Some
+ * time they will learn..
+ */
+#define E820_PRAM    12

Why the PRAM Name. For one 2/3 of this patch say PMEM the Kconfig to enable is _PMEM_, the driver stack that gets loaded is pmem, so PRAM is unexpected.

Also I do believe PRAM is not the correct name. Yes NvDIMMs are RAM, but there are other not RAM technologies that can be supported exactly the same way.
MEM is a more general name meaning "on the memory bus". I think.

Quoted 1 line

I would love the consistency.

One of nice side of effects of having a "PRAM" name is that we can later add a UEFI "PMEM" type where the distinction is thsy "PRAM" is included in the system memory map by default and "PMEM" is analogous to "IOMEM". Just a thought...

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 1973

Quoted 6 lines

Documentation/kernel-parameters.txt +6 -0

+	memmap=nn[KMG]!ss[KMG]
+			[KNL,X86] Mark specific memory as protected.
+			Region of memory to be used, from ss to ss+nn.
+			The memory region may be marked as e820 type 12 (0xc)
+			and is NVDIMM or ADR memory.
+

Do we need to escape "\!" this character on grub command line ? It might help to note that. I did like the original "|" BTW

Quoted 1 line

+#define E820_PRAM 12

Why the PRAM Name. For one 2/3 of this patch say PMEM the Kconfig to enable is _PMEM_, the driver stack that gets loaded is pmem, so PRAM is unexpected.

Also I do believe PRAM is not the correct name. Yes NvDIMMs are RAM, but there are other not RAM technologies that can be supported exactly the same way.
MEM is a more general name meaning "on the memory bus". I think.

I would love the consistency.

arch/x86/kernel/e820.c static void __init e820_print_type(u32 type) · line 153

Quoted 2 lines

arch/x86/kernel/e820.c +2 -0

+	case E820_PRAM:
+		printk(KERN_CONT "persistent (type %u)", type);

This case can only mean 12 in this patch. (I think historically you had a Kconfig to set its Number

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Christoph Hellwig
Sent: Thursday, March 26, 2015 11:43 AM
To: Boaz Harrosh
Cc: Christoph Hellwig; Ingo Molnar; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/3] x86: add a is_e820_ram() helper

Quoted 9 lines · Boaz Harrosh

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 1973

Quoted 6 lines

Documentation/kernel-parameters.txt +6 -0

+	memmap=nn[KMG]!ss[KMG]
+			[KNL,X86] Mark specific memory as protected.
+			Region of memory to be used, from ss to ss+nn.
+			The memory region may be marked as e820 type 12 (0xc)
+			and is NVDIMM or ADR memory.
+

Do we need to escape "\!" this character on grub command line ? It might help to note that. I did like the original "|" BTW

No need to escape it on the kvm command line, which is where I tested this flag only so far. If there is a strong argument for "|" I'm happy to change it.

Quoted 23 lines

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Christoph Hellwig
Sent: Thursday, March 26, 2015 11:43 AM
To: Boaz Harrosh
Cc: Christoph Hellwig; Ingo Molnar; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/3] x86: add a is_e820_ram() helper

Quoted 9 lines · Boaz Harrosh

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 1973

Quoted 6 lines

+ memmap=nn[KMG]!ss[KMG]

Documentation/kernel-parameters.txt +5 -0

+                 [KNL,X86] Mark specific memory as protected.
+                 Region of memory to be used, from ss to ss+nn.
+                 The memory region may be marked as e820 type 12 (0xc)
+                 and is NVDIMM or ADR memory.
+

Do we need to escape "\!" this character on grub command line ? It might help to note that. I did like the original "|" BTW

Quoted 3 lines

No need to escape it on the kvm command line, which is where I tested this flag only so far. If there is a strong argument for "|" I'm happy to change it.

I agree with Boaz that ! is a nuisance if loading pmem as a module with modprobe from bash.

Quoted 6 lines · Boaz Harrosh

Quoted 1 line

+#define E820_PRAM 12

Why the PRAM Name. For one 2/3 of this patch say PMEM the Kconfig to enable is _PMEM_, the driver stack that gets loaded is pmem, so PRAM is unexpected.

Quoted 6 lines

Also I do believe PRAM is not the correct name. Yes NvDIMMs are RAM, but there are other not RAM technologies that can be supported exactly the same way.
MEM is a more general name meaning "on the memory bus". I think.

I would love the consistency.

Ingo asked for the PRAM name, I don't really care either way.

Quoted 1 line · Ingo Molnar

This is_e820_ram() factoring out becomes really messy in patch #3.

Does this patch (replaces patches 2 and 3) look better to you?

---
From 4a6fdc8433559d649d7bf707f72aafa4488f2d23 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Wed, 25 Mar 2015 12:24:11 +0100
Subject: x86: add support for the non-standard protected e820 type

Various recent BIOSes support NVDIMMs or ADR using a non-standard e820 memory type, and Intel supplied reference Linux code using this type to various vendors.

Wire this e820 table type up to export platform devices for the pmem driver so that we can use it in Linux, and also provide a memmap= argument to manually tag memory as protected, which can be used if the BIOSs doesn't use the standard nonstandard interface, or we just want to test the pmem driver with regular memory.

Based on an earlier patch from Dave Jiang <[email protected]>

arch/x86/kernel/pmem.c new file +0 -0

new file mode 100644
index 0000000..f970048

arch/x86/kernel/pmem.c new file +7 -0

+
+void __init reserve_pmem(void)
+{
+       int i;
+
+       for (i = 0; i < e820.nr_map; i++) {
+               struct e820entry *ei = &e820.map[i];

What do you want to get here?

You did not modify memblock_x86_fill() to treat E820_PRAM as E820_RAM, so memblock will not have any entry for E820_PRAM, so you do not need to call memblock_reserve there.

And the same time, init_memory_mapping() will call init_range_memory_mapping/for_each_mem_pfn_range() to
set kernel mapping for memory range in memblock only.
So here calling init_memory_mapping will not do anything. then just drop calling to that init_memory_mapping.
--- so will not kernel mapping pmem, is that what you intended to have?

Here is another version of the same trivial pmem driver, because two obviously aren't enough. The first patch is the same pmem driver that Ross posted a short time ago, just modified to use platform_devices to find the persistant memory region instead of hardconding it in the Kconfig. This allows to keep pmem.c separate from any discovery mechanism, but still allow auto-discovery.

The other two patches are a heavily rewritten version of the code that Intel gave to various storage vendors to discover the type 12 (and earlier type 6) nvdimms, which I massaged into a form that is hopefully suitable for mainline.

Note that pmem.c really is the minimal version as I think we need something included ASAP. We'll eventually need to be able to do other I/O from and to it, and as most people know everyone has their own preferre method to do it, which I'd like to discuss once we have the basic driver in.

This has been tested both with a real NVDIMM on a system with a type 12 capable bios, as well as with "fake persistent" memory using the memmap= option.

Changes since V1:
- s/E820_PROTECTED_KERN/E820_PMEM/g
- map the persistent memory as uncached
- better kernel parameter description
- various typo fixes
- MODULE_LICENSE fix

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Christoph why did you choose the fat and ugly version of pmem.c beats me. Anyway, here are the cleanups you need on top of your pmem patch.

Among other it does:
* Remove getgeo. It is not needed for modern fdisk and was never needed for libgparted and cfdisk.

* remove 89 lines of code to do a single memcpy. The reason this was so in brd (done badly BTW) is because destination memory is page-by-page based. With pmem we have the destination contiguous so we can do any size, in one go.

* Remove SECTOR_SHIFT. It is defined in 6 other places in the Kernel. I do not like a new one. 9 is used through out, including block core. I do not like pmem to blasphemy more than needed.

* More style stuff ...

- * This driver is heavily based on drivers/block/brd.c.
+ * This driver's skeleton is based on drivers/block/brd.c.
* Copyright (C) 2007 Nick Piggin
* Copyright (C) 2007 Novell Inc.

-/*
- * direct translation from (pmem,sector) => void*
- * We do not require that sector be page aligned.
- * The return value will point to the beginning of the page containing the
- * given sector, not to the sector itself.
- */

Christoph why did you choose the fat and ugly version of pmem.c beats me. Anyway, here are the cleanups you need on top of your pmem patch.

Among other it does:
* Remove getgeo. It is not needed for modern fdisk and was never needed for libgparted and cfdisk.

* remove 89 lines of code to do a single memcpy. The reason this was so in brd (done badly BTW) is because destination memory is page-by-page based. With pmem we have the destination contiguous so we can do any size, in one go.

* Remove SECTOR_SHIFT. It is defined in 6 other places in the Kernel. I do not like a new one. 9 is used through out, including block core. I do not like pmem to blasphemy more than needed.

* More style stuff ...

Please squash into your initial submission

Signed-off-by Boaz Harrosh <[email protected]>

Christoph why did you choose the fat and ugly version of pmem.c beats me.

Quoted 3 lines · Boaz Harrosh

Christoph why did you choose the fat and ugly version of pmem.c beats me.

Boaz, I am so very tired of your snide commentary. It severely detracts from the technical merit of your patches. Please stop.

For one this auto discovery of yours is very (very) nice but is a bit inconvenience. Before I would reserve a big chuck on each NUMA range on Kernel's memmap= And then at pmem map= would slice and dice it as I want hot style on modprobe with no need for reboot. Now I need to do it on reboot theoretically. (You know xfstest needs lots of devices some big some small ;-))

Also with the modprob pmem map= I was supporting a PCIE memory card but I guess I need to throw this one out the door.

Quoted 6 lines · Boaz Harrosh

For one this auto discovery of yours is very (very) nice but is a bit inconvenience. Before I would reserve a big chuck on each NUMA range on Kernel's memmap= And then at pmem map= would slice and dice it as I want hot style on modprobe with no need for reboot. Now I need to do it on reboot theoretically. (You know xfstest needs lots of devices some big some small ;-))

Slicing up a block device based on kernel options is not exactly a smart idea. We have partitions that are perfectly fine for that. If you really don't are about persistance of your partitioning you can just set up a device mapper table. No need to reinvent the wheel.

But I hope you are not ignoring my real problem. any two memmap= ranges will halt the boot. Specially if they are dis-contiguous.

Also I need the contiguous variant split into two devices because they might belong to two NUMA nodes. It is very hard to manage if a NUMA crossing is in a middle of a single pmemX device The way we like to configure it is that each /dev/pmem belongs to a single NUMA node. And in a multy device setup each CPU node allocates from "his" pmem device If there is space. (And it lets me set application affinity if need to)

BTW: Will device mapper let me call ->direct_access()

Quoted 7 lines · Christoph Hellwig

Here is another version of the same trivial pmem driver, because two obviously aren't enough. The first patch is the same pmem driver that Ross posted a short time ago, just modified to use platform_devices to find the persistant memory region instead of hardconding it in the Kconfig. This allows to keep pmem.c separate from any discovery mechanism, but still allow auto-discovery.

Hi Christoph

So I've been trying to test your version, and play around with it. I currently have some problems, but this is the end of the week for me so I will debug and fix it after the weekend on Sunday.

Quoted 9 lines · Boaz Harrosh

Quoted 7 lines · Christoph Hellwig

Here is another version of the same trivial pmem driver, because two obviously aren't enough. The first patch is the same pmem driver that Ross posted a short time ago, just modified to use platform_devices to find the persistant memory region instead of hardconding it in the Kconfig. This allows to keep pmem.c separate from any discovery mechanism, but still allow auto-discovery.

Quoted 5 lines

Hi Christoph

So I've been trying to test your version, and play around with it. I currently have some problems, but this is the end of the week for me so I will debug and fix it after the weekend on Sunday.

Any news? I'd really like to resend this ASAP to get it into 4.1..

On 03/31/2015 12:25 PM, Christoph Hellwig wrote:

The problem I see is that if I state a memmap=nn!aa that crosses a NUMA boundary then the machine will not boot.
So BTW for sure I need that "don't merge E820_PMEM ranges" patch because otherwise I will not be able to boot if I have pmem on both NUMA nodes and they happen to be contiguous.

And one last issue. I have some configuration "hardness" with the memmap=nn!aa Kernel command line API, it was better for me with the pmem map= module param. Will you be OK if I split pmem_probe() into calling pmem_alloc(addr, length), so I can keep an out-of-tree patch that adds the map= parameter to pmem?

The problem I see is that if I state a memmap=nn!aa that crosses a NUMA boundary then the machine will not boot.
So BTW for sure I need that "don't merge E820_PMEM ranges" patch because otherwise I will not be able to boot if I have pmem on both NUMA nodes and they happen to be contiguous.

Regarding the SQUASHMEs to PMEM. Originally I had them as 3-4 patches. But I thought since you are squashing them into a single submitted patch I can just send just the one patch. Tell me what you prefer and I'll resend (The one vs the three)

And one last issue. I have some configuration "hardness" with the memmap=nn!aa Kernel command line API, it was better for me with the pmem map= module param. Will you be OK if I split pmem_probe() into calling pmem_alloc(addr, length), so I can keep an out-of-tree patch that adds the map= parameter to pmem?

Any news? I'd really like to resend this ASAP to get it into 4.1..

If we need a NUMA crossing a mids a contiguous memory range we split the range at NUMA boundary so to produce two pmemX devices. We do not like 2 NUMA IDs at the same device.

TODO: What happens with real type-12 NvDIMM the BIOS splits these ranges?

Some error checks had unlikely some did not. Put unlikely on all error handling paths.
(I like unlikely for error paths specially for readability)

Quoted 4 lines · Boaz Harrosh

Some error checks had unlikely some did not. Put unlikely on all error handling paths.
(I like unlikely for error paths specially for readability)

"unlikely()" is not a readability hint, it's specifically for branches that profiling shows adding it makes a difference. Just delete them all until profiling show they make a difference. They certainly don't make a difference in the slow paths.

Quoted 6 lines · Dan Williams

Quoted 4 lines · Boaz Harrosh

Some error checks had unlikely some did not. Put unlikely on all error handling paths.
(I like unlikely for error paths specially for readability)

Quoted 5 lines

"unlikely()" is not a readability hint, it's specifically for branches that profiling shows adding it makes a difference. Just delete them all until profiling show they make a difference. They certainly don't make a difference in the slow paths.

Why?

Quoted 13 lines · Boaz Harrosh

Quoted 11 lines · Dan Williams

Quoted 4 lines · Boaz Harrosh

Some error checks had unlikely some did not. Put unlikely on all error handling paths.
(I like unlikely for error paths specially for readability)

"unlikely()" is not a readability hint, it's specifically for branches that profiling shows adding it makes a difference. Just delete them all until profiling show they make a difference. They certainly don't make a difference in the slow paths.

Quoted 1 line

Why?

Because the compiler and cpu already does a decent job, and if you get the frequency wrong it can hurt performance [1].

It's pre-mature optimization to sprinkle them around, especially in slow paths.

[1]: https://lwn.net/Articles/420019/

But not here. This is not an optimization this is the *error path*. What I'm saying is:
"No compiler nor CPU, even if 99% of the time this branch is taken
I still consider it cold. Because it is the error case and I do not care for it"

On Thu, Mar 26, 2015 at 06:57:47PM +0200, Boaz Harrosh wrote:

Any news? I'd really like to resend this ASAP to get it into 4.1..

We can not accept it as is right now.

We have conducted farther tests. And it messes up NUMA.

Quoted 1 line · Boaz Harrosh

We can not accept it as is right now.

Who is we?

Quoted 1 line

We have conducted farther tests. And it messes up NUMA.

Only you if you use the memmap option in weird ways.

Sounds like I should simply remove the memmap= option so people don't abuse it. The main point is to parse the e820 tables, which works fine.

I'd be fine with that too - mind sending an updated series?

Quoted 1 line · Ingo Molnar

I'd be fine with that too - mind sending an updated series?

I will send an updated one tonight or early tomorrow.

Btw, do you want to keep the E820_PRAM name instead of E820_PMEM? Seems like most people either don't care or prefer E820_PMEM. I'm fine either way.

Quoted 8 lines · Christoph Hellwig

Quoted 1 line · Ingo Molnar

I'd be fine with that too - mind sending an updated series?

I will send an updated one tonight or early tomorrow.

Btw, do you want to keep the E820_PRAM name instead of E820_PMEM? Seems like most people either don't care or prefer E820_PMEM. I'm fine either way.

FWIW, I like the idea of having a separate E820_PRAM name for type-12 memory vs future "can't yet disclose" UEFI memory type. The E820_PRAM type potentially has the property of being relegated to "legacy" NVDIMMs. We can later add E820_PMEM as a memory type that, for example, is not automatically backed by struct page. That said, I'm fine either way.

Quoted 10 lines · Dan Williams

Quoted 8 lines · Christoph Hellwig

Quoted 1 line · Ingo Molnar

I'd be fine with that too - mind sending an updated series?

I will send an updated one tonight or early tomorrow.

Btw, do you want to keep the E820_PRAM name instead of E820_PMEM? Seems like most people either don't care or prefer E820_PMEM. I'm fine either way.

Quoted 6 lines

FWIW, I like the idea of having a separate E820_PRAM name for type-12 memory vs future "can't yet disclose" UEFI memory type. The E820_PRAM type potentially has the property of being relegated to "legacy" NVDIMMs. We can later add E820_PMEM as a memory type that, for example, is not automatically backed by struct page. That said, I'm fine either way.

I agree that it's a minor detail, but I think the separation is useful in two ways:

- We have a generic 'pmem' driver, but the low level, platform specific RAM enumeration name does not use that name.

- 'E820_PRAM' is a more natural extension of 'E820_RAM'.

Later on we can then do a:

    s/E820_PRAM/E820_LEGACY_PRAM

rename or so.

Quoted 1 line · Boaz Harrosh

We can not accept it as is right now.

Who is we?

Quoted 1 line

We have conducted farther tests. And it messes up NUMA.

Only you if you use the memmap option in weird ways.

Sounds like I should simply remove the memmap= option so people don't abuse it. The main point is to parse the e820 tables, which works fine.

And those people having fake pmem, or pcie cards that they are too lazy to submit proper drivers for can stick to their out of tree hacks?

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Christoph Hellwig
Sent: Thursday, March 26, 2015 3:33 AM
To: [email protected]; [email protected]; linux- [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: another pmem variant V2

Here is another version of the same trivial pmem driver, because two obviously aren't enough. The first patch is the same pmem driver that Ross posted a short time ago, just modified to use platform_devices to find the persistant memory region instead of hardconding it in the Kconfig. This allows to keep pmem.c separate from any discovery mechanism, but still allow auto-discovery.

This has been tested both with a real NVDIMM on a system with a type 12 capable bios, as well as with "fake persistent" memory using the memmap= option.

Changes since V1:
- s/E820_PROTECTED_KERN/E820_PMEM/g
- map the persistent memory as uncached
- better kernel parameter description
- various typo fixes
- MODULE_LICENSE fix

I used fio to test 4 KiB random read and write IOPS on a 2-socket x86 DDR4 system. With various cache attributes:

attr read write notes
---- ---- ----- -----
UC 37 K 21 K ioremap_nocache
WB 3.6 M 2.5 M ioremap
WC 764 K 3.7 M ioremap_wc
WT <not tested yet> ioremap_wt

So, although UC and WT are the only modes certain to be safe, the V1 default of UC provides abysmal performance - worse than a consumer-class SATA SSD.

A solution for x86 is to use the MOVNTI instruction in WB mode. This non-temporal hint uses a buffer like the write combining buffer, not filling the cache and not stopping everything in the CPU. The kernel function __copy_from_user() uses that instruction (with SFENCE at the end) - see arch/x86/lib/copy_user_nocache_64.S.

If I made the change from memcpy() to __copy_from_user() correctly, that results in:

attr read write notes
---- ---- ----- -----
WB w/NTI 2.4 M 2.6 M __copy_from_user()
WC w/NTI 3.2 M 2.1 M __copy_from_user()

-----Original Message-----
From: Christoph Hellwig [mailto:[email protected]]
Sent: Wednesday, April 1, 2015 2:26 AM
To: Elliott, Robert (Server Storage)
Cc: Christoph Hellwig; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Kani, Toshimitsu
Subject: Re: another pmem variant V2

Quoted 10 lines · Robert (Server Storage)

I used fio to test 4 KiB random read and write IOPS on a 2-socket x86 DDR4 system. With various cache attributes:

attr read write notes
---- ---- ----- -----
UC 37 K 21 K ioremap_nocache
WB 3.6 M 2.5 M ioremap
WC 764 K 3.7 M ioremap_wc
WT <not tested yet> ioremap_wt

Quoted 3 lines

So, although UC and WT are the only modes certain to be safe, the V1 default of UC provides abysmal performance - worse than a consumer-class SATA SSD.

It doesn't look quite as bad on my setup, but performance is fairly bad here as well.

Quoted 7 lines

A solution for x86 is to use the MOVNTI instruction in WB mode. This non-temporal hint uses a buffer like the write combining buffer, not filling the cache and not stopping everything in the CPU. The kernel function __copy_from_user() uses that instruction (with SFENCE at the end) - see arch/x86/lib/copy_user_nocache_64.S.

Quoted 7 lines

If I made the change from memcpy() to __copy_from_user() correctly, that results in:

attr read write notes
---- ---- ----- -----
WB w/NTI 2.4 M 2.6 M __copy_from_user()
WC w/NTI 3.2 M 2.1 M __copy_from_user()

That looks a lot better. It doesn't help us with a pmem device mapped directly into userspace using mmap with the DAX infrastructure, though.

Note when we want to move to non-temporal copies we'll need to add a new prototype, as __copy_from_user isn't guaranteed to use these, and it is defined to only work on user addresses. That doesn't matter on x86 but would blow up on say sparc or s390.

Attr Copy Read IOPS Write IOPS
==== ==== ========= ==========
UC memcpy 36 K 22 K
UC NT rd,wr 513 K 326 K

WB memcpy 3.4 M 2.5 M
WB NT rd,wr 3.3 M 3.5 M

WC memcpy 776 K 3.5 M
WC NT rd,wr 3.0 M 3.9 M

WT memcpy 2.1 M 22 K
WT NT rd,wr 3.3 M 2.1 M

a few other variations yielded the peak numbers:
WC NT rd only 3.2 M 4.1 M
WC NT wr only 712 K 4.6 M
WT NT wr only 2.6 M 4.0 M

There are lots of tuning considerations for those memcpy functions - how far to unroll the loop, whether to include PRFETCHNTA instructions, etc.

Quoted 19 lines · Robert (Server Storage)

Attr Copy Read IOPS Write IOPS
==== ==== ========= ==========
UC memcpy 36 K 22 K
UC NT rd,wr 513 K 326 K

WB memcpy 3.4 M 2.5 M
WB NT rd,wr 3.3 M 3.5 M

WC memcpy 776 K 3.5 M
WC NT rd,wr 3.0 M 3.9 M

WT memcpy 2.1 M 22 K
WT NT rd,wr 3.3 M 2.1 M

a few other variations yielded the peak numbers:
WC NT rd only 3.2 M 4.1 M
WC NT wr only 712 K 4.6 M
WT NT wr only 2.6 M 4.0 M

Quoted 3 lines

There are lots of tuning considerations for those memcpy functions - how far to unroll the loop, whether to include PRFETCHNTA instructions, etc.

Looks like we should aіm for WC + NT would be a good start.

Can you prepare a patch to add your NT memcpy variants and a second one to use them in the pmem driver?

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Christoph Hellwig
Sent: Thursday, March 26, 2015 3:33 AM
To: [email protected]; [email protected]; linux- [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]
Subject: another pmem variant V2

I triggered a paging error in the memcpy call for a block read from system-udevd (actually in a modified memcpy() for the cache attribute experiments).

1. This triggered an illegal schedule() call from an atomic context. The call trace is shown below.

2. memcpy() doesn't provide exception handling or error reporting. Some functions like do so, like __copy_user_nocache in arch/x85/lib/copy_user_nocache_64.S.

Should pmem only use functions that do so, if available on the architecture?