lkml.org 
[lkml]   [2010]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@intel.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.


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

hm. Is this refcounting correct?

> +
> + return 0;
> +}

One caller of add_sysfs_fw_map_entry() is __meminit and the other is
__init. So this function can be __meminit?

> +/**
> + * 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()/

> + * 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().

>
> ...
>


\
 
 \ /
  Last update: 2010-01-13 23:33    [from the cache]
©2003-2011 Jasper Spaans