mm/page_alloc.c: memory_hotplug: free pages as higher order

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

---
Changes since v6:
- Rebased to 4.20
- Changelog updated.
- No improvement seen on arm64, hence removed removal of prefetch.

Changes since v5:
- Rebased to 4.20-rc1.
- Changelog updated.

Changes since v4:
- As suggested by Michal Hocko,
- Simplify logic in online_pages_block() by using get_order().
- Seperate out removal of prefetch from __free_pages_core().

Changes since v3:
- Renamed _free_pages_boot_core -> __free_pages_core.
- Removed prefetch from __free_pages_core.
- Removed xen_online_page().

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.

v6: https://lore.kernel.org/patchwork/patch/1007253/
v5: https://lore.kernel.org/patchwork/patch/995739/
v4: https://lore.kernel.org/patchwork/patch/995111/
v3: https://lore.kernel.org/patchwork/patch/992348/
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          | 23 +++++++++++++++--------
 include/linux/memory_hotplug.h |  2 +-
 mm/internal.h                  |  1 +
 mm/memory_hotplug.c            | 42 ++++++++++++++++++++++++++++++------------
 mm/page_alloc.c                |  8 ++++----
 6 files changed, 55 insertions(+), 27 deletions(-)

drivers/hv/hv_balloon.c +4 -2

index 5301fef..211f3fe 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 -8

index ceb5048..95f888f 100644
@@ -345,8 +345,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);

@@ -369,15 +369,22 @@ static enum bp_state reserve_additional_memory(void)
 	return BP_ECANCELED;
 }
 
-static void xen_online_page(struct page *page)
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
 {
-	__online_page_set_limits(page);
+	unsigned long i, size = (1 << order);
+	unsigned long start_pfn = page_to_pfn(pg);
+	struct page *p;
 
+	pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, start_pfn);
 	mutex_lock(&balloon_mutex);
-
-	__balloon_append(page);
-
+	for (i = 0; i < size; i++) {
+		p = pfn_to_page(start_pfn + i);
+		__online_page_set_limits(p);
+		__balloon_append(p);
+	}
 	mutex_unlock(&balloon_mutex);
+
+	return 0;
 }
 
 static int xen_memory_notifier(struct notifier_block *nb, unsigned long val, void *v)

@@ -702,7 +709,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);
 #endif

include/linux/memory_hotplug.h +1 -1

index 07da5c6..d56bfba 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 f4a7bb0..536bc2a 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 memblock_free_pages(struct page *page, unsigned long pfn,
 					unsigned int order);
+extern void __free_pages_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 +29 -6

index b9a667d..0ea0eb1 100644
@@ -47,7 +47,7 @@
  * and restore_online_page_callback() for generic callback restore.
  */
 
-static void generic_online_page(struct page *page);
+static int generic_online_page(struct page *page, unsigned int order);
 
 static online_page_callback_t online_page_callback = generic_online_page;
 static DEFINE_MUTEX(online_page_callback_lock);

@@ -656,26 +656,44 @@ void __online_page_free(struct page *page)
 }
 EXPORT_SYMBOL_GPL(__online_page_free);
 
-static void generic_online_page(struct page *page)
+static int generic_online_page(struct page *page, unsigned int order)
 {
-	__online_page_set_limits(page);
-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages_add(1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages_add(1UL << order);
+#endif
+	return 0;
+}
+
+static int online_pages_blocks(unsigned long start, unsigned long nr_pages)
+{
+	unsigned long end = start + nr_pages;
+	int order, ret, onlined_pages = 0;
+
+	while (start < end) {
+		order = min(MAX_ORDER - 1,
+			get_order(PFN_PHYS(end) - PFN_PHYS(start)));
+
+		ret = (*online_page_callback)(pfn_to_page(start), order);
+		if (!ret)
+			onlined_pages += (1UL << order);
+		else if (ret > 0)
+			onlined_pages += ret;
+
+		start += (1UL << order);
+	}
+	return onlined_pages;
 }
 
 static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
 			void *arg)
 {
-	unsigned long i;

mm/memory_hotplug.c +0 -0

 	online_mem_sections(start_pfn, start_pfn + nr_pages);
 

mm/page_alloc.c +4 -4

index cde5dac..f51a920 100644
@@ -1303,7 +1303,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_core(struct page *page, unsigned int order)
 {
 	unsigned int nr_pages = 1 << order;
 	struct page *p = page;

@@ -1382,7 +1382,7 @@ void __init memblock_free_pages(struct page *page, unsigned long pfn,
 {
 	if (early_page_uninitialised(pfn))
 		return;
-	return __free_pages_boot_core(page, order);
+	__free_pages_core(page, order);
 }
 
 /*

@@ -1472,14 +1472,14 @@ static void __init deferred_free_range(unsigned long pfn,
 	if (nr_pages == pageblock_nr_pages &&
 	    (pfn & (pageblock_nr_pages - 1)) == 0) {
 		set_pageblock_migratetype(page, MIGRATE_MOVABLE);
-		__free_pages_boot_core(page, pageblock_order);
+		__free_pages_core(page, pageblock_order);
 		return;
 	}
 
 	for (i = 0; i < nr_pages; i++, page++, pfn++) {
 		if ((pfn & (pageblock_nr_pages - 1)) == 0)
 			set_pageblock_migratetype(page, MIGRATE_MOVABLE);
-		__free_pages_boot_core(page, 0);
+		__free_pages_core(page, 0);
 	}
 }
 

On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:

Quoted 7 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -2

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;

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

Quoted 7 lines

mm/memory_hotplug.c +1 -5

 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Shouldn't this be a "+=" instead of an "="? It seems like you are going to lose your count otherwise.

Quoted 1 line · Alexander Duyck

On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:

[...]

Quoted 7 lines

Quoted 7 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -2

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;

Quoted 10 lines

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

Quoted 7 lines

mm/memory_hotplug.c +1 -5

 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Shouldn't this be a "+=" instead of an "="? It seems like you are going to lose your count otherwise.

You are right of course. I should have noticed during the review. Thanks!

Quoted 22 lines · Michal Hocko

Quoted 1 line · Alexander Duyck

On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:

[...]

Quoted 18 lines

Quoted 4 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -1

 			void *arg)
 {
-	unsigned long i;

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

Quoted 10 lines

mm/memory_hotplug.c +1 -6

 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;
 
 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Shouldn't this be a "+=" instead of an "="? It seems like you are going
to lose your count otherwise.

Quoted 2 lines

You are right of course. I should have noticed during the review. Thanks!

I think we don't need to. The caller function is setting onlined_pages = 0 before calling online_pages_range().
And there are no other reference to online_pages_range other than from online_pages().

Quoted 26 lines · Arun KS

Quoted 24 lines · Michal Hocko

Quoted 1 line · Alexander Duyck

On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:

[...]

Quoted 7 lines

Quoted 7 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -2

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;

Quoted 11 lines

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

Quoted 7 lines

mm/memory_hotplug.c +1 -5

 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Shouldn't this be a "+=" instead of an "="? It seems like you are going
to lose your count otherwise.

You are right of course. I should have noticed during the review. Thanks!

Quoted 4 lines

I think we don't need to. The caller function is setting onlined_pages = 0 before calling online_pages_range().
And there are no other reference to online_pages_range other than from online_pages().

Are you missing that we accumulate onlined_pages via

	*(unsigned long *)arg = onlined_pages;

in online_pages_range?

Quoted 33 lines · Michal Hocko

Quoted 31 lines · Arun KS

Quoted 22 lines · Michal Hocko

Quoted 1 line · Alexander Duyck

On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:

[...]

Quoted 7 lines

Quoted 7 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -2

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;

Quoted 11 lines

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

Quoted 7 lines

mm/memory_hotplug.c +1 -5

 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Shouldn't this be a "+=" instead of an "="? It seems like you are going
to lose your count otherwise.

Quoted 2 lines

You are right of course. I should have noticed during the review. Thanks!

I think we don't need to. The caller function is setting onlined_pages = 0
before calling online_pages_range().
And there are no other reference to online_pages_range other than from online_pages().

Quoted 3 lines

Are you missing that we accumulate onlined_pages via

	*(unsigned long *)arg = onlined_pages;

in online_pages_range?

In my testing I didn't find any problem. To match the code being replaced and to avoid any corner cases, it is better to use +=
Will update the patch.

Quoted 38 lines · Arun KS

Quoted 36 lines · Michal Hocko

Quoted 23 lines · Arun KS

Quoted 22 lines · Michal Hocko

Quoted 1 line · Alexander Duyck

On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:

[...]

Quoted 18 lines

Quoted 4 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

(quoted diff) +0 -1

 			void *arg)
 {
-	unsigned long i;

Review

Quoted 10 lines

(quoted diff) +1 -6

 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;
 
 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Shouldn't this be a "+=" instead of an "="? It seems like you are going
to lose your count otherwise.

Quoted 8 lines

Quoted 2 lines

You are right of course. I should have noticed during the review. Thanks!

I think we don't need to. The caller function is setting onlined_pages = 0
before calling online_pages_range().
And there are no other reference to online_pages_range other than from online_pages().

Are you missing that we accumulate onlined_pages via

	*(unsigned long *)arg = onlined_pages;

in online_pages_range?

Quoted 3 lines

In my testing I didn't find any problem. To match the code being replaced and to avoid any corner cases, it is better to use +=
Will update the patch.

Have you checked that the number of present pages both in the zone and the node is correct because I fail to see how that would be possible.

It will be called once per online of a section and the arg value is always set to 0 while entering online_pages_range.

On Wed 09-01-19 16:12:48, Arun KS wrote:
[...]

Quoted 2 lines

It will be called once per online of a section and the arg value is always set to 0 while entering online_pages_range.

You rare right that this will be the case in the most simple scenario. But the point is that the callback can be called several times from walk_system_ram_range and then your current code wouldn't work properly.

Quoted 6 lines · Michal Hocko

On Wed 09-01-19 16:12:48, Arun KS wrote:
[...]

Quoted 3 lines

It will be called once per online of a section and the arg value is always
set to 0 while entering online_pages_range.

Quoted 4 lines

You rare right that this will be the case in the most simple scenario. But the point is that the callback can be called several times from walk_system_ram_range and then your current code wouldn't work properly.

Thanks. Will use +=

Quoted 7 lines · On Wed, 09 Jan 2019 16:36:36 +0530 Arun KS <[email protected]> wrote:

Quoted 6 lines · Michal Hocko

On Wed 09-01-19 16:12:48, Arun KS wrote:
[...]

Quoted 3 lines

It will be called once per online of a section and the arg value is always
set to 0 while entering online_pages_range.

Quoted 6 lines

Quoted 4 lines

You rare right that this will be the case in the most simple scenario. But the point is that the callback can be called several times from walk_system_ram_range and then your current code wouldn't work properly.

Thanks. Will use +=

The v8 patch
https://lore.kernel.org/lkml/[email protected]/T/#u

(which you apparently sent 7 minutes after typing the above) still has

static int online_pages_range(unsigned long start_pfn, unsigned long
nr_pages,

(quoted diff) +1 -7

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;
 
 	if (PageReserved(pfn_to_page(start_pfn)))
-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);

Even then the code makes no sense.

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
void *arg)

{
	unsigned long onlined_pages = *(unsigned long *)arg;

	if (PageReserved(pfn_to_page(start_pfn)))
		onlined_pages += online_pages_blocks(start_pfn, nr_pages);

	online_mem_sections(start_pfn, start_pfn + nr_pages);

	*(unsigned long *)arg += onlined_pages;
	return 0;
}

Either the final assignment should be

	*(unsigned long *)arg = onlined_pages;

or the initialization should be

	unsigned long onlined_pages = 0;

This is becoming a tad tiresome and I'd prefer not to have to check up on such things. Can we please get this right?

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

[...]

mm/memory_hotplug.c +9 -0

+static int online_pages_blocks(unsigned long start, unsigned long nr_pages)
+{
+	unsigned long end = start + nr_pages;
+	int order, ret, onlined_pages = 0;
+
+	while (start < end) {
+		order = min(MAX_ORDER - 1,
+			get_order(PFN_PHYS(end) - PFN_PHYS(start)));
+

mm/memory_hotplug.c +1 -5

-		for (i = 0; i < nr_pages; i++) {
-			page = pfn_to_page(start_pfn + i);
-			(*online_page_callback)(page);
-			onlined_pages++;
-		}
+		onlined_pages = online_pages_blocks(start_pfn, nr_pages);
 
 	online_mem_sections(start_pfn, start_pfn + nr_pages);

Quoted 9 lines · Arun KS

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

After running into my initial issue I actually had a few more questions about this patch.

Quoted 18 lines

[...]

mm/memory_hotplug.c +17 -0

+static int online_pages_blocks(unsigned long start, unsigned long nr_pages)
+{
+	unsigned long end = start + nr_pages;
+	int order, ret, onlined_pages = 0;
+
+	while (start < end) {
+		order = min(MAX_ORDER - 1,
+			get_order(PFN_PHYS(end) - PFN_PHYS(start)));
+
+		ret = (*online_page_callback)(pfn_to_page(start), order);
+		if (!ret)
+			onlined_pages += (1UL << order);
+		else if (ret > 0)
+			onlined_pages += ret;
+
+		start += (1UL << order);
+	}

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

Quoted 3 lines

mm/memory_hotplug.c +1 -0

+	return onlined_pages;
 }

Should the limit for this really be MAX_ORDER - 1 or should it be pageblock_order? In some cases this will be the same value, but I seem to recall that for x86 MAX_ORDER can be several times larger than pageblock_order.

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

Quoted 8 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -2

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;
 
 	if (PageReserved(pfn_to_page(start_pfn)))

I'm not sure we even really need this check. Getting back to the discussion I have been having with Michal in regards to the need for the DAX pages to not have the reserved bit cleared I was originally wondering if we could replace this check with a call to online_section_nr since the section shouldn't be online until we set the bit below in online_mem_sections.

However after doing some further digging it looks like this could probably be dropped entirely since we only call this function from online_pages and that function is only called by memory_block_action if pages_correctly_probed returns true. However pages_correctly_probed should return false if any of the sections contained in the page range is already online.

Quoted 32 lines · Alexander Duyck

Quoted 9 lines · Arun KS

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

After running into my initial issue I actually had a few more questions about this patch.

Quoted 18 lines

[...]

mm/memory_hotplug.c +17 -0

+static int online_pages_blocks(unsigned long start, unsigned long nr_pages)
+{
+	unsigned long end = start + nr_pages;
+	int order, ret, onlined_pages = 0;
+
+	while (start < end) {
+		order = min(MAX_ORDER - 1,
+			get_order(PFN_PHYS(end) - PFN_PHYS(start)));
+
+		ret = (*online_page_callback)(pfn_to_page(start), order);
+		if (!ret)
+			onlined_pages += (1UL << order);
+		else if (ret > 0)
+			onlined_pages += ret;
+
+		start += (1UL << order);
+	}

Quoted 8 lines

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

Quoted 3 lines

mm/memory_hotplug.c +1 -0

+	return onlined_pages;
 }

Should the limit for this really be MAX_ORDER - 1 or should it be pageblock_order? In some cases this will be the same value, but I seem to recall that for x86 MAX_ORDER can be several times larger than pageblock_order.

Does it make any difference when we are in fact trying to onine nr_pages and we clamp to it properly?

Quoted 16 lines

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

Quoted 8 lines

static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,

mm/memory_hotplug.c +0 -2

 			void *arg)
 {
-	unsigned long i;
 	unsigned long onlined_pages = *(unsigned long *)arg;
-	struct page *page;
 
 	if (PageReserved(pfn_to_page(start_pfn)))

I'm not sure we even really need this check. Getting back to the discussion I have been having with Michal in regards to the need for the DAX pages to not have the reserved bit cleared I was originally wondering if we could replace this check with a call to online_section_nr since the section shouldn't be online until we set the bit below in online_mem_sections.

Quoted 6 lines

However after doing some further digging it looks like this could probably be dropped entirely since we only call this function from online_pages and that function is only called by memory_block_action if pages_correctly_probed returns true. However pages_correctly_probed should return false if any of the sections contained in the page range is already online.

Yes you are right but I guess it would be better to address in a separate patch that deals with PageReserved manipulation in general. I do not think we want to remove the check silently. People who might be interested in backporting this for whatever reason might screatch their head why the test is not needed anymore.

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

drivers/hv/hv_balloon.c +1 -1

index 5301fef..211f3fe 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;
 

drivers/xen/balloon.c +2 -2

index ceb5048..95f888f 100644
@@ -345,8 +345,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);

drivers/xen/balloon.c +5 -0

+static int online_pages_blocks(unsigned long start, unsigned long nr_pages)
+{
+	unsigned long end = start + nr_pages;
+	int order, ret, onlined_pages = 0;
+

Quoted 9 lines · Arun KS

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

Sorry, ended up encountering a couple more things that have me a bit confused.

[...]

Quoted 27 lines

drivers/hv/hv_balloon.c +1 -1

index 5301fef..211f3fe 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;

drivers/hv/hv_balloon.c +3 -1

 	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;
 }
 

Quoted 2 lines

drivers/hv/hv_balloon.c +0 -0

 static int pfn_covered(unsigned long start_pfn, unsigned long 

pfn_cnt)

So the question I have is why was a return value added to these functions? They were previously void types and now they are int. What is the return value expected other than 0?

Quoted 23 lines

drivers/xen/balloon.c +2 -2

index ceb5048..95f888f 100644
@@ -345,8 +345,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);

@@ -369,15 +369,22 @@ static enum bp_state 
 reserve_additional_memory(void)
 	return BP_ECANCELED;
 }
 

drivers/xen/balloon.c static enum bp_state reserve_additional_memory(void) · line 372

Quoted 2 lines

drivers/xen/balloon.c +1 -1

-static void xen_online_page(struct page *page)
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)

Why did we rename this function? I see it was added as a new function in v3, however in v4 we ended up replacing it completely. So why not just keep the same name and make it easier for us to identify that the is the Xen version of the XXX_online_pages callback?

[...]

Quoted 15 lines

+static int online_pages_blocks(unsigned long start, unsigned long nr_pages)

drivers/xen/balloon.c +13 -0

+{
+	unsigned long end = start + nr_pages;
+	int order, ret, onlined_pages = 0;
+
+	while (start < end) {
+		order = min(MAX_ORDER - 1,
+			get_order(PFN_PHYS(end) - PFN_PHYS(start)));
+
+		ret = (*online_page_callback)(pfn_to_page(start), order);
+		if (!ret)
+			onlined_pages += (1UL << order);
+		else if (ret > 0)
+			onlined_pages += ret;

Quoted 1 line

+

So if the ret > 0 it is supposed to represent how many pages were onlined within a given block? What if the ret was negative? Really I am not a fan of adding a return value to the online functions unless we specifically document what the expected return values are supposed to be. If we don't have any return values other than 0 there isn't much point in having one anyway.

Quoted 46 lines · Alexander Duyck

Quoted 9 lines · Arun KS

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

Quoted 3 lines

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

Sorry, ended up encountering a couple more things that have me a bit confused.

[...]

Quoted 27 lines

drivers/hv/hv_balloon.c +1 -1

index 5301fef..211f3fe 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;

drivers/hv/hv_balloon.c +3 -1

 	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;
 }
 

Quoted 6 lines

Quoted 2 lines

drivers/hv/hv_balloon.c +0 -0

 static int pfn_covered(unsigned long start_pfn, unsigned long 

pfn_cnt)

So the question I have is why was a return value added to these functions? They were previously void types and now they are int. What is the return value expected other than 0?

Earlier with returning a void there was now way for an arch code to denying onlining of this particular page. By using an int as return type, we can implement this. In one of the boards I was using, there are some pages which should not be onlined because they are used for other purposes(like secure trust zone or hypervisor).

Quoted 54 lines · Arun KS

Quoted 46 lines · Alexander Duyck

Quoted 9 lines · Arun KS

When freeing pages are done with higher order, time spent 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 times. Modify external providers of online callback to align with the change.

Quoted 3 lines

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

Acked-by Michal Hocko <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

Sorry, ended up encountering a couple more things that have me a bit confused.

[...]

Quoted 27 lines

drivers/hv/hv_balloon.c +1 -1

index 5301fef..211f3fe 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;

drivers/hv/hv_balloon.c +3 -1

 	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;
 }
 

Quoted 6 lines

Quoted 2 lines

drivers/hv/hv_balloon.c +0 -0

 static int pfn_covered(unsigned long start_pfn, unsigned long

pfn_cnt)

So the question I have is why was a return value added to these functions? They were previously void types and now they are int. What is the return value expected other than 0?

Quoted 5 lines

Earlier with returning a void there was now way for an arch code to denying onlining of this particular page. By using an int as return type, we can implement this. In one of the boards I was using, there are some pages which should not be onlined because they are used for other purposes(like secure trust zone or hypervisor).

So where is the code using that? I don't see any functions in the kernel that are returning anything other than 0. Maybe you should hold off on changing the return type and make that a separate patch to be enabled when you add the new functions that can return non-zero values.

That way if someone wants to backport this they are just getting the bits needed to enable the improved hot-plug times without adding the extra overhead for changing the return type.