[RFC,v8,08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs

From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type}
sysfs files are created. But there is no code to remove these files. The patch
implements the function to remove them.

Note : The code does not free firmware_map_entry since there is no way to free
       memory which is allocated by bootmem.

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
---
 drivers/firmware/memmap.c    |   78 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/firmware-map.h |    6 +++
 mm/memory_hotplug.c          |    9 ++++-
 3 files changed, 90 insertions(+), 3 deletions(-)

On Tue, 28 Aug 2012 18:00:15 +0800
wency@cn.fujitsu.com wrote:

> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> 
> When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type}
> sysfs files are created. But there is no code to remove these files. The patch
> implements the function to remove them.
> 
> Note : The code does not free firmware_map_entry since there is no way to free
>        memory which is allocated by bootmem.
> 
> ....
>
> +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj)

It would be better to implement this as an inlined C function.  That
has improved type safety and improved readability.

> +static void release_firmware_map_entry(struct kobject *kobj)
> +{
> +	struct firmware_map_entry *entry = to_memmap_entry(kobj);
> +	struct page *page;
> +
> +	page = virt_to_page(entry);
> +	if (PageSlab(page) || PageCompound(page))

That PageCompound() test looks rather odd.  Why is this done?

> +		kfree(entry);
> +
> +	/* There is no way to free memory allocated from bootmem*/
> +}

This function is a bit ugly - poking around in page flags to determine
whether or not the memory came from bootmem.  It would be cleaner to
use a separate boolean.  Although I guess we can live with it as you
have it here.

>  static struct kobj_type memmap_ktype = {
> +	.release	= release_firmware_map_entry,
>  	.sysfs_ops	= &memmap_attr_ops,
>  	.default_attrs	= def_attrs,
>  };
> @@ -123,6 +139,16 @@ static int firmware_map_add_entry(u64 start, u64 end,
>  	return 0;
>  }
>  
> +/**
> + * firmware_map_remove_entry() - Does the real work to remove a firmware
> + * memmap entry.
> + * @entry: removed entry.
> + **/
> +static inline void firmware_map_remove_entry(struct firmware_map_entry *entry)
> +{
> +	list_del(&entry->list);
> +}

Is there no locking  to protect that list?

>  /*
>   * Add memmap entry on sysfs
>   */
> @@ -144,6 +170,31 @@ static int add_sysfs_fw_map_entry(struct firmware_map_entry *entry)
>  	return 0;
>  }
>  
> +/*
> + * Remove memmap entry on sysfs
> + */
> +static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry)
> +{
> +	kobject_put(&entry->kobj);
> +}
> +
> +/*
> + * Search memmap entry
> + */
> +
> +struct firmware_map_entry * __meminit
> +find_firmware_map_entry(u64 start, u64 end, const char *type)

A better name would be firmware_map_find_entry().  To retain the (good)
convention that symbols exported from here all start with
"firmware_map_".

> +{
> +	struct firmware_map_entry *entry;
> +
> +	list_for_each_entry(entry, &map_entries, list)
> +		if ((entry->start == start) && (entry->end == end) &&
> +		    (!strcmp(entry->type, type)))
> +			return entry;
> +
> +	return NULL;
> +}
> +
>  /**
>   * firmware_map_add_hotplug() - Adds a firmware mapping entry when we do
>   * memory hotplug.
> @@ -196,6 +247,32 @@ int __init firmware_map_add_early(u64 start, u64 end, const char *type)
>  	return firmware_map_add_entry(start, end, type, entry);
>  }
>  
> +/**
> + * firmware_map_remove() - remove a firmware mapping entry
> + * @start: Start of the memory range.
> + * @end:   End of the memory range.
> + * @type:  Type of the memory range.
> + *
> + * removes a firmware mapping entry.
> + *
> + * Returns 0 on success, or -EINVAL if no entry.
> + **/
> +int __meminit firmware_map_remove(u64 start, u64 end, const char *type)
> +{
> +	struct firmware_map_entry *entry;
> +
> +	entry = find_firmware_map_entry(start, end - 1, type);
> +	if (!entry)
> +		return -EINVAL;
> +
> +	firmware_map_remove_entry(entry);
> +
> +	/* remove the memmap entry */
> +	remove_sysfs_fw_map_entry(entry);
> +
> +	return 0;
> +}

Again, the lack of locking looks bad.

> ...
>
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1052,9 +1052,9 @@ int offline_memory(u64 start, u64 size)
>  	return 0;
>  }
>  
> -int remove_memory(int nid, u64 start, u64 size)
> +int __ref remove_memory(int nid, u64 start, u64 size)

Why was __ref added?

>  {
> -	int ret = -EBUSY;
> +	int ret = 0;
>  	lock_memory_hotplug();
>  	/*
>  	 * The memory might become online by other task, even if you offine it.
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

At 09/01/2012 05:06 AM, Andrew Morton Wrote:
> On Tue, 28 Aug 2012 18:00:15 +0800
> wency@cn.fujitsu.com wrote:
> 
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type}
>> sysfs files are created. But there is no code to remove these files. The patch
>> implements the function to remove them.
>>
>> Note : The code does not free firmware_map_entry since there is no way to free
>>        memory which is allocated by bootmem.
>>
>> ....
>>
>> +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj)
> 
> It would be better to implement this as an inlined C function.  That
> has improved type safety and improved readability.
> 
>> +static void release_firmware_map_entry(struct kobject *kobj)
>> +{
>> +	struct firmware_map_entry *entry = to_memmap_entry(kobj);
>> +	struct page *page;
>> +
>> +	page = virt_to_page(entry);
>> +	if (PageSlab(page) || PageCompound(page))
> 
> That PageCompound() test looks rather odd.  Why is this done?

Liu Jiang and Christoph Lameter discussed how to find slab page
in this mail:
https://lkml.org/lkml/2012/7/6/333.

> 
>> +		kfree(entry);
>> +
>> +	/* There is no way to free memory allocated from bootmem*/
>> +}
> 
> This function is a bit ugly - poking around in page flags to determine
> whether or not the memory came from bootmem.  It would be cleaner to
> use a separate boolean.  Although I guess we can live with it as you
> have it here.
> 
>>  static struct kobj_type memmap_ktype = {
>> +	.release	= release_firmware_map_entry,
>>  	.sysfs_ops	= &memmap_attr_ops,
>>  	.default_attrs	= def_attrs,
>>  };
>> @@ -123,6 +139,16 @@ static int firmware_map_add_entry(u64 start, u64 end,
>>  	return 0;
>>  }
>>  
>> +/**
>> + * firmware_map_remove_entry() - Does the real work to remove a firmware
>> + * memmap entry.
>> + * @entry: removed entry.
>> + **/
>> +static inline void firmware_map_remove_entry(struct firmware_map_entry *entry)
>> +{
>> +	list_del(&entry->list);
>> +}
> 
> Is there no locking  to protect that list?
> 
>>  /*
>>   * Add memmap entry on sysfs
>>   */
>> @@ -144,6 +170,31 @@ static int add_sysfs_fw_map_entry(struct firmware_map_entry *entry)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Remove memmap entry on sysfs
>> + */
>> +static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry)
>> +{
>> +	kobject_put(&entry->kobj);
>> +}
>> +
>> +/*
>> + * Search memmap entry
>> + */
>> +
>> +struct firmware_map_entry * __meminit
>> +find_firmware_map_entry(u64 start, u64 end, const char *type)
> 
> A better name would be firmware_map_find_entry().  To retain the (good)
> convention that symbols exported from here all start with
> "firmware_map_".

OK.

> 
>> +{
>> +	struct firmware_map_entry *entry;
>> +
>> +	list_for_each_entry(entry, &map_entries, list)
>> +		if ((entry->start == start) && (entry->end == end) &&
>> +		    (!strcmp(entry->type, type)))
>> +			return entry;
>> +
>> +	return NULL;
>> +}
>> +
>>  /**
>>   * firmware_map_add_hotplug() - Adds a firmware mapping entry when we do
>>   * memory hotplug.
>> @@ -196,6 +247,32 @@ int __init firmware_map_add_early(u64 start, u64 end, const char *type)
>>  	return firmware_map_add_entry(start, end, type, entry);
>>  }
>>  
>> +/**
>> + * firmware_map_remove() - remove a firmware mapping entry
>> + * @start: Start of the memory range.
>> + * @end:   End of the memory range.
>> + * @type:  Type of the memory range.
>> + *
>> + * removes a firmware mapping entry.
>> + *
>> + * Returns 0 on success, or -EINVAL if no entry.
>> + **/
>> +int __meminit firmware_map_remove(u64 start, u64 end, const char *type)
>> +{
>> +	struct firmware_map_entry *entry;
>> +
>> +	entry = find_firmware_map_entry(start, end - 1, type);
>> +	if (!entry)
>> +		return -EINVAL;
>> +
>> +	firmware_map_remove_entry(entry);
>> +
>> +	/* remove the memmap entry */
>> +	remove_sysfs_fw_map_entry(entry);
>> +
>> +	return 0;
>> +}
> 
> Again, the lack of locking looks bad.
> 
>> ...
>>
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1052,9 +1052,9 @@ int offline_memory(u64 start, u64 size)
>>  	return 0;
>>  }
>>  
>> -int remove_memory(int nid, u64 start, u64 size)
>> +int __ref remove_memory(int nid, u64 start, u64 size)
> 
> Why was __ref added?

Hmm, firmware_map_remove() was put in meminit section, and we call it
in this function, so __ref is added here.

Thanks
Wen Congyang

> 
>>  {
>> -	int ret = -EBUSY;
>> +	int ret = 0;
>>  	lock_memory_hotplug();
>>  	/*
>>  	 * The memory might become online by other task, even if you offine it.
>>
>> ...
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

At 09/01/2012 05:06 AM, Andrew Morton Wrote:
> On Tue, 28 Aug 2012 18:00:15 +0800
> wency@cn.fujitsu.com wrote:
> 
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type}
>> sysfs files are created. But there is no code to remove these files. The patch
>> implements the function to remove them.
>>
>> Note : The code does not free firmware_map_entry since there is no way to free
>>        memory which is allocated by bootmem.
>>
>> ....
>>
>> +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj)
> 
> It would be better to implement this as an inlined C function.  That
> has improved type safety and improved readability.

Hmm, this macro is not a new macro. It is defined after the function
release_firmware_map_entry(). We just moved it here because we
need it in the function release_firmware_map_entry().

> 
>> +static void release_firmware_map_entry(struct kobject *kobj)
>> +{
>> +	struct firmware_map_entry *entry = to_memmap_entry(kobj);
>> +	struct page *page;
>> +
>> +	page = virt_to_page(entry);
>> +	if (PageSlab(page) || PageCompound(page))
> 
> That PageCompound() test looks rather odd.  Why is this done?
> 
>> +		kfree(entry);
>> +
>> +	/* There is no way to free memory allocated from bootmem*/
>> +}
> 
> This function is a bit ugly - poking around in page flags to determine
> whether or not the memory came from bootmem.  It would be cleaner to
> use a separate boolean.  Although I guess we can live with it as you
> have it here.
> 
>>  static struct kobj_type memmap_ktype = {
>> +	.release	= release_firmware_map_entry,
>>  	.sysfs_ops	= &memmap_attr_ops,
>>  	.default_attrs	= def_attrs,
>>  };
>> @@ -123,6 +139,16 @@ static int firmware_map_add_entry(u64 start, u64 end,
>>  	return 0;
>>  }
>>  
>> +/**
>> + * firmware_map_remove_entry() - Does the real work to remove a firmware
>> + * memmap entry.
>> + * @entry: removed entry.
>> + **/
>> +static inline void firmware_map_remove_entry(struct firmware_map_entry *entry)
>> +{
>> +	list_del(&entry->list);
>> +}
> 
> Is there no locking  to protect that list?

OK, I will add a lock to protect it.

Thanks
Wen Congyang

> 
>>  /*
>>   * Add memmap entry on sysfs
>>   */
>> @@ -144,6 +170,31 @@ static int add_sysfs_fw_map_entry(struct firmware_map_entry *entry)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Remove memmap entry on sysfs
>> + */
>> +static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry)
>> +{
>> +	kobject_put(&entry->kobj);
>> +}
>> +
>> +/*
>> + * Search memmap entry
>> + */
>> +
>> +struct firmware_map_entry * __meminit
>> +find_firmware_map_entry(u64 start, u64 end, const char *type)
> 
> A better name would be firmware_map_find_entry().  To retain the (good)
> convention that symbols exported from here all start with
> "firmware_map_".
> 
>> +{
>> +	struct firmware_map_entry *entry;
>> +
>> +	list_for_each_entry(entry, &map_entries, list)
>> +		if ((entry->start == start) && (entry->end == end) &&
>> +		    (!strcmp(entry->type, type)))
>> +			return entry;
>> +
>> +	return NULL;
>> +}
>> +
>>  /**
>>   * firmware_map_add_hotplug() - Adds a firmware mapping entry when we do
>>   * memory hotplug.
>> @@ -196,6 +247,32 @@ int __init firmware_map_add_early(u64 start, u64 end, const char *type)
>>  	return firmware_map_add_entry(start, end, type, entry);
>>  }
>>  
>> +/**
>> + * firmware_map_remove() - remove a firmware mapping entry
>> + * @start: Start of the memory range.
>> + * @end:   End of the memory range.
>> + * @type:  Type of the memory range.
>> + *
>> + * removes a firmware mapping entry.
>> + *
>> + * Returns 0 on success, or -EINVAL if no entry.
>> + **/
>> +int __meminit firmware_map_remove(u64 start, u64 end, const char *type)
>> +{
>> +	struct firmware_map_entry *entry;
>> +
>> +	entry = find_firmware_map_entry(start, end - 1, type);
>> +	if (!entry)
>> +		return -EINVAL;
>> +
>> +	firmware_map_remove_entry(entry);
>> +
>> +	/* remove the memmap entry */
>> +	remove_sysfs_fw_map_entry(entry);
>> +
>> +	return 0;
>> +}
> 
> Again, the lack of locking looks bad.
> 
>> ...
>>
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1052,9 +1052,9 @@ int offline_memory(u64 start, u64 size)
>>  	return 0;
>>  }
>>  
>> -int remove_memory(int nid, u64 start, u64 size)
>> +int __ref remove_memory(int nid, u64 start, u64 size)
> 
> Why was __ref added?
> 
>>  {
>> -	int ret = -EBUSY;
>> +	int ret = 0;
>>  	lock_memory_hotplug();
>>  	/*
>>  	 * The memory might become online by other task, even if you offine it.
>>
>> ...
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 03 Sep 2012 13:51:10 +0800
Wen Congyang <wency@cn.fujitsu.com> wrote:

> >> +static void release_firmware_map_entry(struct kobject *kobj)
> >> +{
> >> +	struct firmware_map_entry *entry = to_memmap_entry(kobj);
> >> +	struct page *page;
> >> +
> >> +	page = virt_to_page(entry);
> >> +	if (PageSlab(page) || PageCompound(page))
> > 
> > That PageCompound() test looks rather odd.  Why is this done?
> 
> Liu Jiang and Christoph Lameter discussed how to find slab page
> in this mail:
> https://lkml.org/lkml/2012/7/6/333.

Well, please add a code comment to release_firmware_map_entry() which
fully explains these things.

I see that Christoph and I agree: "It would be cleaner if memory
hotplug had an indicator which allocation mechanism was used and would
use the corresponding free action".  You didn't respond to this
suggestion when he made it, nor when I made it.  What are your thoughts
on this?

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

At 09/05/2012 07:16 AM, Andrew Morton Wrote:
> On Mon, 03 Sep 2012 13:51:10 +0800
> Wen Congyang <wency@cn.fujitsu.com> wrote:
> 
>>>> +static void release_firmware_map_entry(struct kobject *kobj)
>>>> +{
>>>> +	struct firmware_map_entry *entry = to_memmap_entry(kobj);
>>>> +	struct page *page;
>>>> +
>>>> +	page = virt_to_page(entry);
>>>> +	if (PageSlab(page) || PageCompound(page))
>>>
>>> That PageCompound() test looks rather odd.  Why is this done?
>>
>> Liu Jiang and Christoph Lameter discussed how to find slab page
>> in this mail:
>> https://lkml.org/lkml/2012/7/6/333.
> 
> Well, please add a code comment to release_firmware_map_entry() which
> fully explains these things.
> 
> I see that Christoph and I agree: "It would be cleaner if memory
> hotplug had an indicator which allocation mechanism was used and would
> use the corresponding free action".  You didn't respond to this
> suggestion when he made it, nor when I made it.  What are your thoughts
> on this?

Hmm, I think it is better to use an indicator which allocation mechanism was
used. I will do it in the next version.

Thanks
Wen Congyang
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html