linux-kernel - Re: [PATCH v6 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: <30A5DAFA-9594-46F3-8054-ADBA83C551C5@nvidia.com>
Date: Mon, 02 Jun 2025 10:18:51 -0400
From: Zi Yan <ziy@...dia.com>
To: David Hildenbrand <david@...hat.com>
Cc: Johannes Weiner <hannes@...xchg.org>, Vlastimil Babka <vbabka@...e.cz>,
 linux-mm@...ck.org, 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 v6 6/6] mm/page_isolation: remove migratetype parameter
 from more functions.

On 30 May 2025, at 16:55, David Hildenbrand wrote:

> On 30.05.25 22:46, Zi Yan wrote:
>> On 30 May 2025, at 16:08, David Hildenbrand wrote:
>>
>>> On 30.05.25 21:58, Zi Yan wrote:
>>>> On 30 May 2025, at 15:56, David Hildenbrand wrote:
>>>>
>>>>> On 30.05.25 18:22, 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
>>>>>> (PB_ISOLATE_MODE_MEM_OFFLINE, PB_ISOLATE_MODE_CMA_ALLOC,
>>>>>> PB_ISOLATE_MODE_OTHER). Remove REPORT_FAILURE and check
>>>>>> MEMORY_OFFLINE instead, since only PB_ISOLATE_MODE_MEM_OFFLINE reports
>>>>>> isolation failures.
>>>>>>
>>>>>> alloc_contig_range() no longer needs migratetype. Replace it with
>>>>>> enum 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    |  2 +-
>>>>>>     include/linux/gfp.h            |  9 ++++-
>>>>>>     include/linux/page-isolation.h | 20 ++++++++--
>>>>>>     include/trace/events/kmem.h    | 14 ++++---
>>>>>>     mm/cma.c                       |  2 +-
>>>>>>     mm/memory_hotplug.c            |  6 +--
>>>>>>     mm/page_alloc.c                | 27 ++++++-------
>>>>>>     mm/page_isolation.c            | 70 +++++++++++++++-------------------
>>>>>>     8 files changed, 82 insertions(+), 68 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>>>>>> index 56d0dbe62163..6bce70b139b2 100644
>>>>>> --- a/drivers/virtio/virtio_mem.c
>>>>>> +++ b/drivers/virtio/virtio_mem.c
>>>>>> @@ -1243,7 +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,
>>>>>> +		rc = alloc_contig_range(pfn, pfn + nr_pages, ACR_OTHER,
>>>>>>     					GFP_KERNEL);
>>>>>>     		if (rc == -ENOMEM)
>>>>>>     			/* whoops, out of memory */
>>>>>> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
>>>>>> index be160e8d8bcb..51990d571e3e 100644
>>>>>> --- a/include/linux/gfp.h
>>>>>> +++ b/include/linux/gfp.h
>>>>>> @@ -423,9 +423,16 @@ 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
>>>>>> +
>>>>>> +enum acr_flags_t {
>>>>>> +	ACR_CMA,	// CMA allocation
>>>>>> +	ACR_OTHER,	// other allocation
>>>>>> +};
>>>>>
>>>>> Hm, enum != flags.
>>>>>
>>>>> If you want to use flags, then just have ACR_CMA. ACR_OTHER is implied if not set.
>>>>>
>>>>> And ACR_CMA would then have to be "1" etc.
>>>>
>>>> I have a fixup to change acr_flags_t to acr_mode.
>>>>
>>>>>
>>>>>> +
>>>>>>     /* 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);
>>>>>> +				     enum 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..3e2f960e166c 100644
>>>>>> --- a/include/linux/page-isolation.h
>>>>>> +++ b/include/linux/page-isolation.h
>>>>>> @@ -38,8 +38,20 @@ static inline void set_pageblock_isolate(struct page *page)
>>>>>>     }
>>>>>>     #endif
>>>>>>    -#define MEMORY_OFFLINE	0x1
>>>>>> -#define REPORT_FAILURE	0x2
>>>>>> +/*
>>>>>> + * Pageblock isolation modes:
>>>>>> + * PB_ISOLATE_MODE_MEM_OFFLINE - isolate to offline (!allocate) memory
>>>>>> + *				 e.g., skip over PageHWPoison() pages and
>>>>>> + *				 PageOffline() pages. Unmovable pages will be
>>>>>> + *				 reported in this mode.
>>>>>> + * PB_ISOLATE_MODE_CMA_ALLOC   - isolate for CMA allocations
>>>>>> + * PB_ISOLATE_MODE_OTHER       - isolate for other purposes
>>>>>> + */
>>>>>> +enum pb_isolate_mode {
>>>>>> +	PB_ISOLATE_MODE_MEM_OFFLINE,
>>>>>> +	PB_ISOLATE_MODE_CMA_ALLOC,
>>>>>> +	PB_ISOLATE_MODE_OTHER,
>>>>>> +};
>>>>>
>>>>> It's late on friady, but it looks like we are duplicating things here.
>>>>>
>>>>> Let me think about that once my brain is recharged :)
>>>>
>>>> Sure. Take your time.
>>>
>>> Could we abstract both settings and use a single one? Then, we could simply reject if MEM_OFFLINE is passed into alloc_contig_range().
>>>
>>> alloc_contig_pages and page isolation, hmmmm, MEM_OFFLINE is kind-of an allocation. CMA is an allocation.
>>>
>>> Just an idea, not sure ...
>>
>> I think so.
>>
>> This is the fixup of removing acr_flags_t. It is on top of the original
>> patch. Take your time. I guess I will send V7 with all fixups next week.
>
> Only wondering if we should rename "pb_isolate_mode" to something more abstract, that covers both use cases clearer.
>
> Hmmm.

It is specifying the purpose of either alloc_contig_range() or
test_pages_isolated(), but these two use cases do not have high level
connection AFAICT, except that pageblock isolation is involved
in the implementation. Let’s postpone the naming to the future.
As we discussed, we have two TODOs: 1) remove MIGRATE_ISOLATE,
and 2) simplify alloc_contig_range() to isolate the whole range in
one shot, then do migration. Probably we can think about naming at
that time. :)

>
>>
>> The one strange part is that in virtio_mem_fake_offline,
>> PB_ISOLATE_MODE_OTHER is used instead of PB_ISOLATE_MODE_MEM_OFFLINE.
>> I guess you do not want to report failures there.
>
> That's the right thing to do. It's not ordinary memory offlining code.

Got it.


Best Regards,
Yan, Zi