Date:	Fri, 15 Jan 2010 11:41:14 +0800
From:	"Zheng, Shaohui" <shaohui.zheng@...el.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>,
	"y-goto@...fujitsu.com" <y-goto@...fujitsu.com>,
	Dave Hansen <haveblue@...ibm.com>,
	"Wu, Fengguang" <fengguang.wu@...el.com>,
	"x86@...nel.org" <x86@...nel.org>
Subject: RE: [PATCH-RESEND v4] memory-hotplug: create /sys/firmware/memmap
 entry for new memory

On Mon, 11 Jan 2010 10:00:11 +0800
"Zheng, Shaohui" <shaohui.zheng@...el.com> wrote:

> Resend the memmap patch v4 to mailing-list after follow up fengguang's review 
> comments. 
> 
> memory-hotplug: create /sys/firmware/memmap entry for hot-added memory
> 
> Interface firmware_map_add was not called in explict, Remove it and add function
> firmware_map_add_hotplug as hotplug interface of memmap.
> 
> When we hot-add new memory, sysfs does not export memmap entry for it. we add
>  a call in function add_memory to function firmware_map_add_hotplug.
> 
> Add a new function add_sysfs_fw_map_entry to create memmap entry, it can avoid 
> duplicated codes.
> 
> Thanks for the careful review from Fengguang Wu and Dave Hansen.
> 

Please describe the format of the proposed sysfs file.  Example output
would be suitable.

> @@ -123,20 +123,40 @@ static int firmware_map_add_entry(u64 start, u64 end,
>  }
>  
>  /**
> - * firmware_map_add() - Adds a firmware mapping entry.
> + * Add memmap entry on sysfs
> + */
> +static int add_sysfs_fw_map_entry(struct firmware_map_entry *entry)
> +{
> +	static int map_entries_nr;
> +	static struct kset *mmap_kset;
> +
> +	if (!mmap_kset) {
> +		mmap_kset = kset_create_and_add("memmap", NULL, firmware_kobj);
> +		if (!mmap_kset)
> +			return -ENOMEM;
> +	}

This is a bit racy if two threads execute it at the same time.  I guess
it doesn't matter.
[Zheng, Shaohui] function add_sysfs_fw_map_entry will be called when OS boots up and we hot-add memory, it never has chance to be execute in two threads, it should be okay.


> +	entry->kobj.kset = mmap_kset;
> +	if (kobject_add(&entry->kobj, NULL, "%d", map_entries_nr++))
> +		kobject_put(&entry->kobj);

hm.  Is this refcounting correct?
[Zheng, Shaohui] all objects of firmware_map_entry shares the same kset, I use a static pointer to store it. When create next entry, we can get the reference easier. I already test it, it works fine.

> +
> +	return 0;
> +}

One caller of add_sysfs_fw_map_entry() is __meminit and the other is
__init.  So this function can be __meminit?
[Zheng, Shaohui] function add_sysfs_fw_map_entry() should be still here after booting up, it will be called when we hot-add new memory, so it can not be _init. __meminit should be the correct since this function is related memory hot-plug.

> +/**
> + * firmware_map_add_early() - Adds a firmware mapping entry.
>   * @start: Start of the memory range.
>   * @end:   End of the memory range (inclusive).
>   * @type:  Type of the memory range.
>   *
> - * This function uses kmalloc() for memory
> - * allocation. Use firmware_map_add_early() if you want to use the bootmem
> - * allocator.
> + * Adds a firmware mapping entry. This function uses the bootmem allocator
> + * for memory allocation.
>   *
>   * That function must be called before late_initcall.
>   *
>   * Returns 0 on success, or -ENOMEM if no memory could be allocated.
>   **/
> -int firmware_map_add(u64 start, u64 end, const char *type)
> +int __init firmware_map_add_early(u64 start, u64 end, const char *type)
>  {
>  	struct firmware_map_entry *entry;
>  
> @@ -148,27 +168,31 @@ int firmware_map_add(u64 start, u64 end, const char *type)
>  }
>  
>  /**
> - * firmware_map_add_early() - Adds a firmware mapping entry.
> + * firmware_map_add_hotplug() - Adds a firmware mapping entry when we do
> + * memory hotplug.
>   * @start: Start of the memory range.
>   * @end:   End of the memory range (inclusive).
>   * @type:  Type of the memory range.
>   *
> - * Adds a firmware mapping entry. This function uses the bootmem allocator
> - * for memory allocation. Use firmware_map_add() if you want to use kmalloc().
> - *
> - * That function must be called before late_initcall.
> + * Adds a firmware mapping entry. This function is for memory hotplug, it is
> + * simiar with function firmware_map_add_early. the only difference is that

s/simiar/similar/
s/with/to/
s/the/The/
s/function firmware_map_add_early/firmware_map_add_early()/
[Zheng, Shaohui] sorry for my carelessness.
	
> + * it will create the syfs entry dynamically.
>   *
>   * Returns 0 on success, or -ENOMEM if no memory could be allocated.
>   **/
> -int __init firmware_map_add_early(u64 start, u64 end, const char *type)
> +int __meminit firmware_map_add_hotplug(u64 start, u64 end, const char *type)
>  {
>  	struct firmware_map_entry *entry;
>  
> -	entry = alloc_bootmem(sizeof(struct firmware_map_entry));
> -	if (WARN_ON(!entry))
> +	entry = kzalloc(sizeof(struct firmware_map_entry), GFP_ATOMIC);
> +	if (!entry)
>  		return -ENOMEM;
>  
> -	return firmware_map_add_entry(start, end, type, entry);
> +	firmware_map_add_entry(start, end, type, entry);
> +	/* create the memmap entry */
> +	add_sysfs_fw_map_entry(entry);
> +
> +	return 0;
>  }
>  
>  /*
> @@ -214,18 +238,10 @@ static ssize_t memmap_attr_show(struct kobject *kobj,
>   */
>  static int __init memmap_init(void)
>  {
> -	int i = 0;
>  	struct firmware_map_entry *entry;
> -	struct kset *memmap_kset;
> -
> -	memmap_kset = kset_create_and_add("memmap", NULL, firmware_kobj);
> -	if (WARN_ON(!memmap_kset))
> -		return -ENOMEM;
>  
>  	list_for_each_entry(entry, &map_entries, list) {
> -		entry->kobj.kset = memmap_kset;
> -		if (kobject_add(&entry->kobj, NULL, "%d", i++))
> -			kobject_put(&entry->kobj);
> +		add_sysfs_fw_map_entry(entry);
>  	}

The braces are now unneeded.  checkpatch used to warn about this I
think.  Either someone broke checkpatch or it doesn't understand
list_for_each_entry().
[Zheng, Shaohui] I will remove the unneeded braces.

>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists