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

[Michal Hocko <mhocko@kernel.org>]

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