linux-kernel - Re: [PATCH v5 6/6] mm/page_isolation: remove migratetype parameter from more functions.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e8a6247b-c3cc-4f40-b41c-1a38e90ccaf0@suse.cz>
Date: Tue, 27 May 2025 14:55:54 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Zi Yan <ziy@...dia.com>, David Hildenbrand <david@...hat.com>,
 Johannes Weiner <hannes@...xchg.org>, linux-mm@...ck.org
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 Oscar Salvador <osalvador@...e.de>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>,
 "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
 Mel Gorman <mgorman@...hsingularity.net>,
 Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
 Brendan Jackman <jackmanb@...gle.com>, Richard Chang
 <richardycc@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/6] mm/page_isolation: remove migratetype parameter
 from more functions.

On 5/23/25 21:12, Zi Yan wrote:
> migratetype is no longer overwritten during pageblock isolation,
> start_isolate_page_range(), has_unmovable_pages(), and
> set_migratetype_isolate() no longer need which migratetype to restore
> during isolation failure.
> 
> For has_unmoable_pages(), it needs to know if the isolation is for CMA
> allocation, so adding CMA_ALLOCATION to provide the information. At the
> same time change isolation flags to enum pb_isolate_mode (MEMORY_OFFLINE,
> CMA_ALLOCATION, and ISOLATE_MODE_OTHERS). Remove REPORT_FAILURE and check
> MEMORY_OFFLINE instead, since only MEMORY_OFFLINE reports isolation
> failures.
> 
> alloc_contig_range() no longer needs migratetype. Replace it with
> a newly defined acr_flags_t to tell if an allocation is for CMA. So does
> __alloc_contig_migrate_range().
> 
> Signed-off-by: Zi Yan <ziy@...dia.com>
> ---
>  drivers/virtio/virtio_mem.c    |  3 +-
>  include/linux/gfp.h            |  6 ++-
>  include/linux/page-isolation.h | 19 ++++++++--
>  include/trace/events/kmem.h    | 14 ++++---
>  mm/cma.c                       |  2 +-
>  mm/memory_hotplug.c            |  4 +-
>  mm/page_alloc.c                | 25 ++++++-------
>  mm/page_isolation.c            | 67 +++++++++++++++-------------------
>  8 files changed, 72 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 56d0dbe62163..8accc0f255a8 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -1243,8 +1243,7 @@ static int virtio_mem_fake_offline(struct virtio_mem *vm, unsigned long pfn,
>  		if (atomic_read(&vm->config_changed))
>  			return -EAGAIN;
>  
> -		rc = alloc_contig_range(pfn, pfn + nr_pages, MIGRATE_MOVABLE,
> -					GFP_KERNEL);
> +		rc = alloc_contig_range(pfn, pfn + nr_pages, 0, GFP_KERNEL);
>  		if (rc == -ENOMEM)
>  			/* whoops, out of memory */
>  			return rc;
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index be160e8d8bcb..dea27ed24f8e 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -423,9 +423,13 @@ static inline bool gfp_compaction_allowed(gfp_t gfp_mask)
>  extern gfp_t vma_thp_gfp_mask(struct vm_area_struct *vma);
>  
>  #ifdef CONFIG_CONTIG_ALLOC
> +
> +typedef unsigned int __bitwise acr_flags_t;
> +#define ACR_CMA		((__force acr_flags_t)BIT(0))	// allocate for CMA

Would it make sense to define ACR_NONE as 0 so it's more descriptive than
seeing 0 somewhere?

> +
>  /* The below functions must be run on a range from a single zone. */
>  extern int alloc_contig_range_noprof(unsigned long start, unsigned long end,
> -			      unsigned migratetype, gfp_t gfp_mask);
> +			      acr_flags_t alloc_flags, gfp_t gfp_mask);
>  #define alloc_contig_range(...)			alloc_hooks(alloc_contig_range_noprof(__VA_ARGS__))
>  
>  extern struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index 7a681a49e73c..b61bca909ddc 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -38,8 +38,19 @@ static inline void set_pageblock_isolate(struct page *page)
>  }
>  #endif
>  
> -#define MEMORY_OFFLINE	0x1
> -#define REPORT_FAILURE	0x2
> +/*
> + * Pageblock isolation modes:
> + * MEMORY_OFFLINE      - isolate to offline (!allocate) memory e.g., skip over
> + *		         PageHWPoison() pages and PageOffline() pages.
> + *		         Unmovable pages will be reported in this mode.
> + * CMA_ALLOCATION      - isolate for CMA allocations
> + * ISOLATE_MODE_OTHERS - isolate for other purposes
> + */
> +enum pb_isolate_mode {
> +	MEMORY_OFFLINE,
> +	CMA_ALLOCATION,
> +	ISOLATE_MODE_OTHERS,

Since this is in a .h file, I'd prefer more consistent naming, and longer
names shouldn't hurt as there are not that many users.

Even something like:
PB_ISOLATE_MODE_OFFLINE
PB_ISOLATE_MODE_CMA
PB_ISOLATE_MODE_OTHER

?