[RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[Chao Fan]

In sometimes users specify the memory region in immovable node in
some kernel commandline, such as "kernel_core" or the "immovable_mem="
in the patchset that I have send. But users don't know the memory
region. So add this interface to print it.

It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
region, "ss" means the start position of this region.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 1d60b58a8c19..9cadf1a9dccb 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -25,6 +25,7 @@
 
 #include <linux/atomic.h>
 #include <linux/uaccess.h>
+#include <linux/acpi.h>
 
 static DEFINE_MUTEX(mem_sysfs_mutex);
 
@@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
+/*
+ * Immovable memory region
+ */
+
+static ssize_t
+show_immovable_mem(struct device *dev, struct device_attribute *attr,
+		   char *buf)
+{
+	struct acpi_table_header *table_header = NULL;
+	struct acpi_srat_mem_affinity *ma;
+	struct acpi_subtable_header *th;
+	unsigned long long table_size;
+	unsigned long long table_end;
+	char pbuf[35], *p = buf;
+	int len;
+
+	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
+
+	table_size = sizeof(struct acpi_table_srat);
+	table_end = (unsigned long)table_header + table_header->length;
+	th = (struct acpi_subtable_header *)((unsigned long)
+	      table_header + table_size);
+
+	while (((unsigned long)th) +
+	       sizeof(struct acpi_subtable_header) < table_end) {
+		if (th->type == 1) {
+			ma = (struct acpi_srat_mem_affinity *)th;
+			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
+				continue;
+			len = sprintf(pbuf, "%llx@%llx",
+			       ma->length, ma->base_address);
+			if (p != buf) {
+				*p = ',';
+				p++;
+			}
+			memcpy(p, pbuf, len);
+			p = p + len;
+		}
+		th = (struct acpi_subtable_header *)((unsigned long)
+		      th + th->length);
+	}
+	return sprintf(buf, "%s\n", buf);
+}
+
+static DEVICE_ATTR(immovable_mem, 0444, show_immovable_mem, NULL);
+
 static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
 		unsigned long nr_pages, int online_type,
 		struct zone *default_zone)
@@ -798,6 +845,9 @@ static struct attribute *memory_root_attrs[] = {
 #endif
 
 	&dev_attr_block_size_bytes.attr,
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	&dev_attr_immovable_mem.attr,
+#endif
 	&dev_attr_auto_online_blocks.attr,
 	NULL
 };
-- 
2.14.3

Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[Chao Fan]

The result in my virtual machine looks like this:
[root@localhost ~]# cat /sys/devices/system/memory/immovable_mem 
a0000@0,1f300000@100000,1f400000@1f400000,1f400000@3e800000,1f400000@5dc00000,1f400000@7d000000,1f400000@9c400000,4800000@bb800000,1ac00000@100000000,1f400000@11ac00000,1f400000@13a000000,1f400000@159400000

Any comments will be welcome.

Thanks,
Chao Fan

On Wed, Dec 27, 2017 at 08:30:12PM +0800, Chao Fan wrote:
>In sometimes users specify the memory region in immovable node in
>some kernel commandline, such as "kernel_core" or the "immovable_mem="
>in the patchset that I have send. But users don't know the memory
>region. So add this interface to print it.
>
>It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
>region, "ss" means the start position of this region.
>
>Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>---
> drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index 1d60b58a8c19..9cadf1a9dccb 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -25,6 +25,7 @@
> 
> #include <linux/atomic.h>
> #include <linux/uaccess.h>
>+#include <linux/acpi.h>
> 
> static DEFINE_MUTEX(mem_sysfs_mutex);
> 
>@@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
> }
> 
> #ifdef CONFIG_MEMORY_HOTREMOVE
>+/*
>+ * Immovable memory region
>+ */
>+
>+static ssize_t
>+show_immovable_mem(struct device *dev, struct device_attribute *attr,
>+		   char *buf)
>+{
>+	struct acpi_table_header *table_header = NULL;
>+	struct acpi_srat_mem_affinity *ma;
>+	struct acpi_subtable_header *th;
>+	unsigned long long table_size;
>+	unsigned long long table_end;
>+	char pbuf[35], *p = buf;
>+	int len;
>+
>+	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
>+
>+	table_size = sizeof(struct acpi_table_srat);
>+	table_end = (unsigned long)table_header + table_header->length;
>+	th = (struct acpi_subtable_header *)((unsigned long)
>+	      table_header + table_size);
>+
>+	while (((unsigned long)th) +
>+	       sizeof(struct acpi_subtable_header) < table_end) {
>+		if (th->type == 1) {
>+			ma = (struct acpi_srat_mem_affinity *)th;
>+			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
>+				continue;
>+			len = sprintf(pbuf, "%llx@%llx",
>+			       ma->length, ma->base_address);
>+			if (p != buf) {
>+				*p = ',';
>+				p++;
>+			}
>+			memcpy(p, pbuf, len);
>+			p = p + len;
>+		}
>+		th = (struct acpi_subtable_header *)((unsigned long)
>+		      th + th->length);
>+	}
>+	return sprintf(buf, "%s\n", buf);
>+}
>+
>+static DEVICE_ATTR(immovable_mem, 0444, show_immovable_mem, NULL);
>+
> static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
> 		unsigned long nr_pages, int online_type,
> 		struct zone *default_zone)
>@@ -798,6 +845,9 @@ static struct attribute *memory_root_attrs[] = {
> #endif
> 
> 	&dev_attr_block_size_bytes.attr,
>+#ifdef CONFIG_MEMORY_HOTREMOVE
>+	&dev_attr_immovable_mem.attr,
>+#endif
> 	&dev_attr_auto_online_blocks.attr,
> 	NULL
> };
>-- 
>2.14.3
>

Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[Greg KH]

On Wed, Dec 27, 2017 at 08:30:12PM +0800, Chao Fan wrote:
> In sometimes users specify the memory region in immovable node in
> some kernel commandline, such as "kernel_core" or the "immovable_mem="
> in the patchset that I have send. But users don't know the memory
> region. So add this interface to print it.
> 
> It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
> region, "ss" means the start position of this region.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)

Why did you not also create the needed Documentation/ABI/ file update?

That's required for sysfs attributes.

> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 1d60b58a8c19..9cadf1a9dccb 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -25,6 +25,7 @@
>  
>  #include <linux/atomic.h>
>  #include <linux/uaccess.h>
> +#include <linux/acpi.h>
>  
>  static DEFINE_MUTEX(mem_sysfs_mutex);
>  
> @@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> +/*
> + * Immovable memory region
> + */
> +
> +static ssize_t
> +show_immovable_mem(struct device *dev, struct device_attribute *attr,
> +		   char *buf)
> +{
> +	struct acpi_table_header *table_header = NULL;
> +	struct acpi_srat_mem_affinity *ma;
> +	struct acpi_subtable_header *th;
> +	unsigned long long table_size;
> +	unsigned long long table_end;
> +	char pbuf[35], *p = buf;
> +	int len;
> +
> +	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
> +
> +	table_size = sizeof(struct acpi_table_srat);
> +	table_end = (unsigned long)table_header + table_header->length;
> +	th = (struct acpi_subtable_header *)((unsigned long)
> +	      table_header + table_size);
> +
> +	while (((unsigned long)th) +
> +	       sizeof(struct acpi_subtable_header) < table_end) {
> +		if (th->type == 1) {
> +			ma = (struct acpi_srat_mem_affinity *)th;
> +			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
> +				continue;
> +			len = sprintf(pbuf, "%llx@%llx",
> +			       ma->length, ma->base_address);

sysfs is "one value per file", and if you ever have to care if you are
overrunning the length of the buffer, that's a huge hint you are doing
something wrong here.

sorry,

greg k-h

Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[Chao Fan]

On Wed, Dec 27, 2017 at 01:47:26PM +0100, Greg KH wrote:
>On Wed, Dec 27, 2017 at 08:30:12PM +0800, Chao Fan wrote:
>> In sometimes users specify the memory region in immovable node in
>> some kernel commandline, such as "kernel_core" or the "immovable_mem="
>> in the patchset that I have send. But users don't know the memory
>> region. So add this interface to print it.
>> 
>> It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
>> region, "ss" means the start position of this region.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)

Hi Greg,

Thanks for your review.

>
>Why did you not also create the needed Documentation/ABI/ file update?

Sorry, I didn't notice the document issues.

>
>That's required for sysfs attributes.
>
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 1d60b58a8c19..9cadf1a9dccb 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -25,6 +25,7 @@
>>  
>>  #include <linux/atomic.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/acpi.h>
>>  
>>  static DEFINE_MUTEX(mem_sysfs_mutex);
>>  
>> @@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTREMOVE
>> +/*
>> + * Immovable memory region
>> + */
>> +
>> +static ssize_t
>> +show_immovable_mem(struct device *dev, struct device_attribute *attr,
>> +		   char *buf)
>> +{
>> +	struct acpi_table_header *table_header = NULL;
>> +	struct acpi_srat_mem_affinity *ma;
>> +	struct acpi_subtable_header *th;
>> +	unsigned long long table_size;
>> +	unsigned long long table_end;
>> +	char pbuf[35], *p = buf;
>> +	int len;
>> +
>> +	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
>> +
>> +	table_size = sizeof(struct acpi_table_srat);
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +	th = (struct acpi_subtable_header *)((unsigned long)
>> +	      table_header + table_size);
>> +
>> +	while (((unsigned long)th) +
>> +	       sizeof(struct acpi_subtable_header) < table_end) {
>> +		if (th->type == 1) {
>> +			ma = (struct acpi_srat_mem_affinity *)th;
>> +			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
>> +				continue;
>> +			len = sprintf(pbuf, "%llx@%llx",
>> +			       ma->length, ma->base_address);
>
>sysfs is "one value per file", and if you ever have to care if you are
>overrunning the length of the buffer, that's a huge hint you are doing

Yes, you are right, the length of "buf" should be limited here.
I saw in this file, one sysfs only has one value here. But the memory
has more than one region and may be discontinuous, so I stitch the string.
So should I send the new version, or you think this feature should not
be placed here?

Thanks,
Chao Fan

>something wrong here.
>
>sorry,
>
>greg k-h
>
>

Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[Michal Hocko]

Always make sure to CC linux-api mailing list when proposing user
visible API patches.

On Wed 27-12-17 20:30:12, Chao Fan wrote:
> In sometimes users specify the memory region in immovable node in
> some kernel commandline, such as "kernel_core" or the "immovable_mem="
> in the patchset that I have send. But users don't know the memory
> region. So add this interface to print it.
> 
> It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
> region, "ss" means the start position of this region.

The patch doesn't explain who is going to use this information and what
for. Moreover we already do have removable which tells whether the given
memblock is movable.

Finally you are pulling an ACPI specific code into a generic code which
is not acceptable.

Based on all this
Nacked-by: Michal Hocko <mhocko@suse.com>

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 1d60b58a8c19..9cadf1a9dccb 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -25,6 +25,7 @@
>  
>  #include <linux/atomic.h>
>  #include <linux/uaccess.h>
> +#include <linux/acpi.h>
>  
>  static DEFINE_MUTEX(mem_sysfs_mutex);
>  
> @@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> +/*
> + * Immovable memory region
> + */
> +
> +static ssize_t
> +show_immovable_mem(struct device *dev, struct device_attribute *attr,
> +		   char *buf)
> +{
> +	struct acpi_table_header *table_header = NULL;
> +	struct acpi_srat_mem_affinity *ma;
> +	struct acpi_subtable_header *th;
> +	unsigned long long table_size;
> +	unsigned long long table_end;
> +	char pbuf[35], *p = buf;
> +	int len;
> +
> +	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
> +
> +	table_size = sizeof(struct acpi_table_srat);
> +	table_end = (unsigned long)table_header + table_header->length;
> +	th = (struct acpi_subtable_header *)((unsigned long)
> +	      table_header + table_size);
> +
> +	while (((unsigned long)th) +
> +	       sizeof(struct acpi_subtable_header) < table_end) {
> +		if (th->type == 1) {
> +			ma = (struct acpi_srat_mem_affinity *)th;
> +			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
> +				continue;
> +			len = sprintf(pbuf, "%llx@%llx",
> +			       ma->length, ma->base_address);
> +			if (p != buf) {
> +				*p = ',';
> +				p++;
> +			}
> +			memcpy(p, pbuf, len);
> +			p = p + len;
> +		}
> +		th = (struct acpi_subtable_header *)((unsigned long)
> +		      th + th->length);
> +	}
> +	return sprintf(buf, "%s\n", buf);
> +}
> +
> +static DEVICE_ATTR(immovable_mem, 0444, show_immovable_mem, NULL);
> +
>  static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
>  		unsigned long nr_pages, int online_type,
>  		struct zone *default_zone)
> @@ -798,6 +845,9 @@ static struct attribute *memory_root_attrs[] = {
>  #endif
>  
>  	&dev_attr_block_size_bytes.attr,
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +	&dev_attr_immovable_mem.attr,
> +#endif
>  	&dev_attr_auto_online_blocks.attr,
>  	NULL
>  };
> -- 
> 2.14.3
> 
> 

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

[Chao Fan]

On Fri, Dec 29, 2017 at 01:23:05PM +0100, Michal Hocko wrote:

Hi Michal,

>Always make sure to CC linux-api mailing list when proposing user
>visible API patches.

Sorry for that, since scripts/get_maintainer.pl didn't show that,
so I don't know should Cc that.

>
>On Wed 27-12-17 20:30:12, Chao Fan wrote:
>> In sometimes users specify the memory region in immovable node in
>> some kernel commandline, such as "kernel_core" or the "immovable_mem="
>> in the patchset that I have send. But users don't know the memory
>> region. So add this interface to print it.
>> 
>> It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
>> region, "ss" means the start position of this region.
>
>The patch doesn't explain who is going to use this information and what
>for. Moreover we already do have removable which tells whether the given
>memblock is movable.
>

Thanks for your review, and sorry for my bad explain.
Since my patchset has not neen merged, so I didn't explain the details.
https://patchwork.kernel.org/patch/10106787/
Here is a my patchset. I was trying to add a new parameter to specify
the memory region in immovable node, so that kaslr can work well with
memory hotplug. So we need something can show the immovable memory
regions.
Yes, as you said the removable is based on memblock. But as for this
issue, I want to specify the regions based on node. The memory region
in a NUMA node just contain one or two acpi_srat_mem_affinity in acpi.
But the memblock may have an intersection with two NUMA nodes.
So the data in acpi table is exactly suitable for this issue.
So I write the patch like this.

Thanks,
Chao Fan

>Finally you are pulling an ACPI specific code into a generic code which
>is not acceptable.
>
>Based on all this
>Nacked-by: Michal Hocko <mhocko@suse.com>
>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 1d60b58a8c19..9cadf1a9dccb 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -25,6 +25,7 @@
>>  
>>  #include <linux/atomic.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/acpi.h>
>>  
>>  static DEFINE_MUTEX(mem_sysfs_mutex);
>>  
>> @@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTREMOVE
>> +/*
>> + * Immovable memory region
>> + */
>> +
>> +static ssize_t
>> +show_immovable_mem(struct device *dev, struct device_attribute *attr,
>> +		   char *buf)
>> +{
>> +	struct acpi_table_header *table_header = NULL;
>> +	struct acpi_srat_mem_affinity *ma;
>> +	struct acpi_subtable_header *th;
>> +	unsigned long long table_size;
>> +	unsigned long long table_end;
>> +	char pbuf[35], *p = buf;
>> +	int len;
>> +
>> +	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
>> +
>> +	table_size = sizeof(struct acpi_table_srat);
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +	th = (struct acpi_subtable_header *)((unsigned long)
>> +	      table_header + table_size);
>> +
>> +	while (((unsigned long)th) +
>> +	       sizeof(struct acpi_subtable_header) < table_end) {
>> +		if (th->type == 1) {
>> +			ma = (struct acpi_srat_mem_affinity *)th;
>> +			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
>> +				continue;
>> +			len = sprintf(pbuf, "%llx@%llx",
>> +			       ma->length, ma->base_address);
>> +			if (p != buf) {
>> +				*p = ',';
>> +				p++;
>> +			}
>> +			memcpy(p, pbuf, len);
>> +			p = p + len;
>> +		}
>> +		th = (struct acpi_subtable_header *)((unsigned long)
>> +		      th + th->length);
>> +	}
>> +	return sprintf(buf, "%s\n", buf);
>> +}
>> +
>> +static DEVICE_ATTR(immovable_mem, 0444, show_immovable_mem, NULL);
>> +
>>  static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
>>  		unsigned long nr_pages, int online_type,
>>  		struct zone *default_zone)
>> @@ -798,6 +845,9 @@ static struct attribute *memory_root_attrs[] = {
>>  #endif
>>  
>>  	&dev_attr_block_size_bytes.attr,
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +	&dev_attr_immovable_mem.attr,
>> +#endif
>>  	&dev_attr_auto_online_blocks.attr,
>>  	NULL
>>  };
>> -- 
>> 2.14.3
>> 
>> 
>
>-- 
>Michal Hocko
>SUSE Labs
>
>