Re: [RFC PATCH v4 4/13] memory-hotplug : remove /sys/firmware/memmap/X sysfs

2012-07-18 Thread Wen Congyang
At 07/18/2012 06:09 PM, Yasuaki Ishimatsu Wrote:
 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 rient...@google.com
 CC: Jiang Liu liu...@gmail.com
 CC: Len Brown len.br...@intel.com
 CC: Benjamin Herrenschmidt b...@kernel.crashing.org
 CC: Paul Mackerras pau...@samba.org 
 CC: Christoph Lameter c...@linux.com
 Cc: Minchan Kim minchan@gmail.com
 CC: Andrew Morton a...@linux-foundation.org
 CC: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com 
 CC: Wen Congyang we...@cn.fujitsu.com
 Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@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(-)
 
 Index: linux-3.5-rc6/mm/memory_hotplug.c
 ===
 --- linux-3.5-rc6.orig/mm/memory_hotplug.c2012-07-18 17:20:05.670024283 
 +0900
 +++ linux-3.5-rc6/mm/memory_hotplug.c 2012-07-18 17:51:03.933189930 +0900
 @@ -1012,9 +1012,9 @@ int offline_memory(u64 start, u64 size)
   return offline_pages(start_pfn, end_pfn, 120 * HZ);
  }
  
 -int remove_memory(int nid, u64 start, u64 size)
 +int __ref remove_memory(int nid, u64 start, u64 size)
  {
 - int ret = -EBUSY;
 + int ret = 0;
   lock_memory_hotplug();
   /*
* The memory might become online by other task, even if you offine it.
 @@ -1025,8 +1025,13 @@ int remove_memory(int nid, u64 start, u6
   because the memmory range is online\n,
   start, start + size);
   ret = -EAGAIN;
 + goto out;
   }
  
 + /* remove memmap entry */
 + firmware_map_remove(start, start + size, System RAM);
 +
 +out:
   unlock_memory_hotplug();
   return ret;
  
 Index: linux-3.5-rc6/include/linux/firmware-map.h
 ===
 --- linux-3.5-rc6.orig/include/linux/firmware-map.h   2012-07-18 
 17:19:37.007382563 +0900
 +++ linux-3.5-rc6/include/linux/firmware-map.h2012-07-18 
 17:42:20.804730245 +0900
 @@ -25,6 +25,7 @@
  
  int firmware_map_add_early(u64 start, u64 end, const char *type);
  int firmware_map_add_hotplug(u64 start, u64 end, const char *type);
 +int firmware_map_remove(u64 start, u64 end, const char *type);
  
  #else /* CONFIG_FIRMWARE_MEMMAP */
  
 @@ -38,6 +39,11 @@ static inline int firmware_map_add_hotpl
   return 0;
  }
  
 +static inline int firmware_map_remove(u64 start, u64 end, const char *type)
 +{
 + return 0;
 +}
 +
  #endif /* CONFIG_FIRMWARE_MEMMAP */
  
  #endif /* _LINUX_FIRMWARE_MAP_H */
 Index: linux-3.5-rc6/drivers/firmware/memmap.c
 ===
 --- linux-3.5-rc6.orig/drivers/firmware/memmap.c  2012-07-18 
 17:19:43.618300182 +0900
 +++ linux-3.5-rc6/drivers/firmware/memmap.c   2012-07-18 17:42:20.846729721 
 +0900
 @@ -21,6 +21,7 @@
  #include linux/types.h
  #include linux/bootmem.h
  #include linux/slab.h
 +#include linux/mm.h
  
  /*
   * Data types 
 --
 @@ -79,7 +80,22 @@ static const struct sysfs_ops memmap_att
   .show = memmap_attr_show,
  };
  
 +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, 
 kobj)
 +
 +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))
 + kfree(entry);

IIRC, this function's implementation is changed. Why do you do it?
If PageCompound(page), should we check page-first_page's flags?

Thanks
Wen Congyang

 +
 + /* There is no way to free memory allocated from bootmem*/
 +}
 +
  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 st
   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);
 +}
 +
  /*
   * Add memmap entry on sysfs
   */
 @@ -144,6 +170,31 @@ static int add_sysfs_fw_map_entry(struct
   return 0;
  }
  
 +/*
 + * Remove memmap entry on sysfs
 + */
 +static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry 
 *entry)
 +{
 + 

Re: [RFC PATCH v4 4/13] memory-hotplug : remove /sys/firmware/memmap/X sysfs

2012-07-18 Thread Yasuaki Ishimatsu
Hi Wen,

2012/07/18 19:33, Wen Congyang wrote:
 At 07/18/2012 06:09 PM, Yasuaki Ishimatsu Wrote:
 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 rient...@google.com
 CC: Jiang Liu liu...@gmail.com
 CC: Len Brown len.br...@intel.com
 CC: Benjamin Herrenschmidt b...@kernel.crashing.org
 CC: Paul Mackerras pau...@samba.org
 CC: Christoph Lameter c...@linux.com
 Cc: Minchan Kim minchan@gmail.com
 CC: Andrew Morton a...@linux-foundation.org
 CC: KOSAKI Motohiro kosaki.motoh...@jp.fujitsu.com
 CC: Wen Congyang we...@cn.fujitsu.com
 Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@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(-)

 Index: linux-3.5-rc6/mm/memory_hotplug.c
 ===
 --- linux-3.5-rc6.orig/mm/memory_hotplug.c   2012-07-18 17:20:05.670024283 
 +0900
 +++ linux-3.5-rc6/mm/memory_hotplug.c2012-07-18 17:51:03.933189930 
 +0900
 @@ -1012,9 +1012,9 @@ int offline_memory(u64 start, u64 size)
  return offline_pages(start_pfn, end_pfn, 120 * HZ);
   }
   
 -int remove_memory(int nid, u64 start, u64 size)
 +int __ref remove_memory(int nid, u64 start, u64 size)
   {
 -int ret = -EBUSY;
 +int ret = 0;
  lock_memory_hotplug();
  /*
   * The memory might become online by other task, even if you offine it.
 @@ -1025,8 +1025,13 @@ int remove_memory(int nid, u64 start, u6
  because the memmory range is online\n,
  start, start + size);
  ret = -EAGAIN;
 +goto out;
  }
   
 +/* remove memmap entry */
 +firmware_map_remove(start, start + size, System RAM);
 +
 +out:
  unlock_memory_hotplug();
  return ret;
   
 Index: linux-3.5-rc6/include/linux/firmware-map.h
 ===
 --- linux-3.5-rc6.orig/include/linux/firmware-map.h  2012-07-18 
 17:19:37.007382563 +0900
 +++ linux-3.5-rc6/include/linux/firmware-map.h   2012-07-18 
 17:42:20.804730245 +0900
 @@ -25,6 +25,7 @@
   
   int firmware_map_add_early(u64 start, u64 end, const char *type);
   int firmware_map_add_hotplug(u64 start, u64 end, const char *type);
 +int firmware_map_remove(u64 start, u64 end, const char *type);
   
   #else /* CONFIG_FIRMWARE_MEMMAP */
   
 @@ -38,6 +39,11 @@ static inline int firmware_map_add_hotpl
  return 0;
   }
   
 +static inline int firmware_map_remove(u64 start, u64 end, const char *type)
 +{
 +return 0;
 +}
 +
   #endif /* CONFIG_FIRMWARE_MEMMAP */
   
   #endif /* _LINUX_FIRMWARE_MAP_H */
 Index: linux-3.5-rc6/drivers/firmware/memmap.c
 ===
 --- linux-3.5-rc6.orig/drivers/firmware/memmap.c 2012-07-18 
 17:19:43.618300182 +0900
 +++ linux-3.5-rc6/drivers/firmware/memmap.c  2012-07-18 17:42:20.846729721 
 +0900
 @@ -21,6 +21,7 @@
   #include linux/types.h
   #include linux/bootmem.h
   #include linux/slab.h
 +#include linux/mm.h
   
   /*
* Data types 
 --
 @@ -79,7 +80,22 @@ static const struct sysfs_ops memmap_att
  .show = memmap_attr_show,
   };
   
 +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, 
 kobj)
 +
 +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))
 +kfree(entry);
 
 IIRC, this function's implementation is changed. Why do you do it?
 If PageCompound(page), should we check page-first_page's flags?

I forgot to write the change to change log. Jiang and Christoph discussed
how to find the slab page:

- https://lkml.org/lkml/2012/7/6/333

Then, Christoph proposed this method.  So I changed it.

Thanks,
Yasuaki Ishimatsu

 
 Thanks
 Wen Congyang
 
 +
 +/* There is no way to free memory allocated from bootmem*/
 +}
 +
   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 st
  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)
 +{
 +