diff mbox series

[1/4] mm: pass set_count and set_reserved to __init_single_page

Message ID 20230922070923.355656-2-yajun.deng@linux.dev (mailing list archive)
State New
Headers show
Series mm: Don't set and reset page count in MEMINIT_EARLY | expand

Commit Message

Yajun Deng Sept. 22, 2023, 7:09 a.m. UTC 
When we init a single page, we need to mark this page reserved if it
does. And somes page may not need to set page count, such as compound
pages.

Pass set_count and set_reserved to __init_single_page, let the caller
decide if it needs to set page count or mark page reserved.

Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 mm/hugetlb.c  |  2 +-
 mm/internal.h |  3 ++-
 mm/mm_init.c  | 30 ++++++++++++++++--------------
 3 files changed, 19 insertions(+), 16 deletions(-)

Comments

Matthew Wilcox Sept. 22, 2023, 7:47 a.m. UTC | #1 
On Fri, Sep 22, 2023 at 03:09:20PM +0800, Yajun Deng wrote:
> -		__init_single_page(page, pfn, zone, nid);
> +		__init_single_page(page, pfn, zone, nid, true, false);

So Linus has just had a big rant about not doing bool flags to
functions.  And in particular _multiple_ bool flags to functions.

ie this should be:

#define INIT_PAGE_COUNT		(1 << 0)
#define INIT_PAGE_RESERVED	(1 << 1)

		__init_single_page(page, pfn, zone, nid, INIT_PAGE_COUNT);

or something similar.

I have no judgement on the merits of this patch so far.  Do you have
performance numbers for each of these patches?  Some of them seem quite
unlikely to actually help, at least on a machine which is constrained
by cacheline fetches.
David Hildenbrand Sept. 22, 2023, 7:48 a.m. UTC | #2 
On 22.09.23 09:47, Matthew Wilcox wrote:
> On Fri, Sep 22, 2023 at 03:09:20PM +0800, Yajun Deng wrote:
>> -		__init_single_page(page, pfn, zone, nid);
>> +		__init_single_page(page, pfn, zone, nid, true, false);
> 
> So Linus has just had a big rant about not doing bool flags to
> functions.  And in particular _multiple_ bool flags to functions.
> 
> ie this should be:
> 
> #define INIT_PAGE_COUNT		(1 << 0)
> #define INIT_PAGE_RESERVED	(1 << 1)
> 
> 		__init_single_page(page, pfn, zone, nid, INIT_PAGE_COUNT);
> 
> or something similar.
> 
> I have no judgement on the merits of this patch so far.  Do you have
> performance numbers for each of these patches?  Some of them seem quite
> unlikely to actually help, at least on a machine which is constrained
> by cacheline fetches.

The last patch contains

before:
node 0 deferred pages initialised in 78ms

after:
node 0 deferred pages initialised in 72ms

Not earth-shattering :D Maybe with much bigger machines relevant?
Mike Rapoport Sept. 22, 2023, 8:08 a.m. UTC | #3 
On Fri, Sep 22, 2023 at 09:48:59AM +0200, David Hildenbrand wrote:
> On 22.09.23 09:47, Matthew Wilcox wrote:
> > On Fri, Sep 22, 2023 at 03:09:20PM +0800, Yajun Deng wrote:
> > > -		__init_single_page(page, pfn, zone, nid);
> > > +		__init_single_page(page, pfn, zone, nid, true, false);
> > 
> > So Linus has just had a big rant about not doing bool flags to
> > functions.  And in particular _multiple_ bool flags to functions.
> > 
> > ie this should be:
> > 
> > #define INIT_PAGE_COUNT		(1 << 0)
> > #define INIT_PAGE_RESERVED	(1 << 1)
> > 
> > 		__init_single_page(page, pfn, zone, nid, INIT_PAGE_COUNT);
> > 
> > or something similar.
> > 
> > I have no judgement on the merits of this patch so far.  Do you have
> > performance numbers for each of these patches?  Some of them seem quite
> > unlikely to actually help, at least on a machine which is constrained
> > by cacheline fetches.
> 
> The last patch contains
> 
> before:
> node 0 deferred pages initialised in 78ms
> 
> after:
> node 0 deferred pages initialised in 72ms
> 
> Not earth-shattering :D Maybe with much bigger machines relevant?

Patch 3 contains

The following data was tested on an x86 machine with 190GB of RAM.

before:
free_low_memory_core_early()    342ms

after:
free_low_memory_core_early()    286ms

Which is more impressive, but still I'm not convinced that it's worth the
added complexity and potential subtle bugs.

> -- 
> Cheers,
> 
> David / dhildenb
>
diff mbox series

Patch

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index e2123d1bb4a2..4f91e47430ce 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3196,7 +3196,7 @@  static void __init hugetlb_folio_init_tail_vmemmap(struct folio *folio,
 	for (pfn = head_pfn + start_page_number; pfn < end_pfn; pfn++) {
 		struct page *page = pfn_to_page(pfn);
 
-		__init_single_page(page, pfn, zone, nid);
+		__init_single_page(page, pfn, zone, nid, true, false);
 		prep_compound_tail((struct page *)folio, pfn - head_pfn);
 		ret = page_ref_freeze(page, 1);
 		VM_BUG_ON(!ret);
diff --git a/mm/internal.h b/mm/internal.h
index 7a961d12b088..8bded7f98493 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1210,7 +1210,8 @@  struct vma_prepare {
 };
 
 void __meminit __init_single_page(struct page *page, unsigned long pfn,
-				unsigned long zone, int nid);
+				  unsigned long zone, int nid, bool set_count,
+				  bool set_reserved);
 
 /* shrinker related functions */
 unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg,
diff --git a/mm/mm_init.c b/mm/mm_init.c
index 06a72c223bce..c40042098a82 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -557,11 +557,13 @@  static void __init find_zone_movable_pfns_for_nodes(void)
 }
 
 void __meminit __init_single_page(struct page *page, unsigned long pfn,
-				unsigned long zone, int nid)
+				  unsigned long zone, int nid, bool set_count,
+				  bool set_reserved)
 {
 	mm_zero_struct_page(page);
 	set_page_links(page, zone, nid, pfn);
-	init_page_count(page);
+	if (set_count)
+		init_page_count(page);
 	page_mapcount_reset(page);
 	page_cpupid_reset_last(page);
 	page_kasan_tag_reset(page);
@@ -572,6 +574,8 @@  void __meminit __init_single_page(struct page *page, unsigned long pfn,
 	if (!is_highmem_idx(zone))
 		set_page_address(page, __va(pfn << PAGE_SHIFT));
 #endif
+	if (set_reserved)
+		__SetPageReserved(page);
 }
 
 #ifdef CONFIG_NUMA
@@ -714,7 +718,7 @@  static void __meminit init_reserved_page(unsigned long pfn, int nid)
 		if (zone_spans_pfn(zone, pfn))
 			break;
 	}
-	__init_single_page(pfn_to_page(pfn), pfn, zid, nid);
+	__init_single_page(pfn_to_page(pfn), pfn, zid, nid, true, false);
 }
 #else
 static inline void pgdat_set_deferred_range(pg_data_t *pgdat) {}
@@ -821,8 +825,8 @@  static void __init init_unavailable_range(unsigned long spfn,
 			pfn = pageblock_end_pfn(pfn) - 1;
 			continue;
 		}
-		__init_single_page(pfn_to_page(pfn), pfn, zone, node);
-		__SetPageReserved(pfn_to_page(pfn));
+		__init_single_page(pfn_to_page(pfn), pfn, zone, node,
+				   true, true);
 		pgcnt++;
 	}
 
@@ -884,7 +888,7 @@  void __meminit memmap_init_range(unsigned long size, int nid, unsigned long zone
 		}
 
 		page = pfn_to_page(pfn);
-		__init_single_page(page, pfn, zone, nid);
+		__init_single_page(page, pfn, zone, nid, true, false);
 		if (context == MEMINIT_HOTPLUG)
 			__SetPageReserved(page);
 
@@ -965,11 +969,9 @@  static void __init memmap_init(void)
 #ifdef CONFIG_ZONE_DEVICE
 static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
 					  unsigned long zone_idx, int nid,
-					  struct dev_pagemap *pgmap)
+					  struct dev_pagemap *pgmap,
+					  bool set_count)
 {
-
-	__init_single_page(page, pfn, zone_idx, nid);
-
 	/*
 	 * Mark page reserved as it will need to wait for onlining
 	 * phase for it to be fully associated with a zone.
@@ -977,7 +979,7 @@  static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
 	 * We can use the non-atomic __set_bit operation for setting
 	 * the flag as we are still initializing the pages.
 	 */
-	__SetPageReserved(page);
+	__init_single_page(page, pfn, zone_idx, nid, set_count, true);
 
 	/*
 	 * ZONE_DEVICE pages union ->lru with a ->pgmap back pointer
@@ -1041,7 +1043,7 @@  static void __ref memmap_init_compound(struct page *head,
 	for (pfn = head_pfn + 1; pfn < end_pfn; pfn++) {
 		struct page *page = pfn_to_page(pfn);
 
-		__init_zone_device_page(page, pfn, zone_idx, nid, pgmap);
+		__init_zone_device_page(page, pfn, zone_idx, nid, pgmap, false);
 		prep_compound_tail(head, pfn - head_pfn);
 		set_page_count(page, 0);
 
@@ -1084,7 +1086,7 @@  void __ref memmap_init_zone_device(struct zone *zone,
 	for (pfn = start_pfn; pfn < end_pfn; pfn += pfns_per_compound) {
 		struct page *page = pfn_to_page(pfn);
 
-		__init_zone_device_page(page, pfn, zone_idx, nid, pgmap);
+		__init_zone_device_page(page, pfn, zone_idx, nid, pgmap, true);
 
 		if (pfns_per_compound == 1)
 			continue;
@@ -2058,7 +2060,7 @@  static unsigned long  __init deferred_init_pages(struct zone *zone,
 		} else {
 			page++;
 		}
-		__init_single_page(page, pfn, zid, nid);
+		__init_single_page(page, pfn, zid, nid, true, false);
 		nr_pages++;
 	}
 	return (nr_pages);