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

[Chao Fan <fanc.fnst@cn.fujitsu.com>]

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