Re: [PATCH 7/8] mm: Introduce promo_wmark_pages()

Re: [PATCH 7/8] mm: Introduce promo_wmark_pages()

[Johannes Weiner]

Hello Kirill,

On Mon, Aug 05, 2024 at 05:59:39PM +0300, Kirill A. Shutemov wrote:
> Add promo_wmark_pages() helper to complement other zone watermark
> accessors.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Andrew picked up a change that does this a few days ago:

https://lore.kernel.org/all/20240801232548.36604-2-kaiyang2@cs.cmu.edu/T/#u

Re: [PATCH 7/8] mm: Introduce promo_wmark_pages()

[Kirill A. Shutemov]

On Mon, Aug 05, 2024 at 01:04:42PM -0400, Johannes Weiner wrote:
> Hello Kirill,
> 
> On Mon, Aug 05, 2024 at 05:59:39PM +0300, Kirill A. Shutemov wrote:
> > Add promo_wmark_pages() helper to complement other zone watermark
> > accessors.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 
> Andrew picked up a change that does this a few days ago:
> 
> https://lore.kernel.org/all/20240801232548.36604-2-kaiyang2@cs.cmu.edu/T/#u

Ah. Great. I'll drop the patch.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 1/8] mm: Fix endless reclaim on machines with unaccepted memory

[David Hildenbrand]

On 05.08.24 16:59, Kirill A. Shutemov wrote:
> Unaccepted memory is considered unusable free memory, which is not
> counted as free on the zone watermark check. This causes
> get_page_from_freelist() to accept more memory to hit the high
> watermark, but it creates problems in the reclaim path.
> 
> The reclaim path encounters a failed zone watermark check and attempts
> to reclaim memory. This is usually successful, but if there is little or
> no reclaimable memory, it can result in endless reclaim with little to
> no progress. This can occur early in the boot process, just after start
> of the init process when the only reclaimable memory is the page cache
> of the init executable and its libraries.
> 
> Make unaccepted memory free from watermark check point of view. This way
> unaccepted memory will never be the trigger of memory reclaim.
> Accept more memory in the get_page_from_freelist() if needed.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Jianxiong Gao <jxgao@google.com>
> Fixes: dcdfdd40fa82 ("mm: Add support for unaccepted memory")
> Cc: stable@vger.kernel.org # v6.5+
> ---

Nothing jumped at me, sounds reasonable

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/8] mm: Accept memory in __alloc_pages_bulk().

[Mike Rapoport]

On Mon, Aug 05, 2024 at 05:59:34PM +0300, Kirill A. Shutemov wrote:
> Currently, the kernel only accepts memory in get_page_from_freelist(),
> but there is another path that directly takes pages from free lists -
> __alloc_page_bulk(). This function can consume all accepted memory and
> will resort to __alloc_pages_noprof() if necessary.
> 
> Conditionally accepted in __alloc_pages_bulk().
> 
> The same issue may arise due to deferred page initialization. Kick the
> deferred initialization machinery before abandoning the zone, as the
> kernel does in get_page_from_freelist().
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

> ---
>  mm/page_alloc.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index aa9b1eaa638c..90a1f01d5996 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4576,12 +4576,25 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  			goto failed;
>  		}
>  
> +		cond_accept_memory(zone, 0);
> +retry_this_zone:
>  		mark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK) + nr_pages;
>  		if (zone_watermark_fast(zone, 0,  mark,
>  				zonelist_zone_idx(ac.preferred_zoneref),
>  				alloc_flags, gfp)) {
>  			break;
>  		}
> +
> +		if (cond_accept_memory(zone, 0))
> +			goto retry_this_zone;
> +
> +#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> +		/* Try again if zone has deferred pages */
> +		if (deferred_pages_enabled()) {
> +			if (_deferred_grow_zone(zone, 0))
> +				goto retry_this_zone;
> +		}
> +#endif
>  	}
>  
>  	/*
> -- 
> 2.43.0
> 

Re: [PATCH 2/8] mm: Accept memory in __alloc_pages_bulk().

[David Hildenbrand]

On 05.08.24 16:59, Kirill A. Shutemov wrote:
> Currently, the kernel only accepts memory in get_page_from_freelist(),
> but there is another path that directly takes pages from free lists -
> __alloc_page_bulk(). This function can consume all accepted memory and
> will resort to __alloc_pages_noprof() if necessary.
> 
> Conditionally accepted in __alloc_pages_bulk().
> 
> The same issue may arise due to deferred page initialization. Kick the
> deferred initialization machinery before abandoning the zone, as the
> kernel does in get_page_from_freelist().
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>   mm/page_alloc.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index aa9b1eaa638c..90a1f01d5996 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4576,12 +4576,25 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>   			goto failed;
>   		}
>   
> +		cond_accept_memory(zone, 0);
> +retry_this_zone:
>   		mark = wmark_pages(zone, alloc_flags & ALLOC_WMARK_MASK) + nr_pages;
>   		if (zone_watermark_fast(zone, 0,  mark,
>   				zonelist_zone_idx(ac.preferred_zoneref),
>   				alloc_flags, gfp)) {
>   			break;
>   		}
> +
> +		if (cond_accept_memory(zone, 0))
> +			goto retry_this_zone;
> +
> +#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> +		/* Try again if zone has deferred pages */
> +		if (deferred_pages_enabled()) {
> +			if (_deferred_grow_zone(zone, 0))
> +				goto retry_this_zone;
> +		}
> +#endif

We could probably avoid the #ifdef if we add a dummy function for 
_deferred_grow_zone().

Same applies to the other similar users in this file.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 3/8] mm: Introduce PageUnaccepted() page type

[David Hildenbrand]

On 05.08.24 16:59, Kirill A. Shutemov wrote:
> The new page type allows physical memory scanners to detect unaccepted
> memory and handle it accordingly.
> 
> The page type is serialized with zone lock.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>   include/linux/page-flags.h | 3 +++
>   mm/page_alloc.c            | 2 ++
>   2 files changed, 5 insertions(+)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 5769fe6e4950..e19eac9c2b5c 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -943,6 +943,7 @@ enum pagetype {
>   	PG_hugetlb	= 0x04000000,
>   	PG_slab		= 0x02000000,
>   	PG_zsmalloc	= 0x01000000,
> +	PG_unaccepted	= 0x00800000,
>   
>   	PAGE_TYPE_BASE	= 0x80000000,
>   
> @@ -1076,6 +1077,8 @@ FOLIO_TEST_FLAG_FALSE(hugetlb)
>   
>   PAGE_TYPE_OPS(Zsmalloc, zsmalloc, zsmalloc)
>   
> +PAGE_TYPE_OPS(Unaccepted, unaccepted, unaccepted)

I'm sure you're able to come up with some documentation ;)

> +
>   /**
>    * PageHuge - Determine if the page belongs to hugetlbfs
>    * @page: The page to test.
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 90a1f01d5996..a35efb114496 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6972,6 +6972,7 @@ static bool try_to_accept_memory_one(struct zone *zone)
>   
>   	account_freepages(zone, -MAX_ORDER_NR_PAGES, MIGRATE_MOVABLE);
>   	__mod_zone_page_state(zone, NR_UNACCEPTED, -MAX_ORDER_NR_PAGES);
> +	__ClearPageUnaccepted(page);
>   	spin_unlock_irqrestore(&zone->lock, flags);
>   
>   	accept_page(page, MAX_PAGE_ORDER);
> @@ -7030,6 +7031,7 @@ static bool __free_unaccepted(struct page *page)
>   	list_add_tail(&page->lru, &zone->unaccepted_pages);
>   	account_freepages(zone, MAX_ORDER_NR_PAGES, MIGRATE_MOVABLE);
>   	__mod_zone_page_state(zone, NR_UNACCEPTED, MAX_ORDER_NR_PAGES);
> +	__SetPageUnaccepted(page);
>   	spin_unlock_irqrestore(&zone->lock, flags);
>   
>   	if (first)

At the point PG_unaccepted is set/cleared, we don't have another type 
set, right? (IOW, PG_buddy is only set after we cleared PG_unaccepted)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 3/8] mm: Introduce PageUnaccepted() page type

[Kirill A. Shutemov]

On Tue, Aug 06, 2024 at 02:06:02PM +0200, David Hildenbrand wrote:
> On 05.08.24 16:59, Kirill A. Shutemov wrote:
> > The new page type allows physical memory scanners to detect unaccepted
> > memory and handle it accordingly.
> > 
> > The page type is serialized with zone lock.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >   include/linux/page-flags.h | 3 +++
> >   mm/page_alloc.c            | 2 ++
> >   2 files changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 5769fe6e4950..e19eac9c2b5c 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -943,6 +943,7 @@ enum pagetype {
> >   	PG_hugetlb	= 0x04000000,
> >   	PG_slab		= 0x02000000,
> >   	PG_zsmalloc	= 0x01000000,
> > +	PG_unaccepted	= 0x00800000,
> >   	PAGE_TYPE_BASE	= 0x80000000,
> > @@ -1076,6 +1077,8 @@ FOLIO_TEST_FLAG_FALSE(hugetlb)
> >   PAGE_TYPE_OPS(Zsmalloc, zsmalloc, zsmalloc)
> > +PAGE_TYPE_OPS(Unaccepted, unaccepted, unaccepted)
> 
> I'm sure you're able to come up with some documentation ;)

Will do.

> > +
> >   /**
> >    * PageHuge - Determine if the page belongs to hugetlbfs
> >    * @page: The page to test.
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 90a1f01d5996..a35efb114496 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -6972,6 +6972,7 @@ static bool try_to_accept_memory_one(struct zone *zone)
> >   	account_freepages(zone, -MAX_ORDER_NR_PAGES, MIGRATE_MOVABLE);
> >   	__mod_zone_page_state(zone, NR_UNACCEPTED, -MAX_ORDER_NR_PAGES);
> > +	__ClearPageUnaccepted(page);
> >   	spin_unlock_irqrestore(&zone->lock, flags);
> >   	accept_page(page, MAX_PAGE_ORDER);
> > @@ -7030,6 +7031,7 @@ static bool __free_unaccepted(struct page *page)
> >   	list_add_tail(&page->lru, &zone->unaccepted_pages);
> >   	account_freepages(zone, MAX_ORDER_NR_PAGES, MIGRATE_MOVABLE);
> >   	__mod_zone_page_state(zone, NR_UNACCEPTED, MAX_ORDER_NR_PAGES);
> > +	__SetPageUnaccepted(page);
> >   	spin_unlock_irqrestore(&zone->lock, flags);
> >   	if (first)
> 
> At the point PG_unaccepted is set/cleared, we don't have another type set,
> right? (IOW, PG_buddy is only set after we cleared PG_unaccepted)

Right. PG_buddy is set after we clear PG_unaccepted.

There's brief period when the page is under accept when PG_unaccepted
already cleared, but PG_buddy is not yet set. But I don't think it can be
problematic.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 4/8] mm: Rename accept_page() to accept_page_memory()

[David Hildenbrand]

On 05.08.24 16:59, Kirill A. Shutemov wrote:
> Rename the helper. The accept_page() name is going to be used for
> different function.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>   mm/page_alloc.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a35efb114496..34718852d576 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -286,7 +286,7 @@ EXPORT_SYMBOL(nr_online_nodes);
>   #endif
>   
>   static bool page_contains_unaccepted(struct page *page, unsigned int order);
> -static void accept_page(struct page *page, unsigned int order);
> +static void accept_page_memory(struct page *page, unsigned int order);
>   static bool cond_accept_memory(struct zone *zone, unsigned int order);
>   static inline bool has_unaccepted_memory(void);
>   static bool __free_unaccepted(struct page *page);
> @@ -1263,7 +1263,7 @@ void __meminit __free_pages_core(struct page *page, unsigned int order,
>   		if (order == MAX_PAGE_ORDER && __free_unaccepted(page))
>   			return;
>   
> -		accept_page(page, order);
> +		accept_page_memory(page, order);

I wonder if we can do better at naming.

Naming them accept_page1 and accept_page2 might be just as confusing as what you use here.

And I think we better just don't have this "page" wrapper here at all and just move to
memory ranges.

After all, the page contains no information we need here besides the PFN.


What about:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 7a8bdfa742e1..5dc4066f35b3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -286,7 +286,6 @@ EXPORT_SYMBOL(nr_online_nodes);
  #endif
  
  static bool page_contains_unaccepted(struct page *page, unsigned int order);
-static void accept_page(struct page *page, unsigned int order);
  static inline bool has_unaccepted_memory(void);
  static bool __free_unaccepted(struct page *page);
  
@@ -1262,7 +1261,7 @@ void __meminit __free_pages_core(struct page *page, unsigned int order,
                 if (order == MAX_PAGE_ORDER && __free_unaccepted(page))
                         return;
  
-               accept_page(page, order);
+               __accept_memory(page_to_phys(page), PAGE_SIZE << order);
         }
  
         /*
@@ -6975,11 +6974,9 @@ static bool page_contains_unaccepted(struct page *page, unsigned int order)
         return range_contains_unaccepted_memory(start, end);
  }
  
-static void accept_page(struct page *page, unsigned int order)
+static void __accept_memory(phys_addr_t start, phys_addr_t size)
  {
-       phys_addr_t start = page_to_phys(page);
-
-       accept_memory(start, start + (PAGE_SIZE << order));
+       accept_memory(start, start + size);
  }
  
  static bool try_to_accept_memory_one(struct zone *zone)
@@ -7006,7 +7003,7 @@ static bool try_to_accept_memory_one(struct zone *zone)
         __mod_zone_page_state(zone, NR_UNACCEPTED, -MAX_ORDER_NR_PAGES);
         spin_unlock_irqrestore(&zone->lock, flags);
  
-       accept_page(page, MAX_PAGE_ORDER);
+       __accept_memory(page_to_phys(page), PAGE_SIZE << MAX_PAGE_ORDER);
  
         __free_pages_ok(page, MAX_PAGE_ORDER, FPI_TO_TAIL);
  
@@ -7071,7 +7068,7 @@ static bool page_contains_unaccepted(struct page *page, unsigned int order)
         return false;
  }
  
-static void accept_page(struct page *page, unsigned int order)
+static void __accept_memory(phys_addr_t start, phys_addr_t size)
  {
  }
  

It would be even easier if accept_memory() would just accept start+size.

-- 
Cheers,

David / dhildenb

Re: [PATCH 5/8] mm: Add a helper to accept page

[David Hildenbrand]

On 05.08.24 16:59, Kirill A. Shutemov wrote:
> Accept a given struct page and add it free list.
> 
> The help is useful for physical memory scanners that want to use free
> unaccepted memory.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb