pmem: Support for "struct page" with Persistent Memory storage

From: Boaz Harrosh <[email protected]>

With current values fdisk does the wrong thing.

Setting all values to 1, will make everything nice and easy.

Note that current code had a BUG with anything bigger than 64G because hd_geometry->cylinders is ushort and it would overflow at this value. Any way capacity is not calculated through getgeo so it does not matter what you put here.

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

---
 drivers/block/prd.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/block/prd.c +4 -4

index cc0aabf..62af81e 100644
@@ -55,10 +55,18 @@ struct prd_device {
 
 static int prd_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;
+	/* Just tell fdisk to get out of the way. The math here is so
+	 * convoluted and does not make any sense at all. With all 1s
+	 * The math just gets out of the way.
+	 * NOTE: I was trying to get some values that will make fdisk

On Wed, 2014-08-13 at 15:14 +0300, Boaz Harrosh wrote:

Quoted 16 lines

static int prd_getgeo(struct block_device *bd, struct hd_geometry *geo) {

drivers/block/prd.c +10 -4

-	/* some standard values */
-	geo->heads = 1 << 6;
-	geo->sectors = 1 << 5;
-	geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+	/* Just tell fdisk to get out of the way. The math here is so
+	 * convoluted and does not make any sense at all. With all 1s
+	 * The math just gets out of the way.
+	 * NOTE: I was trying to get some values that will make fdisk
+	 * Want to align first sector on 4K (like 8, 16, 20, ... sectors) but
+	 * nothing worked, I searched the net the math is not your regular
+	 * simple multiplication at all. If you managed to get these please
+	 * fix here. For now we use 4k physical sectors for this
+	 */
+	geo->heads = 1;

drivers/block/prd.c struct prd_device { · line 71

Quoted 4 lines

drivers/block/prd.c +2 -0

+	geo->sectors = 1;
+	geo->cylinders = 1;
 	return 0;
 }

I'm okay with this change, but can you let me know in which case fdisk was previously doing the wrong thing? I'm just curious because I never saw it misbehave, and wonder what else I should be testing.

Regarding the note in the comment, is this addressed by the blk_queue_physical_block_size() and prd->prd_queue->limits.io_min changes in your patch 5/9, or is it an open issue? Either way, can we nix the NOTE?

Also, you put "SQUASHME" on this patch. I'm planning on squashing all of my patches together into an "initial version" type patch (see
https://github.com/01org/prd). Based on this, it probably makes sense to keep it separate so you get credit for the patch?

- Ross

From: Boaz Harrosh <[email protected]>

This streamlines prd with the latest brd code.

In prd we do not allocate new devices dynamically on devnod access, because we need parameterization of each device. So the dynamic allocation in prd_init_one is removed.

Therefor prd_init_one only called from prd_prob is moved there, now that it is small.

And other small fixes regarding partitions

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

---
 drivers/block/prd.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

drivers/block/prd.c +0 -0

index 62af81e..c4aeba7 100644
@@ -218,13 +218,13 @@ static long prd_direct_access(struct block_device *bdev, sector_t sector,
 {
 	struct prd_device *prd = bdev->bd_disk->private_data;

Quoted 8 lines

(quoted diff) +1 -1

@@ -218,13 +218,13 @@ static long prd_direct_access(struct block_device *bdev, sector_t sector,
 {
 	struct prd_device *prd = bdev->bd_disk->private_data;
 
-	if (!prd)
+	if (unlikely(!prd))
 		return -ENODEV;
 

Review static long prd_direct_access(struct block_device *bdev, sector_t sector, · line 227

Quoted 5 lines

(quoted diff) +1 -1

 	*kaddr = prd_lookup_pg_addr(prd, sector);
 	*pfn = prd_lookup_pfn(prd, sector);
 
-	return size;
+	return min_t(long, size, prd->size);

This is off course a BUG need to subtract offset, will send version 2

Quoted 11 lines · Boaz Harrosh

Quoted 11 lines

(quoted diff) +1 -1

@@ -218,13 +218,13 @@ static long prd_direct_access(struct block_device *bdev, sector_t sector,
 {
 	struct prd_device *prd = bdev->bd_disk->private_data;
 
-	if (!prd)
+	if (unlikely(!prd))
 		return -ENODEV;
 
 	*kaddr = prd_lookup_pg_addr(prd, sector);
 	*pfn = prd_lookup_pfn(prd, sector);
 

Quoted 4 lines

Review static long prd_direct_access(struct block_device *bdev, sector_t sector, · line 227

Quoted 2 lines

(quoted diff) +1 -1

-	return size;
+	return min_t(long, size, prd->size);

This is off course a BUG need to subtract offset, will send version 2

I was wondering about simplifying the return value for the drivers a little. Something like this:

arch/powerpc/sysdev/axonram.c +1 -1

index 741293f..8709b9f 100644
@@ -149,7 +149,7 @@ axon_ram_direct_access(struct block_device *device, sector_t sector,
 	*kaddr = (void *)(bank->ph_addr + offset);
 	*pfn = virt_to_phys(*kaddr) >> PAGE_SHIFT;
 
-	return min_t(long, size, bank->size - offset);
+	return bank->size - offset;
 }
 
 static const struct block_device_operations axon_ram_devops = {

drivers/block/brd.c +3 -3

index 3483458..344681a 100644
@@ -384,9 +384,9 @@ static long brd_direct_access(struct block_device *bdev, sector_t sector,
 	*kaddr = page_address(page);
 	*pfn = page_to_pfn(page);
 
-	/* Could optimistically check to see if the next page in the
-	 * file is mapped to the next page of physical RAM */
-	return min_t(long, PAGE_SIZE, size);
+	/* If size > PAGE_SIZE, we could look to see if the next page in the
+	 * file happens to be mapped to the next page of physical RAM */
+	return PAGE_SIZE;
 }
 #else
 #define brd_direct_access NULL

drivers/block/prd.c +1 -1

index cc0aabf..1cfbd5b 100644
@@ -216,7 +216,7 @@ static long prd_direct_access(struct block_device *bdev, sector_t sector,
 	*kaddr = prd_lookup_pg_addr(prd, sector);
 	*pfn = prd_lookup_pfn(prd, sector);
 
-	return size;
+	return size - (sector * 512);
 }
 
 static const struct block_device_operations prd_fops = {

drivers/s390/block/dcssblk.c +1 -1

index 2ee5556..96bc411 100644
@@ -881,7 +881,7 @@ dcssblk_direct_access (struct block_device *bdev, sector_t secnum,
 	*kaddr = (void *) (dev_info->start + offset);
 	*pfn = virt_to_phys(*kaddr) >> PAGE_SHIFT;
 
-	return min_t(long, size, dev_sz - offset);
+	return dev_sz - offset;
 }
 
 static void

fs/block_dev.c +5 -2

index 93ebdd53..ce3e69c 100644
@@ -447,6 +447,7 @@ EXPORT_SYMBOL_GPL(bdev_write_page);
 long bdev_direct_access(struct block_device *bdev, sector_t sector,
 			void **addr, unsigned long *pfn, long size)
 {
+	long max;
 	const struct block_device_operations *ops = bdev->bd_disk->fops;
 	if (!ops->direct_access)
 		return -EOPNOTSUPP;

@@ -456,8 +457,10 @@ long bdev_direct_access(struct block_device *bdev, sector_t sector,
 	sector += get_start_sect(bdev);
 	if (sector % (PAGE_SIZE / 512))
 		return -EINVAL;
-	size = ops->direct_access(bdev, sector, addr, pfn, size);
-	return size ? size : -ERANGE;
+	max = ops->direct_access(bdev, sector, addr, pfn, size);
+	if (!max)
+		return -ERANGE;
+	return min(max, size);
 }
 EXPORT_SYMBOL_GPL(bdev_direct_access);
 

From: Boaz Harrosh <[email protected]>

This streamlines prd with the latest brd code.

In prd we do not allocate new devices dynamically on devnod access, because we need parameterization of each device. So the dynamic allocation in prd_init_one is removed.

Therefor prd_init_one only called from prd_prob is moved there, now that it is small.

And other small fixes regarding partitions

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

---
 drivers/block/prd.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

drivers/block/prd.c +12 -3

 	mutex_lock(&prd_devices_mutex);
-	prd = prd_init_one(MINOR(dev));
-	kobj = prd ? get_disk(prd->prd_disk) : NULL;
-	mutex_unlock(&prd_devices_mutex);
 
+	list_for_each_entry(prd, &prd_devices, prd_list) {
+		if (prd->prd_number == number) {
+			kobj = get_disk(prd->prd_disk);
+			goto out;
+		}
+	}
+
+	pr_err("prd: prd_probe: Unexpected parameter=%d\n", number);
+	kobj = NULL;
+
+out:
+	mutex_unlock(&prd_devices_mutex);
 	return kobj;
 }
 

I think we can still have a working probe by instead comparing the passed in dev_t against the dev_t we get back from disk_to_dev(prd->prd_disk), but I'd really like a use case so I can test this. Until then I think I'm just going to stub out prd/pmem_probe() with a BUG() so we can see if anyone hits it.

It seems like this is preferable to just registering NULL for probe, as that would cause an oops in kobj_lookup(() when probe() is blindly called without checking for NULL.

On 08/25/2014 11:10 PM, Ross Zwisler wrote:
<>

Quoted 9 lines

I think we can still have a working probe by instead comparing the passed in dev_t against the dev_t we get back from disk_to_dev(prd->prd_disk), but I'd really like a use case so I can test this. Until then I think I'm just going to stub out prd/pmem_probe() with a BUG() so we can see if anyone hits it.

It seems like this is preferable to just registering NULL for probe, as that would cause an oops in kobj_lookup(() when probe() is blindly called without checking for NULL.

I have a version I think you will love it removes the probe and bunch of other stuff.

I tested it heavily it just works

Comming soon, I'm preparing trees right now
Thanks
Boaz

From 748370b1220693b755b6b92828e651cc0bac7846 Mon Sep 17 00:00:00 2001

From the BLOCK_EXT_MAJOR (259) range. Even Though the

Meanwhile without any explanations, these will come tomorrow, I'm attaching the most interesting bit which you have not seen before.

If you want you can inspect a preview of what's to come here:
http://git.open-osd.org/gitweb.cgi?p=pmem.git;a=summary

Quoted 5 lines · Boaz Harrosh

Meanwhile without any explanations, these will come tomorrow, I'm attaching the most interesting bit which you have not seen before.

If you want you can inspect a preview of what's to come here:
http://git.open-osd.org/gitweb.cgi?p=pmem.git;a=summary

Regarding the top patch "pmem: KISS, remove the all pmem_major registration", I like that we're getting rid of lots of dead code. The only issue I have is that I'm pretty sure we aren't supposed to register our disks directly with a major of BLOCK_EXT_MAJOR.

I think we still need to register our own major via register_blkdev(), and use that. I'm fine with getting rid of the module parameter though, and always getting a major dynamically.

If you look at the other block devices that use the GENHD_FL_EXT_DEVT flag (nvme, loop, md, etc.) they all register their own major. You can't see this major by doing 'ls -l' on the resulting devices in /dev, but you can see it by looking at /proc/devices:

# ls -l /dev/pmem0
brw-rw---- 1 root disk 259, 0 Aug 26 12:37 /dev/pmem0

# grep pmem /proc/devices 
250 pmem

- Ross

It is getting late around here and I will not be pushing new trees tonight, first thing tomorrow morning. (Your last push caused me lots of extra work, you must not do like you did when working on a rebasing out-off-tree project involving more then one person, but more on the proper procedure tomorrow with the push of the trees)

Quoted 5 lines · Boaz Harrosh

It is getting late around here and I will not be pushing new trees tonight, first thing tomorrow morning. (Your last push caused me lots of extra work, you must not do like you did when working on a rebasing out-off-tree project involving more then one person, but more on the proper procedure tomorrow with the push of the trees)

If you're going to push a new tree, please do so on top of the DAX v10 patches I just sent out.

If you have any changes you want to make
to the bdev_direct_access() patch, then please make them separately, instead of editing my patch like you did last time.

Quoted 4 lines

If you have any changes you want to make
to the bdev_direct_access() patch, then please make them separately, instead of editing my patch like you did last time.

I apologize about that but I did use the proper protocol here. I'm sending your patch as part of my push from my tree, through Jens into linus. I have put my sign-off-by on the patch, then Jens will put his, before it will finally be pushed to Linus.

Having my sign-off on it and pushing to Jens means I need to review and approve the patch, and so I had to fix the checkpatch complain before pushing. And I did put at the patch commit message that I have edited for check-patch. And I have sent the mail directly addressed to you for approval.

If I have offended you in any way please forgive me, none was intended, opposite I have only the deepest gratitude and admiration, and I want for these patches to be pushed forward.

Thanks
Boaz

On Wed, Aug 27, 2014 at 12:55:56PM +0300, Boaz Harrosh wrote:

Quoted 5 lines

Having my sign-off on it and pushing to Jens means I need to review and approve the patch, and so I had to fix the checkpatch complain before pushing. And I did put at the patch commit message that I have edited for check-patch. And I have sent the mail directly addressed to you for approval.

That's not what SubmittingPatches says that you are to do. Indeed, option (c) says that you haven't modified my patch.

Quoted 6 lines

If I have offended you in any way please forgive me, none was intended, opposite I have only the deepest gratitude and admiration, and I want for these patches to be pushed forward.

Thanks
Boaz

From: Boaz Harrosh <[email protected]>

This streamlines prd with the latest brd code.

In prd we do not allocate new devices dynamically on devnod access, because we need parameterization of each device. So the dynamic allocation in prd_init_one is removed.

Therefor prd_init_one only called from prd_prob is moved there, now that it is small.

And other small fixes regarding partitions

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

---
 drivers/block/prd.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

drivers/block/prd.c +6 -21

@@ -308,24 +314,6 @@ static void prd_free(struct prd_device *prd)
 	kfree(prd);
 }
 
-static struct prd_device *prd_init_one(int i)
-{
-	struct prd_device *prd;
-
-	list_for_each_entry(prd, &prd_devices, prd_list) {
-		if (prd->prd_number == i)
-			goto out;
-	}
-
-	prd = prd_alloc(i);
-	if (prd) {
-		add_disk(prd->prd_disk);
-		list_add_tail(&prd->prd_list, &prd_devices);
-	}
-out:
-	return prd;
-}
-
 static void prd_del_one(struct prd_device *prd)
 {
 	list_del(&prd->prd_list);

@@ -333,16 +321,27 @@ static void prd_del_one(struct prd_device *prd)
 	prd_free(prd);
 }
 
+/*FIXME: Actually in our driver prd_probe is never used. Can be removed */
 static struct kobject *prd_probe(dev_t dev, int *part, void *data)
 {
 	struct prd_device *prd;
 	struct kobject *kobj;
+	int number = MINOR(dev);
 
 	mutex_lock(&prd_devices_mutex);
-	prd = prd_init_one(MINOR(dev));
-	kobj = prd ? get_disk(prd->prd_disk) : NULL;
-	mutex_unlock(&prd_devices_mutex);
 
+	list_for_each_entry(prd, &prd_devices, prd_list) {
+		if (prd->prd_number == number) {
+			kobj = get_disk(prd->prd_disk);
+			goto out;

On Wed, 2014-08-13 at 15:16 +0300, Boaz Harrosh wrote:

I really like where you're going with getting rid of prd_probe. Clearly I just copied this from brd, but I'd love to be rid of it entirely. Is there a valid way for our probe function to get called? If not, can we just have a little stub with a BUG() in it to make sure we hear about it if it does ever get called, and delete a bunch of code?

I think this would let us get rid of pmem_probe(), pmem_init_one(), and the pmem_devices_mutex.

If there *is* a valid way for this code to get called, let's figure it out so we can at least test this function. This will be especially necessary as we add support for more pmem disks.

drivers/block/prd.c static void __exit prd_exit(void) · line 426

Quoted 6 lines

drivers/block/prd.c +1 -0

@@ -424,5 +423,7 @@ static void __exit prd_exit(void)
 
 MODULE_AUTHOR("Ross Zwisler <[email protected]>");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("pmem");

Let's just go with the full rename s/prd/pmem/. That turned out to be really clean & made everything consistent - thanks for the good suggestion.

- Ross

From: Boaz Harrosh <[email protected]>

This patch removes any global memory information. And lets each prd-device manage it's own memory region.

prd_alloc() Now receives phys_addr and disk_size and will map that region, also prd_free will do the unmaping.

This is so we can support multiple discontinuous memory regions in the next patch

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

---
 drivers/block/prd.c | 125 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 78 insertions(+), 47 deletions(-)

From: Boaz Harrosh <[email protected]>

After the last patch this is easy.

The API to prd module is changed. We now have a single string parameter named "map" of the form:

		 map=mapS[,mapS...]

		 where mapS=nn[KMG]$ss[KMG],
		 or    mapS=nn[KMG]@ss[KMG],

		 nn=size, ss=offset

Just like the Kernel command line map && memmap parameters, so anything you did at grub just copy/paste to here.

The "@" form is exactly the same as the "$" form only that at bash prompt we need to escape the "$" with \$ so also support the '@' char for convenience.

For each specified mapS there will be a device created.

So needless to say that all the previous prd_XXX params are removed as well as the Kconfig defaults.

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

---
 drivers/block/Kconfig | 28 -----------------------
 drivers/block/prd.c   | 62 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 41 insertions(+), 49 deletions(-)

drivers/block/Kconfig +0 -28

index 463c45e..8f0c225 100644
@@ -416,34 +416,6 @@ config BLK_DEV_PMEM
 	  Most normal users won't need this functionality, and can thus say N
 	  here.
 
-config BLK_DEV_PMEM_START
-	int "Offset in GiB of where to start claiming space"
-	default "0"
-	depends on BLK_DEV_PMEM
-	help
-	  Starting offset in GiB that PRD should use when claiming memory.  This
-	  memory needs to be reserved from the OS at boot time using the
-	  "memmap" kernel parameter.
-
-	  If you provide PRD with volatile memory it will act as a volatile
-	  RAM disk and your data will not be persistent.
-
-config BLK_DEV_PMEM_COUNT
-	int "Default number of PMEM disks"
-	default "4"
-	depends on BLK_DEV_PMEM
-	help
-	  Number of equal sized block devices that PRD should create.
-
-config BLK_DEV_PMEM_SIZE
-	int "Size in GiB of space to claim"
-	depends on BLK_DEV_PMEM
-	default "0"
-	help
-	  Amount of memory in GiB that PRD should use when creating block
-	  devices.  This memory needs to be reserved from the OS at
-	  boot time using the "memmap" kernel parameter.
-
 config CDROM_PKTCDVD
 	tristate "Packet writing on CD/DVD media"
 	depends on !UML

drivers/block/prd.c +5 -11

index 6d96e6c..36b8fe4 100644
@@ -228,21 +228,15 @@ static const struct block_device_operations prd_fops = {
 };
 
 /* Kernel module stuff */
-static int prd_start_gb = CONFIG_BLK_DEV_PMEM_START;
-module_param(prd_start_gb, int, S_IRUGO);
-MODULE_PARM_DESC(prd_start_gb, "Offset in GB of where to start claiming space");
-
-static int prd_size_gb = CONFIG_BLK_DEV_PMEM_SIZE;
-module_param(prd_size_gb,  int, S_IRUGO);
-MODULE_PARM_DESC(prd_size_gb,  "Total size in GB of space to claim for all disks");
-
 static int prd_major;
 module_param(prd_major, int, 0);
 MODULE_PARM_DESC(prd_major,  "Major number to request for this driver");
 
-static int prd_count = CONFIG_BLK_DEV_PMEM_COUNT;
-module_param(prd_count, int, S_IRUGO);
-MODULE_PARM_DESC(prd_count, "Number of prd devices to evenly split allocated space");
+static char *map;
+module_param(map, charp, S_IRUGO);
+MODULE_PARM_DESC(map,
+	"pmem device mapping: map=mapS[,mapS...] where:\n"
+	"mapS=nn[KMG]$ss[KMG] or mapS=nn[KMG]@ss[KMG], nn=size, ss=offset.");
 
 static LIST_HEAD(prd_devices);
 static DEFINE_MUTEX(prd_devices_mutex);

drivers/block/prd.c +6 -0

+		       phys_addr, disk_size);
+		err = -EINVAL;
+		goto out;
+	}
+
 	prd = kzalloc(sizeof(*prd), GFP_KERNEL);
 	if (unlikely(!prd)) {
 		err = -ENOMEM;

@@ -388,22 +389,30 @@ out:
 	return kobj;
 }
 
+static int prd_parse_map_one(char *map, phys_addr_t *start, size_t *size)

drivers/block/prd.c +16 -10

+}
+
 static int __init prd_init(void)
 {
 	int result, i;
 	struct prd_device *prd, *next;
-	phys_addr_t phys_addr;
-	size_t total_size, disk_size;
+	char *p, *prd_map = map;
 
-	if (unlikely(!prd_start_gb || !prd_size_gb || !prd_count)) {
-		pr_err("prd: prd_start_gb || prd_size_gb || prd_count are 0!!\n");
+	if (!prd_map) {
+		pr_err("prd: must specify map parameter.\n");
 		return -EINVAL;
 	}
 
-	phys_addr = (phys_addr_t) prd_start_gb * 1024 * 1024 * 1024;
-	total_size = (size_t)	   prd_size_gb  * 1024 * 1024 * 1024;
-	disk_size = total_size / prd_count;
-
 	result = register_blkdev(prd_major, "prd");
 	if (result < 0) {
 		result = -EIO;

@@ -411,13 +420,24 @@ static int __init prd_init(void)
 	} else if (result > 0)
 		prd_major = result;
 
-	for (i = 0; i < prd_count; i++) {
-		prd = prd_alloc(phys_addr + i * disk_size, disk_size, i);
+	i = 0;
+	while ((p = strsep(&prd_map, ",")) != NULL) {
+		phys_addr_t phys_addr;
+		size_t disk_size;
+
+		if (!*p)
+			continue;
+		result = prd_parse_map_one(p, &phys_addr, &disk_size);
+		if (result)
+			goto out_free;
+

From: Yigal Korman <[email protected]>

One of the current short comings of the NVDIMM/PMEM support is that this memory does not have a page-struct(s) associated with its memory and therefor cannot be passed to a block-device or network or DMAed in any way through another device in the system.

This simple patch fixes all this. After this patch an FS can do:

	bdev_direct_access(,&pfn,);
	page = pfn_to_page(pfn);

And use that page for a lock_page(), set_page_dirty(), and/or anything else one might do with a page *.
(Note that with brd one can already do this)

[pmem-pages-ref-count]
pmem will serve it's pages with ref==0. Once an FS does an blkdev_get_XXX(,FMODE_EXCL,), that memory is own by the FS. The FS needs to manage its allocation, just as it already does for its disk blocks. The fs should set page->count = 2, before submission to any Kernel subsystem so when it returns it will never be released to the Kernel's page-allocators. (page_freeze)

All is actually needed for this is to allocate page-sections and map them into kernel virtual memory. Note that these sections are not associated with any zone, because that would add them to the page_allocators.

Quoted 2 lines · Boaz Harrosh

From: Yigal Korman <[email protected]>

Quoted 5 lines

All is actually needed for this is to allocate page-sections and map them into kernel virtual memory. Note that these sections are not associated with any zone, because that would add them to the page_allocators.

Can we just use memory hotplug and call add_memory(), instead of directly calling sparse_add_one_section()? Memory hotplug adds memory as off-line state, and sets all pages reserved. So, I do not think the page allocators will mess with them (unless you put them online). It can also maps the pages with large page size.

Thanks,
-Toshi

--
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

Quoted 2 lines · Toshi Kani

Quoted 1 line · Boaz Harrosh

From: Yigal Korman <[email protected]>

Quoted 5 lines

Quoted 4 lines

All is actually needed for this is to allocate page-sections and map them into kernel virtual memory. Note that these sections are not associated with any zone, because that would add them to the page_allocators.

Quoted 9 lines

Can we just use memory hotplug and call add_memory(), instead of directly calling sparse_add_one_section()? Memory hotplug adds memory as off-line state, and sets all pages reserved. So, I do not think the page allocators will mess with them (unless you put them online). It can also maps the pages with large page size.

Thanks,
-Toshi

Thank you Toshi for your reply

I was thinking about that as well at first, but I was afraid, once I call add_memory() what will prevent the user from enabling that memory through the sysfs interface later, it looks to me that add_memory() will add all the necessary knobs to do it.

It is very important to keep a clear distinction, pmem is *not* memory what-so-ever it is however memory-mapped and needs these accesses enabled for it, hence the need for page-struct so we can DMA it off the buss.

I am very afraid of any thing that will associate a "zone" with this memory. Also the:

	firmware_map_add_hotplug(start, start + size, "System RAM");

"System RAM" it is not.

And also I think that for DDR4 NvDIMMs we will fail with:

	ret = check_hotplug_memory_range(start, size);

On Sun, 2014-08-17 at 12:17 +0300, Boaz Harrosh wrote:

Quoted 1 line

"System RAM" it is not.

I think add_memory() can be easily extended (or modified to provide a separate interface) for persistent memory, and avoid creating the sysfs interface and change the handling with firmware_map. But I can also see your point that persistent memory should not be added to zone at all.

Anyway, I am a bit concerned with the way to create direct mappings with map_vm_area() within the prd driver. Can we use init_memory_mapping() as it's used by add_memory() and supports large page size? The size of persistent memory will grow up quickly.

Also, I'd prefer to have an mm
interface that takes care of page allocations and mappings, and avoid a driver to deal with them.

Quoted 3 lines

And also I think that for DDR4 NvDIMMs we will fail with:

	ret = check_hotplug_memory_range(start, size);

Can you elaborate why DDR4 will fail with the function above?

Thanks,
-Toshi

Quoted 1 line · Toshi Kani

On Sun, 2014-08-17 at 12:17 +0300, Boaz Harrosh wrote:

<>

Quoted 7 lines

Quoted 1 line

"System RAM" it is not.

I think add_memory() can be easily extended (or modified to provide a separate interface) for persistent memory, and avoid creating the sysfs interface and change the handling with firmware_map. But I can also see your point that persistent memory should not be added to zone at all.

Right

Quoted 4 lines

Anyway, I am a bit concerned with the way to create direct mappings with map_vm_area() within the prd driver. Can we use init_memory_mapping() as it's used by add_memory() and supports large page size? The size of persistent memory will grow up quickly.

A bit about large page size. The principal reason of my effort here is that at some stage I need to send pmem blocks to block-layer or network.

The PAGE == 4K is pasted all over the block stack. Do you know how those can work together? will we need some kind of page_split thing how does that work?

Quoted 4 lines

Also, I'd prefer to have an mm
interface that takes care of page allocations and mappings, and avoid a driver to deal with them.

This is a great idea you mean that I define:

(quoted diff) +2 -0

+	int mm_add_page_mapping(phys_addr_t phys_addr, size_t total_size,
+				void **o_virt_addr)

At the mm level. OK It needs a much better name.

I know of 2 more drivers that will need the use of the same interface actually, so you are absolutely right. I didn't dare ask ;-)

Quoted 6 lines

Quoted 3 lines

And also I think that for DDR4 NvDIMMs we will fail with:

	ret = check_hotplug_memory_range(start, size);

Can you elaborate why DDR4 will fail with the function above?

I'm not at all familiar with the details, perhaps the Intel guys that knows better can chip in, but from the little I
understood: Today with DDR3 these chips come up at the e820 controller, as type 12 memory and, each vendor has a driver to drive proprietary enablement and persistence. With DDR4 it will all be standardized, but it will not come up through the e820 manager, but as a separate device on the SMBus/ACPI.
So it is not clear to me that we want to plug this back into the ARCH's memory controllers. check_hotplug_memory_range is it per ARCH?

I will produce a new Patchset that introduces a new API for drivers. And I will try to see about the use of init_memory_mapping(), as long as it is not using zones.

Do you think that the new code should sit in?
mm/memory_hotplug.c

drivers/block/prd.c MODULE_PARM_DESC(map, · line 252

Quoted 9 lines · Boaz Harrosh

drivers/block/prd.c +9 -0

+#ifdef CONFIG_BLK_DEV_PMEM_USE_PAGES
+static int prd_add_page_mapping(phys_addr_t phys_addr, size_t total_size,
+				void **o_virt_addr)
+{
+	int nid = memory_add_physaddr_to_nid(phys_addr);
+	unsigned long start_pfn = phys_addr >> PAGE_SHIFT;
+	unsigned long nr_pages = total_size >> PAGE_SHIFT;
+	unsigned int start_sec = pfn_to_section_nr(start_pfn);
+	unsigned int end_sec = pfn_to_section_nr(start_pfn + nr_pages - 1);

Nit: any chance you'd change this to be an exclusive end? In the mm code, we usually do:

	unsigned int end_sec = pfn_to_section_nr(start_pfn + nr_pages);

so the for loops end up <end_sec instead of <=end_sec.

drivers/block/prd.c MODULE_PARM_DESC(map, · line 258

Quoted 6 lines

drivers/block/prd.c +6 -0

+	unsigned long phys_start_pfn;
+	struct page **page_array, **mapped_page_array;
+	unsigned long i;
+	struct vm_struct *vm_area;
+	void *virt_addr;
+	int ret = 0;

This is a philosophical thing, but I don't see *ANY* block-specific code in here. Seems like this belongs in mm/ to me.

Is there a reason you don't just do this at boot and have to use hotplug at runtime for it?

What are the ratio of pmem to RAM? Is it possible to exhaust all of RAM with 'struct page's for pmem?

Quoted 2 lines · Dave Hansen

Is there a reason you don't just do this at boot and have to use hotplug at runtime for it?

This is a plug and play thing. This memory region is not reached via memory controller, it is on the ACPI/SBUS device with physical address/size specified there. On load of block-device this will be called. Also a block device can be unloaded and should be able to cleanup.

Quoted 2 lines

What are the ratio of pmem to RAM? Is it possible to exhaust all of RAM with 'struct page's for pmem?

Yes! in the not very distant future there will be systems that have only pmem. yes no RAM. This is because once available some pmem has much better power efficiency then DRAM, because of the no refresh thing. So even cellphones and embedded system first.

So the Admin/setup will need to calculate and configure the proper ratio of volatile vs non-volatile portions of its system for proper usage.

And one last Rant if I may?
I hate the prd name, why why? OK so a freak of bad luck forced us to invent a new name for /dev/ram because it would be weird to do an lsmod and see a ram.ko hanging there which is actually a block device driver. OK so a brd == /dev/ram. But why do we need to carry this punishment forever? Why an additional/different name in the namespace? /dev/foo should just be foo.ko in lsmod, No? So please, please, for my peace of mind can we call this driver pmem.ko?
I know, I would hate it if I was inventing a name and people change it, so Ross it is your call, is it OK if we move back to just call it pmem everywhere?