Skip to contentSkip to conversation

memory_hotplug: Free pages as higher order

PATCH Superseded Arun KS 3 participants 9 messages Sep 27, 2018

Subsystems (6)
Memory management MM core Page allocator Xen hypervisor interface +2 more subsystems

v3 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Arun KS

author
#0 lore raw

PATCH v3 1/1 memory_hotplug: Free pages as higher order

When free pages are done with higher order, time spend on coalescing pages by buddy allocator can be reduced. With section size of 256MB, hot add latency of a single section shows improvement from 50-60 ms to less than 1 ms, hence improving the hot add latency by 60%.

Modify external providers of online callback to align with the change.

Signed-off-by Arun KS <[email protected]>

---
Changes since v2:
reuse code from __free_pages_boot_core()

Changes since v1:
- Removed prefetch()

Changes since RFC:
- Rebase.
- As suggested by Michal Hocko remove pages_per_block.
- Modifed external providers of online_page_callback.

v2: https://lore.kernel.org/patchwork/patch/991363/
v1: https://lore.kernel.org/patchwork/patch/989445/
RFC: https://lore.kernel.org/patchwork/patch/984754/

---
 drivers/hv/hv_balloon.c        |  6 ++++--
 drivers/xen/balloon.c          | 18 ++++++++++++++---
 include/linux/memory_hotplug.h |  2 +-
 mm/internal.h                  |  1 +
 mm/memory_hotplug.c            | 44 ++++++++++++++++++++++++++++++------------
 mm/page_alloc.c                |  2 +-
 6 files changed, 54 insertions(+), 19 deletions(-)
drivers/hv/hv_balloon.c +4 -2
index b1b7880..c5bc0b5 100644
@@ -771,7 +771,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 	}
 }
 
-static void hv_online_page(struct page *pg)
+static int hv_online_page(struct page *pg, unsigned int order)
 {
 	struct hv_hotadd_state *has;
 	unsigned long flags;

@@ -783,10 +783,12 @@ static void hv_online_page(struct page *pg)
 		if ((pfn < has->start_pfn) || (pfn >= has->end_pfn))
 			continue;
 
-		hv_page_online_one(has, pg);
+		hv_bring_pgs_online(has, pfn, (1UL << order));
 		break;
 	}
 	spin_unlock_irqrestore(&dm_device.ha_lock, flags);
+
+	return 0;
 }
 
 static int pfn_covered(unsigned long start_pfn, unsigned long pfn_cnt)
drivers/xen/balloon.c +15 -3
index e12bb25..010cf4d 100644
@@ -390,8 +390,8 @@ static enum bp_state reserve_additional_memory(void)
 
 	/*
 	 * add_memory_resource() will call online_pages() which in its turn
-	 * will call xen_online_page() callback causing deadlock if we don't
-	 * release balloon_mutex here. Unlocking here is safe because the
+	 * will call xen_bring_pgs_online() callback causing deadlock if we
+	 * don't release balloon_mutex here. Unlocking here is safe because the
 	 * callers drop the mutex before trying again.
 	 */
 	mutex_unlock(&balloon_mutex);

@@ -422,6 +422,18 @@ static void xen_online_page(struct page *page)
 	mutex_unlock(&balloon_mutex);
 }
 
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
+{
+	unsigned long i, size = (1 << order);
+	unsigned long start_pfn = page_to_pfn(pg);
+
+	pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, start_pfn);
+	for (i = 0; i < size; i++)
+		xen_online_page(pfn_to_page(start_pfn + i));
+
+	return 0;
+}
+
 static int xen_memory_notifier(struct notifier_block *nb, unsigned long val, void *v)
 {
 	if (val == MEM_ONLINE)

@@ -744,7 +756,7 @@ static int __init balloon_init(void)
 	balloon_stats.max_retry_count = RETRY_UNLIMITED;
 
 #ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
-	set_online_page_callback(&xen_online_page);
+	set_online_page_callback(&xen_bring_pgs_online);
 	register_memory_notifier(&xen_memory_nb);
 	register_sysctl_table(xen_root);
include/linux/memory_hotplug.h +1 -1
index 34a2822..7b04c1d 100644
@@ -87,7 +87,7 @@ extern int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn,
 	unsigned long *valid_start, unsigned long *valid_end);
 extern void __offline_isolated_pages(unsigned long, unsigned long);
 
-typedef void (*online_page_callback_t)(struct page *page);
+typedef int (*online_page_callback_t)(struct page *page, unsigned int order);
 
 extern int set_online_page_callback(online_page_callback_t callback);
 extern int restore_online_page_callback(online_page_callback_t callback);
mm/internal.h +1 -0
index 87256ae..2b0efac 100644
@@ -163,6 +163,7 @@ static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
 extern int __isolate_free_page(struct page *page, unsigned int order);
 extern void __free_pages_bootmem(struct page *page, unsigned long pfn,
 					unsigned int order);
+extern void __free_pages_boot_core(struct page *page, unsigned int order);
 extern void prep_compound_page(struct page *page, unsigned int order);
 extern void post_alloc_hook(struct page *page, unsigned int order,
 					gfp_t gfp_flags);
mm/memory_hotplug.c +32 -12

65 lines not on this page · raw message

mm/page_alloc.c +1 -1
index 89d2a2a..a442381 100644
@@ -1252,7 +1252,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	local_irq_restore(flags);
 }
 
-static void __init __free_pages_boot_core(struct page *page, unsigned int order)
+void __free_pages_boot_core(struct page *page, unsigned int order)
 {
 	unsigned int nr_pages = 1 << order;
 	struct page *p = page;
Signature · 1 line
1.9.1
#1 lore raw

mm/memory_hotplug.c void __online_page_free(struct page *page) · line 660

Quoted 1 line · Arun KS
mm/memory_hotplug.c +1 -0
+	__free_pages_boot_core(page, order);

I am not sure, but if we are going to use that function from the memory-hotplug code, we might want to rename that function to something more generic?
The word "boot" suggests that this is only called from the boot stage.

And what about the prefetch operations?
I saw that you removed them in your previous patch and that had some benefits [1].

Should we remove them here as well?

[1] https://patchwork.kernel.org/patch/10613359/

Thanks

Signature · 2 lines
Oscar Salvador
SUSE L3
author
#2 lore raw
Quoted 3 lines · Oscar Salvador

mm/memory_hotplug.c void __online_page_free(struct page *page) · line 660

Quoted 1 line · Arun KS
mm/memory_hotplug.c +1 -0
+	__free_pages_boot_core(page, order);

Hi,

Quoted 4 lines

I am not sure, but if we are going to use that function from the memory-hotplug code,
we might want to rename that function to something more generic? The word "boot" suggests that this is only called from the boot stage.

I ll rename it to __free_pages_core()

Quoted 6 lines

And what about the prefetch operations?
I saw that you removed them in your previous patch and that had some benefits [1].

Should we remove them here as well?

Sure. Will update this as well.

Thanks,
Arun

Quoted 4 lines

[1] https://patchwork.kernel.org/patch/10613359/

Thanks

author
#3 lore raw
In reply to Oscar Salvador
Quoted 3 lines · Oscar Salvador

mm/memory_hotplug.c void __online_page_free(struct page *page) · line 660

Quoted 1 line · Arun KS
mm/memory_hotplug.c +1 -0
+	__free_pages_boot_core(page, order);

Hi,

Quoted 4 lines

I am not sure, but if we are going to use that function from the memory-hotplug code,
we might want to rename that function to something more generic? The word "boot" suggests that this is only called from the boot stage.

I ll rename it to __free_pages_core()

Quoted 6 lines

And what about the prefetch operations?
I saw that you removed them in your previous patch and that had some benefits [1].

Should we remove them here as well?

Sure. Will update this as well.

Thanks,
Arun

Quoted 4 lines

[1] https://patchwork.kernel.org/patch/10613359/

Thanks

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#4 lore raw
In reply to Arun KS

mm/memory_hotplug.c void __online_page_free(struct page *page) · line 660

Quoted 1 line · Arun KS
mm/memory_hotplug.c +1 -0
+	__free_pages_boot_core(page, order);

I am not sure, but if we are going to use that function from the memory-hotplug code, we might want to rename that function to something more generic?
The word "boot" suggests that this is only called from the boot stage.

And what about the prefetch operations?
I saw that you removed them in your previous patch and that had some benefits [1].

Should we remove them here as well?

[1] https://patchwork.kernel.org/patch/10613359/

Thanks

Signature · 7 lines
Oscar Salvador
SUSE L3

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
maintainer
#5 lore raw
In reply to Arun KS
Quoted 35 lines · Arun KS

When free pages are done with higher order, time spend on coalescing pages by buddy allocator can be reduced. With section size of 256MB, hot add latency of a single section shows improvement from 50-60 ms to less than 1 ms, hence improving the hot add latency by 60%.

Modify external providers of online callback to align with the change.

Signed-off-by Arun KS <[email protected]>

---
Changes since v2:
reuse code from __free_pages_boot_core()

Changes since v1:
- Removed prefetch()

Changes since RFC:
- Rebase.
- As suggested by Michal Hocko remove pages_per_block.
- Modifed external providers of online_page_callback.

v2: https://lore.kernel.org/patchwork/patch/991363/
v1: https://lore.kernel.org/patchwork/patch/989445/
RFC: https://lore.kernel.org/patchwork/patch/984754/

---
 drivers/hv/hv_balloon.c        |  6 ++++--
 drivers/xen/balloon.c          | 18 ++++++++++++++---
 include/linux/memory_hotplug.h |  2 +-
 mm/internal.h                  |  1 +
 mm/memory_hotplug.c            | 44 ++++++++++++++++++++++++++++++------------
 mm/page_alloc.c                |  2 +-
 6 files changed, 54 insertions(+), 19 deletions(-)

...

Quoted 17 lines
drivers/xen/balloon.c +2 -2
index e12bb25..010cf4d 100644
@@ -390,8 +390,8 @@ static enum bp_state reserve_additional_memory(void)
 
 	/*
 	 * add_memory_resource() will call online_pages() which in its turn
-	 * will call xen_online_page() callback causing deadlock if we don't
-	 * release balloon_mutex here. Unlocking here is safe because the
+	 * will call xen_bring_pgs_online() callback causing deadlock if we
+	 * don't release balloon_mutex here. Unlocking here is safe because the
 	 * callers drop the mutex before trying again.
 	 */
 	mutex_unlock(&balloon_mutex);

@@ -422,6 +422,18 @@ static void xen_online_page(struct page *page)
 	mutex_unlock(&balloon_mutex);

drivers/xen/balloon.c static void xen_online_page(struct page *page) · line 432

Quoted 10 lines
drivers/xen/balloon.c +8 -0
 }
 
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
+{
+	unsigned long i, size = (1 << order);
+	unsigned long start_pfn = page_to_pfn(pg);
+
+	pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, start_pfn);
+	for (i = 0; i < size; i++)
+		xen_online_page(pfn_to_page(start_pfn + i));

xen_online_page() isn't very complex and this is the only user.

Why don't you move its body in here and drop the extra function? And now you can execute the loop with balloon_mutex held instead of taking and releasing it in each iteration of the loop.

Juergen

author
#6 lore raw
Quoted 61 lines · Juergen Gross
Quoted 36 lines · Arun KS

When free pages are done with higher order, time spend on coalescing pages by buddy allocator can be reduced. With section size of 256MB, hot add latency of a single section shows improvement from 50-60 ms to less than 1 ms, hence improving the hot add latency by 60%.

Modify external providers of online callback to align with the change.

Signed-off-by Arun KS <[email protected]>

---
Changes since v2:
reuse code from __free_pages_boot_core()

Changes since v1:
- Removed prefetch()

Changes since RFC:
- Rebase.
- As suggested by Michal Hocko remove pages_per_block.
- Modifed external providers of online_page_callback.

v2: https://lore.kernel.org/patchwork/patch/991363/
v1: https://lore.kernel.org/patchwork/patch/989445/
RFC: https://lore.kernel.org/patchwork/patch/984754/

---
 drivers/hv/hv_balloon.c        |  6 ++++--
 drivers/xen/balloon.c          | 18 ++++++++++++++---
 include/linux/memory_hotplug.h |  2 +-
 mm/internal.h                  |  1 +
 mm/memory_hotplug.c            | 44 
++++++++++++++++++++++++++++++------------
 mm/page_alloc.c                |  2 +-
 6 files changed, 54 insertions(+), 19 deletions(-)

...

Quoted 21 lines
drivers/xen/balloon.c +2 -2
index e12bb25..010cf4d 100644
@@ -390,8 +390,8 @@ static enum bp_state 
 reserve_additional_memory(void)
 
 	/*
 	 * add_memory_resource() will call online_pages() which in its turn
-	 * will call xen_online_page() callback causing deadlock if we don't
-	 * release balloon_mutex here. Unlocking here is safe because the
+	 * will call xen_bring_pgs_online() callback causing deadlock if we
+	 * don't release balloon_mutex here. Unlocking here is safe because

the

drivers/xen/balloon.c +0 -0
 	 * callers drop the mutex before trying again.
 	 */
 	mutex_unlock(&balloon_mutex);

@@ -422,6 +422,18 @@ static void xen_online_page(struct page *page)
 	mutex_unlock(&balloon_mutex);
 }
Quoted 10 lines

drivers/xen/balloon.c static void xen_online_page(struct page *page) · line 432

Quoted 9 lines
drivers/xen/balloon.c +8 -0
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
+{
+	unsigned long i, size = (1 << order);
+	unsigned long start_pfn = page_to_pfn(pg);
+
+	pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, 
 start_pfn);
+	for (i = 0; i < size; i++)
+		xen_online_page(pfn_to_page(start_pfn + i));

Hi,

Quoted 5 lines

xen_online_page() isn't very complex and this is the only user.

Why don't you move its body in here and drop the extra function? And now you can execute the loop with balloon_mutex held instead of taking and releasing it in each iteration of the loop.

Point taken. Will incorporate them.

Regards,
Arun

Quoted 3 lines

Juergen

author
#7 lore raw
In reply to Juergen Gross
Quoted 61 lines · Juergen Gross
Quoted 36 lines · Arun KS

When free pages are done with higher order, time spend on coalescing pages by buddy allocator can be reduced. With section size of 256MB, hot add latency of a single section shows improvement from 50-60 ms to less than 1 ms, hence improving the hot add latency by 60%.

Modify external providers of online callback to align with the change.

Signed-off-by Arun KS <[email protected]>

---
Changes since v2:
reuse code from __free_pages_boot_core()

Changes since v1:
- Removed prefetch()

Changes since RFC:
- Rebase.
- As suggested by Michal Hocko remove pages_per_block.
- Modifed external providers of online_page_callback.

v2: https://lore.kernel.org/patchwork/patch/991363/
v1: https://lore.kernel.org/patchwork/patch/989445/
RFC: https://lore.kernel.org/patchwork/patch/984754/

---
 drivers/hv/hv_balloon.c        |  6 ++++--
 drivers/xen/balloon.c          | 18 ++++++++++++++---
 include/linux/memory_hotplug.h |  2 +-
 mm/internal.h                  |  1 +
 mm/memory_hotplug.c            | 44 
++++++++++++++++++++++++++++++------------
 mm/page_alloc.c                |  2 +-
 6 files changed, 54 insertions(+), 19 deletions(-)

...

Quoted 21 lines
drivers/xen/balloon.c +2 -2
index e12bb25..010cf4d 100644
@@ -390,8 +390,8 @@ static enum bp_state 
 reserve_additional_memory(void)
 
 	/*
 	 * add_memory_resource() will call online_pages() which in its turn
-	 * will call xen_online_page() callback causing deadlock if we don't
-	 * release balloon_mutex here. Unlocking here is safe because the
+	 * will call xen_bring_pgs_online() callback causing deadlock if we
+	 * don't release balloon_mutex here. Unlocking here is safe because

the

drivers/xen/balloon.c +0 -0
 	 * callers drop the mutex before trying again.
 	 */
 	mutex_unlock(&balloon_mutex);

@@ -422,6 +422,18 @@ static void xen_online_page(struct page *page)
 	mutex_unlock(&balloon_mutex);
 }
Quoted 10 lines

drivers/xen/balloon.c static void xen_online_page(struct page *page) · line 432

Quoted 9 lines
drivers/xen/balloon.c +8 -0
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
+{
+	unsigned long i, size = (1 << order);
+	unsigned long start_pfn = page_to_pfn(pg);
+
+	pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, 
 start_pfn);
+	for (i = 0; i < size; i++)
+		xen_online_page(pfn_to_page(start_pfn + i));

Hi,

Quoted 5 lines

xen_online_page() isn't very complex and this is the only user.

Why don't you move its body in here and drop the extra function? And now you can execute the loop with balloon_mutex held instead of taking and releasing it in each iteration of the loop.

Point taken. Will incorporate them.

Regards,
Arun

Quoted 3 lines

Juergen

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

maintainer
#8 lore raw
In reply to Arun KS
Quoted 35 lines · Arun KS

When free pages are done with higher order, time spend on coalescing pages by buddy allocator can be reduced. With section size of 256MB, hot add latency of a single section shows improvement from 50-60 ms to less than 1 ms, hence improving the hot add latency by 60%.

Modify external providers of online callback to align with the change.

Signed-off-by Arun KS <[email protected]>

---
Changes since v2:
reuse code from __free_pages_boot_core()

Changes since v1:
- Removed prefetch()

Changes since RFC:
- Rebase.
- As suggested by Michal Hocko remove pages_per_block.
- Modifed external providers of online_page_callback.

v2: https://lore.kernel.org/patchwork/patch/991363/
v1: https://lore.kernel.org/patchwork/patch/989445/
RFC: https://lore.kernel.org/patchwork/patch/984754/

---
 drivers/hv/hv_balloon.c        |  6 ++++--
 drivers/xen/balloon.c          | 18 ++++++++++++++---
 include/linux/memory_hotplug.h |  2 +-
 mm/internal.h                  |  1 +
 mm/memory_hotplug.c            | 44 ++++++++++++++++++++++++++++++------------
 mm/page_alloc.c                |  2 +-
 6 files changed, 54 insertions(+), 19 deletions(-)

...

Quoted 17 lines
drivers/xen/balloon.c +2 -2
index e12bb25..010cf4d 100644
@@ -390,8 +390,8 @@ static enum bp_state reserve_additional_memory(void)
 
 	/*
 	 * add_memory_resource() will call online_pages() which in its turn
-	 * will call xen_online_page() callback causing deadlock if we don't
-	 * release balloon_mutex here. Unlocking here is safe because the
+	 * will call xen_bring_pgs_online() callback causing deadlock if we
+	 * don't release balloon_mutex here. Unlocking here is safe because the
 	 * callers drop the mutex before trying again.
 	 */
 	mutex_unlock(&balloon_mutex);

@@ -422,6 +422,18 @@ static void xen_online_page(struct page *page)
 	mutex_unlock(&balloon_mutex);

drivers/xen/balloon.c static void xen_online_page(struct page *page) · line 432

Quoted 10 lines
drivers/xen/balloon.c +8 -0
 }
 
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
+{
+	unsigned long i, size = (1 << order);
+	unsigned long start_pfn = page_to_pfn(pg);
+
+	pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, start_pfn);
+	for (i = 0; i < size; i++)
+		xen_online_page(pfn_to_page(start_pfn + i));

xen_online_page() isn't very complex and this is the only user.

Why don't you move its body in here and drop the extra function? And now you can execute the loop with balloon_mutex held instead of taking and releasing it in each iteration of the loop.

Juergen

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel