memory_hotplug: Free pages as higher order

They not only increase the code footprint, they actually make things slower rather than faster. Remove them as contemporary hardware doesn't need any hint.

Suggested-by Dan Williams <[email protected]>

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

---
 mm/page_alloc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

mm/page_alloc.c +1 -5

index 7ab5274..90db431 100644
@@ -1258,14 +1258,10 @@ void __free_pages_core(struct page *page, unsigned int order)
 	struct page *p = page;
 	unsigned int loop;
 
-	prefetchw(p);
-	for (loop = 0; loop < (nr_pages - 1); loop++, p++) {
-		prefetchw(p + 1);
+	for (loop = 0; loop < nr_pages ; loop++, p++) {
 		__ClearPageReserved(p);
 		set_page_count(p, 0);
 	}
-	__ClearPageReserved(p);
-	set_page_count(p, 0);
 
 	page_zone(page)->managed_pages += nr_pages;
 	set_page_refcounted(page);

Signature · 1 line

1.9.1

They not only increase the code footprint, they actually make things slower rather than faster. Remove them as contemporary hardware doesn't need any hint.

Suggested-by Dan Williams <[email protected]>

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

---
 mm/page_alloc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

mm/page_alloc.c +1 -5

index 7ab5274..90db431 100644
@@ -1258,14 +1258,10 @@ void __free_pages_core(struct page *page, unsigned int order)
 	struct page *p = page;
 	unsigned int loop;
 
-	prefetchw(p);
-	for (loop = 0; loop < (nr_pages - 1); loop++, p++) {
-		prefetchw(p + 1);
+	for (loop = 0; loop < nr_pages ; loop++, p++) {
 		__ClearPageReserved(p);
 		set_page_count(p, 0);
 	}
-	__ClearPageReserved(p);
-	set_page_count(p, 0);
 
 	page_zone(page)->managed_pages += nr_pages;
 	set_page_refcounted(page);

Signature · 1 line

1.9.1

They not only increase the code footprint, they actually make things slower rather than faster. Remove them as contemporary hardware doesn't need any hint.

Suggested-by Dan Williams <[email protected]>

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

---
 mm/page_alloc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

mm/page_alloc.c +1 -5

index 7ab5274..90db431 100644
@@ -1258,14 +1258,10 @@ void __free_pages_core(struct page *page, unsigned int order)
 	struct page *p = page;
 	unsigned int loop;
 
-	prefetchw(p);
-	for (loop = 0; loop < (nr_pages - 1); loop++, p++) {
-		prefetchw(p + 1);
+	for (loop = 0; loop < nr_pages ; loop++, p++) {
 		__ClearPageReserved(p);
 		set_page_count(p, 0);
 	}
-	__ClearPageReserved(p);
-	set_page_count(p, 0);
 
 	page_zone(page)->managed_pages += nr_pages;
 	set_page_refcounted(page);

They not only increase the code footprint, they actually make things slower rather than faster. Remove them as contemporary hardware doesn't need any hint.

Suggested-by Dan Williams <[email protected]>

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

---
 mm/page_alloc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

mm/page_alloc.c +0 -2

index 7ab5274..90db431 100644
@@ -1258,14 +1258,10 @@ void __free_pages_core(struct page *page, unsigned int order)
 	struct page *p = page;
 	unsigned int loop;
 
-	prefetchw(p);
-	for (loop = 0; loop < (nr_pages - 1); loop++, p++) {

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]>

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]>

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.

Quoted 6 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.

Acked-by Michal Hocko <[email protected]>

Thanks for your patience with all the resubmission.

Quoted 8 lines · Michal Hocko

Quoted 6 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.

Quoted 3 lines

Acked-by Michal Hocko <[email protected]>

Thanks for your patience with all the resubmission.

Hello Michal,

I got the below email few days back. Do I still need to resubmit the patch? or is it already on the way to merge?

Quoted 8 lines · Michal Hocko

Quoted 6 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.

Quoted 3 lines

Acked-by Michal Hocko <[email protected]>

Thanks for your patience with all the resubmission.

Hello Michal,

I got the below email few days back. Do I still need to resubmit the patch? or is it already on the way to merge?

Quoted 6 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.

Acked-by Michal Hocko <[email protected]>

Thanks for your patience with all the resubmission.

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.

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.

Quoted 6 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.

Hi Arun, out of curiosity:

could you please explain how exactly did you mesure the speed improvement?

Thanks

mm/memory_hotplug.c +0 -0

index e379e85..2416136 100644
@@ -690,9 +690,13 @@ static int online_pages_range(unsigned long start_pfn,
 unsigned long nr_pages,
                        void *arg)
 {

mm/memory_hotplug.c +0 -0

index e379e85..2416136 100644
@@ -690,9 +690,13 @@ static int online_pages_range(unsigned long start_pfn,
 unsigned long nr_pages,
                        void *arg)
 {

Quoted 6 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.

Hi Arun, out of curiosity:

could you please explain how exactly did you mesure the speed improvement?

Thanks

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.

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]>

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock.

Quoted 40 lines · Arun KS

Quoted 38 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 1 line

Looks unsafe without managed_page_count_lock.

Why does it matter actually? We cannot online/offline memory in parallel. This is not the case for the boot where we initialize memory in parallel on multiple nodes. So this seems to be safe currently unless I am missing something. A comment explaining that would be helpful though.

Quoted 41 lines · Michal Hocko

Quoted 40 lines · Arun KS

Quoted 38 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 7 lines

Quoted 1 line

Looks unsafe without managed_page_count_lock.

Why does it matter actually? We cannot online/offline memory in parallel. This is not the case for the boot where we initialize memory in parallel on multiple nodes. So this seems to be safe currently unless I am missing something. A comment explaining that would be helpful though.

Other main callers of adjust_manage_page_count(),

static inline void free_reserved_page(struct page *page)

{
        __free_reserved_page(page);
        adjust_managed_page_count(page, 1);
}

static inline void mark_page_reserved(struct page *page)

{
        SetPageReserved(page);
        adjust_managed_page_count(page, -1);
}

Won't they race with memory hotplug?

Few more,

./drivers/xen/balloon.c:519:            adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:175:  adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:196:  adjust_managed_page_count(page, 1);
./mm/hugetlb.c:2158:                    adjust_managed_page_count(page, 1 <<

h->order);

Quoted 42 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 7 lines

Quoted 1 line

Looks unsafe without managed_page_count_lock.

Why does it matter actually? We cannot online/offline memory in parallel. This is not the case for the boot where we initialize memory in parallel on multiple nodes. So this seems to be safe currently unless I am missing something. A comment explaining that would be helpful though.

Other main callers of adjust_manage_page_count(),

static inline void free_reserved_page(struct page *page)

{
        __free_reserved_page(page);
        adjust_managed_page_count(page, 1);
}

static inline void mark_page_reserved(struct page *page)

{
        SetPageReserved(page);
        adjust_managed_page_count(page, -1);
}

Won't they race with memory hotplug?

Quoted 6 lines

Few more,

./drivers/xen/balloon.c:519:            adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:175:  adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:196:  adjust_managed_page_count(page, 1);
./mm/hugetlb.c:2158:                    adjust_managed_page_count(page, 1 <<

h->order);

They can, and I have missed those.

So this patch needs more work, yes?

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more.

Quoted 6 lines · Michal Hocko

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more.

Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one.

Quoted 9 lines · Arun KS

Quoted 7 lines · Michal Hocko

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he
will play with this some more.

Quoted 3 lines

Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one.

Hello Andrew/Michal,

Will this be going in subsequent -rcs?

Quoted 9 lines · Arun KS

Quoted 7 lines · Michal Hocko

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he
will play with this some more.

Quoted 3 lines

Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one.

Hello Andrew/Michal,

Will this be going in subsequent -rcs?

Quoted 13 lines · On Mon, 05 Nov 2018 15:12:27 +0530 Arun KS <[email protected]> wrote:

Quoted 12 lines · Arun KS

Quoted 7 lines · Michal Hocko

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he
will play with this some more.

Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one.

Quoted 3 lines

Hello Andrew/Michal,

Will this be going in subsequent -rcs?

I thought were awaiting a new version? "Will send out a separate patch on top of this one"?

I do think a resend would be useful, please. Ensure the changelog is updated to capture the above info and any other worthy issues which arose during review.

Quoted 13 lines · On Mon, 05 Nov 2018 15:12:27 +0530 Arun KS <[email protected]> wrote:

Quoted 12 lines · Arun KS

Quoted 7 lines · Michal Hocko

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he
will play with this some more.

Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one.

Quoted 3 lines

Hello Andrew/Michal,

Will this be going in subsequent -rcs?

I thought were awaiting a new version? "Will send out a separate patch on top of this one"?

I do think a resend would be useful, please. Ensure the changelog is updated to capture the above info and any other worthy issues which arose during review.

Quoted 6 lines · Michal Hocko

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more.

Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one.

On Thu 18-10-18 19:18:25, Andrew Morton wrote:
[...]

Quoted 1 line

So this patch needs more work, yes?

Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more.

So this patch needs more work, yes?

Quoted 42 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 7 lines

Quoted 1 line

Looks unsafe without managed_page_count_lock.

Why does it matter actually? We cannot online/offline memory in parallel. This is not the case for the boot where we initialize memory in parallel on multiple nodes. So this seems to be safe currently unless I am missing something. A comment explaining that would be helpful though.

Other main callers of adjust_manage_page_count(),

static inline void free_reserved_page(struct page *page)

{
        __free_reserved_page(page);
        adjust_managed_page_count(page, 1);
}

static inline void mark_page_reserved(struct page *page)

{
        SetPageReserved(page);
        adjust_managed_page_count(page, -1);
}

Won't they race with memory hotplug?

Quoted 6 lines

Few more,

./drivers/xen/balloon.c:519:            adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:175:  adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:196:  adjust_managed_page_count(page, 1);
./mm/hugetlb.c:2158:                    adjust_managed_page_count(page, 1 <<

h->order);

They can, and I have missed those.

Quoted 41 lines · Michal Hocko

Quoted 40 lines · Arun KS

Quoted 38 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 7 lines

Quoted 1 line

Looks unsafe without managed_page_count_lock.

Why does it matter actually? We cannot online/offline memory in parallel. This is not the case for the boot where we initialize memory in parallel on multiple nodes. So this seems to be safe currently unless I am missing something. A comment explaining that would be helpful though.

Other main callers of adjust_manage_page_count(),

static inline void free_reserved_page(struct page *page)

{
        __free_reserved_page(page);
        adjust_managed_page_count(page, 1);
}

static inline void mark_page_reserved(struct page *page)

{
        SetPageReserved(page);
        adjust_managed_page_count(page, -1);
}

Won't they race with memory hotplug?

Few more,

./drivers/xen/balloon.c:519:            adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:175:  adjust_managed_page_count(page, -1);
./drivers/virtio/virtio_balloon.c:196:  adjust_managed_page_count(page, 1);
./mm/hugetlb.c:2158:                    adjust_managed_page_count(page, 1 <<

h->order);

Quoted 40 lines · Arun KS

Quoted 38 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Quoted 1 line

Looks unsafe without managed_page_count_lock.

Why does it matter actually? We cannot online/offline memory in parallel. This is not the case for the boot where we initialize memory in parallel on multiple nodes. So this seems to be safe currently unless I am missing something. A comment explaining that would be helpful though.

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock.

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock. I think better have a similar implementation of free_boot_core() in memory_hotplug.c like we had in version 1 of patch. And use adjust_managed_page_count() instead of page_zone(page)->managed_pages += nr_pages;

https://lore.kernel.org/patchwork/patch/989445/

(quoted diff) +15 -4

-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);
+	unsigned long nr_pages = 1 << order;
+	struct page *p = page;
+
+	for (loop = 0 ; loop < nr_pages ; loop++, p++) {
+		__ClearPageReserved(p);
+		set_page_count(p, 0);
+	}
+
+	adjust_managed_page_count(page, nr_pages);
+	set_page_refcounted(page);
+	__free_pages(page, order);
+
+	return 0;
+}

Regards,
Arun

Quoted 45 lines · Arun KS

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock. I think better have a similar implementation of free_boot_core() in memory_hotplug.c like we had in version 1 of patch. And use adjust_managed_page_count() instead of page_zone(page)->managed_pages += nr_pages;

Quoted 1 line

https://lore.kernel.org/patchwork/patch/989445/

Looks like deferred_free_range() has the same problem calling __free_pages_core() to adjust zone->managed_pages.

__free_pages_bootmem() is OK because at that point the system is still single-threaded?
Could be solved by moving that out of __free_pages_core().

But do we care about readers potentially seeing a store tear? If yes then maybe these counters should be converted to atomics...

Quoted 45 lines · Arun KS

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock. I think better have a similar implementation of free_boot_core() in memory_hotplug.c like we had in version 1 of patch. And use adjust_managed_page_count() instead of page_zone(page)->managed_pages += nr_pages;

Quoted 1 line

https://lore.kernel.org/patchwork/patch/989445/

Looks like deferred_free_range() has the same problem calling __free_pages_core() to adjust zone->managed_pages.

__free_pages_bootmem() is OK because at that point the system is still single-threaded?
Could be solved by moving that out of __free_pages_core().

But do we care about readers potentially seeing a store tear? If yes then maybe these counters should be converted to atomics...

Quoted 45 lines · Arun KS

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-   __online_page_increment_counters(page);
-   __online_page_free(page);
+   __free_pages_core(page, order);
+   totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+   if (PageHighMem(page))
+           totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock. I think better have a similar implementation of free_boot_core() in memory_hotplug.c like we had in version 1 of patch. And use adjust_managed_page_count() instead of page_zone(page)->managed_pages += nr_pages;

Quoted 1 line

https://lore.kernel.org/patchwork/patch/989445/

Looks like deferred_free_range() has the same problem calling __free_pages_core() to adjust zone->managed_pages. I expect __free_pages_bootmem() is OK because at that point the system is still single-threaded?
Could be solved by moving that out of __free_pages_core().

But do we care about readers potentially seeing a store tear? If yes then maybe these counters should be converted to atomics...

Quoted 25 lines

(quoted diff) +15 -4

-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);
+     unsigned long nr_pages = 1 << order;
+     struct page *p = page;
+
+     for (loop = 0 ; loop < nr_pages ; loop++, p++) {
+             __ClearPageReserved(p);
+             set_page_count(p, 0);
+     }
+
+     adjust_managed_page_count(page, nr_pages);
+     set_page_refcounted(page);
+     __free_pages(page, order);
+
+     return 0;
+}

Regards,
Arun

Quoted 45 lines · Arun KS

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-   __online_page_increment_counters(page);
-   __online_page_free(page);
+   __free_pages_core(page, order);
+   totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+   if (PageHighMem(page))
+           totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock. I think better have a similar implementation of free_boot_core() in memory_hotplug.c like we had in version 1 of patch. And use adjust_managed_page_count() instead of page_zone(page)->managed_pages += nr_pages;

Quoted 1 line

https://lore.kernel.org/patchwork/patch/989445/

Looks like deferred_free_range() has the same problem calling __free_pages_core() to adjust zone->managed_pages. I expect __free_pages_bootmem() is OK because at that point the system is still single-threaded?
Could be solved by moving that out of __free_pages_core().

But do we care about readers potentially seeing a store tear? If yes then maybe these counters should be converted to atomics...

Quoted 22 lines

(quoted diff) +15 -4

-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);
+     unsigned long nr_pages = 1 << order;
+     struct page *p = page;
+
+     for (loop = 0 ; loop < nr_pages ; loop++, p++) {
+             __ClearPageReserved(p);
+             set_page_count(p, 0);
+     }
+
+     adjust_managed_page_count(page, nr_pages);
+     set_page_refcounted(page);
+     __free_pages(page, order);
+
+     return 0;
+}

Quoted 3 lines

Regards,
Arun

Quoted 25 lines · Vlastimil Babka

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

Quoted 13 lines

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

Looks unsafe without managed_page_count_lock. I think better have a similar implementation of free_boot_core() in memory_hotplug.c like we had in version 1 of patch. And use adjust_managed_page_count() instead of page_zone(page)->managed_pages += nr_pages;

https://lore.kernel.org/patchwork/patch/989445/

(quoted diff) +15 -4

-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);
+	unsigned long nr_pages = 1 << order;
+	struct page *p = page;
+
+	for (loop = 0 ; loop < nr_pages ; loop++, p++) {
+		__ClearPageReserved(p);
+		set_page_count(p, 0);
+	}
+
+	adjust_managed_page_count(page, nr_pages);
+	set_page_refcounted(page);
+	__free_pages(page, order);
+
+	return 0;
+}

Regards,
Arun

Quoted 8 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]>

[...]

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

Quoted 8 lines

mm/memory_hotplug.c +1 -2

@@ -655,26 +655,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);

This is now not called anymore, although the xen/hv variants still do it. The function seems empty these days, maybe remove it as a followup cleanup?

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

Quoted 8 lines

mm/memory_hotplug.c +6 -2

-	__online_page_increment_counters(page);
-	__online_page_free(page);
+	__free_pages_core(page, order);
+	totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+	if (PageHighMem(page))
+		totalhigh_pages += (1UL << order);
+#endif

__online_page_increment_counters() would have used adjust_managed_page_count() which would do the changes under managed_page_count_lock. Are we safe without the lock? If yes, there should perhaps be a comment explaining why.

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]>