Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
On Mon, 03 Sep 2012 13:51:10 +0800
Wen Congyang we...@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?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
At 09/05/2012 07:16 AM, Andrew Morton Wrote:
On Mon, 03 Sep 2012 13:51:10 +0800
Wen Congyang we...@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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
At 09/01/2012 05:06 AM, Andrew Morton Wrote:
On Tue, 28 Aug 2012 18:00:15 +0800
we...@cn.fujitsu.com wrote:
From: Yasuaki Ishimatsu isimatu.yasu...@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.
...
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
At 09/01/2012 05:06 AM, Andrew Morton Wrote:
On Tue, 28 Aug 2012 18:00:15 +0800
we...@cn.fujitsu.com wrote:
From: Yasuaki Ishimatsu isimatu.yasu...@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.
...
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
On Tue, 28 Aug 2012 18:00:15 +0800
we...@cn.fujitsu.com wrote:
From: Yasuaki Ishimatsu isimatu.yasu...@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.
...
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[RFC v8 PATCH 08/20] memory-hotplug: remove /sys/firmware/memmap/X sysfs
From: Yasuaki Ishimatsu isimatu.yasu...@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 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(-)
diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
index c1cdc92..b2e7e5e 100644
--- a/drivers/firmware/memmap.c
+++ b/drivers/firmware/memmap.c
@@ -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_attr_ops = {
.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);
+
+ /* 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 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);
+}
+
/*
* 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)
+{
+ 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;
+}
+
/*
* Sysfs functions
-
*/
@@ -218,7 +295,6 @@ static ssize_t type_show(struct firmware_map_entry *entry,
char *buf)
}
#define to_memmap_attr(_attr) container_of(_attr, struct memmap_attribute,
attr)
-#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj)
static ssize_t memmap_attr_show(struct kobject *kobj,
struct attribute *attr, char *buf)
diff --git a/include/linux/firmware-map.h b/include/linux/firmware-map.h
index 43fe52f..71d4fa7 100644
--- a/include/linux/firmware-map.h
+++ b/include/linux/firmware-map.h
@@ -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,
