| 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
| ||
|
Message-ID: <FB393186-FE4D-4A67-9701-7BC4946AC222@nvidia.com>
Date: Tue, 27 May 2025 11:02:29 -0400
From: Zi Yan <ziy@...dia.com>
To: Vlastimil Babka <vbabka@...e.cz>, Johannes Weiner <hannes@...xchg.org>
Cc: David Hildenbrand <david@...hat.com>, 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 v5 4/6] mm/page_isolation: remove migratetype from
move_freepages_block_isolate()
On 27 May 2025, at 6:50, Vlastimil Babka wrote:
> On 5/23/25 21:12, Zi Yan wrote:
>> Since migratetype is no longer overwritten during pageblock isolation,
>> moving a pageblock out of MIGRATE_ISOLATE no longer needs a new
>> migratetype.
>>
>> Add pageblock_isolate_and_move_free_pages() and
>> pageblock_unisolate_and_move_free_pages() to be explicit about the page
>> isolation operations. Both share the common code in
>> __move_freepages_block_isolate(), which is renamed from
>> move_freepages_block_isolate().
>>
>> Add toggle_pageblock_isolate() to flip pageblock isolation bit in
>> __move_freepages_block_isolate().
>>
>> Make set_pageblock_migratetype() only accept non MIGRATE_ISOLATE types,
>> so that one should use set_pageblock_isolate() to isolate pageblocks.
>> As a result, move pageblock migratetype code out of
>> __move_freepages_block().
>>
>> Signed-off-by: Zi Yan <ziy@...dia.com>
>> ---
>> include/linux/page-isolation.h | 5 +-
>> mm/page_alloc.c | 97 ++++++++++++++++++++++++++++------
>> mm/page_isolation.c | 21 ++++----
>> 3 files changed, 92 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
>> index 14c6a5f691c2..7241a6719618 100644
>> --- a/include/linux/page-isolation.h
>> +++ b/include/linux/page-isolation.h
>> @@ -44,10 +44,9 @@ static inline void set_pageblock_isolate(struct page *page)
>> void __meminit init_pageblock_migratetype(struct page *page,
>> enum migratetype migratetype,
>> bool isolate);
>> -void set_pageblock_migratetype(struct page *page, enum migratetype migratetype);
>>
>> -bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>> - int migratetype);
>> +bool pageblock_isolate_and_move_free_pages(struct zone *zone, struct page *page);
>> +bool pageblock_unisolate_and_move_free_pages(struct zone *zone, struct page *page);
>>
>> int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>> int migratetype, int flags);
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 8fcbd7fa13c2..44a08b1a9de4 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -524,13 +524,36 @@ void clear_pfnblock_bit(const struct page *page, unsigned long pfn,
>> __clear_bit(bitidx + pb_bit, bitmap_word);
>> }
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +/**
>> + * toggle_pfnblock_bit - Toggle a standalone bit of a pageblock
>> + * @page: The page within the block of interest
>> + * @pfn: The target page frame number
>> + * @pb_bit: pageblock bit to toggle
>> + */
>> +static void toggle_pfnblock_bit(const struct page *page, unsigned long pfn,
>> + enum pageblock_bits pb_bit)
>> +{
>> + unsigned long *bitmap_word;
>> + unsigned long bitidx;
>> +
>> + if (WARN_ON_ONCE(pb_bit <= PB_migrate_end ||
>> + pb_bit >= __NR_PAGEBLOCK_BITS))
>> + return;
>> +
>> + get_pfnblock_bitmap_bitidx(page, pfn, &bitmap_word, &bitidx);
>> +
>> + __change_bit(bitidx + pb_bit, bitmap_word);
>
> Again the non-atomic variant, but actually below I suggest we drop this.
Yep.
>
>> +}
>> +#endif
>> +
>> /**
>> * set_pageblock_migratetype - Set the migratetype of a pageblock
>> * @page: The page within the block of interest
>> * @migratetype: migratetype to set
>> */
>> -__always_inline void set_pageblock_migratetype(struct page *page,
>> - enum migratetype migratetype)
>> +static void set_pageblock_migratetype(struct page *page,
>> + enum migratetype migratetype)
>> {
>> unsigned long mask = MIGRATETYPE_MASK;
>>
>> @@ -540,11 +563,15 @@ __always_inline void set_pageblock_migratetype(struct page *page,
>>
>> #ifdef CONFIG_MEMORY_ISOLATION
>> if (migratetype == MIGRATE_ISOLATE) {
>> - set_pfnblock_bit(page, page_to_pfn(page), PB_migrate_isolate);
>> + VM_WARN_ONCE(1,
>> + "Use set_pageblock_isolate() for pageblock isolation");
>> return;
>> }
>> /* change mask to clear PB_migrate_isolate if it is set */
>> mask = MIGRATETYPE_AND_ISO_MASK;
>> + VM_WARN_ONCE(get_pfnblock_bit(page, page_to_pfn(page),
>> + PB_migrate_isolate),
>> + "Use clear_pageblock_isolate() to unisolate pageblock");
>> #endif
>
> We might be too paranoid with the warnings given these are all local
> functions to this file so risk of misuse should be low. Maybe we could
> remove later...
Yeah. In the next step, when struct pageblock_info is used to change
a pageblock migratetype and isolation state, these warnings should
go away, since caller will need to be explicit about isolation operations.
>
>> __set_pfnblock_flags_mask(page, page_to_pfn(page),
>> (unsigned long)migratetype, mask);
>> @@ -1931,8 +1958,8 @@ static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
>> #endif
>>
>> /*
>> - * Change the type of a block and move all its free pages to that
>> - * type's freelist.
>> + * Move all free pages of a block to new type's freelist. Caller needs to
>> + * change the block type.
>> */
>> static int __move_freepages_block(struct zone *zone, unsigned long start_pfn,
>> int old_mt, int new_mt)
>> @@ -1964,8 +1991,6 @@ static int __move_freepages_block(struct zone *zone, unsigned long start_pfn,
>> pages_moved += 1 << order;
>> }
>>
>> - set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>> -
>> return pages_moved;
>> }
>>
>> @@ -2023,11 +2048,16 @@ static int move_freepages_block(struct zone *zone, struct page *page,
>> int old_mt, int new_mt)
>> {
>> unsigned long start_pfn;
>> + int res;
>>
>> if (!prep_move_freepages_block(zone, page, &start_pfn, NULL, NULL))
>> return -1;
>>
>> - return __move_freepages_block(zone, start_pfn, old_mt, new_mt);
>> + res = __move_freepages_block(zone, start_pfn, old_mt, new_mt);
>> + set_pageblock_migratetype(pfn_to_page(start_pfn), new_mt);
>> +
>> + return res;
>> +
>> }
>>
>> #ifdef CONFIG_MEMORY_ISOLATION
>> @@ -2055,11 +2085,16 @@ static unsigned long find_large_buddy(unsigned long start_pfn)
>> return start_pfn;
>> }
>>
>> +static inline void toggle_pageblock_isolate(struct page *page)
>> +{
>> + toggle_pfnblock_bit(page, page_to_pfn(page), PB_migrate_isolate);
>> +}
>
> I'm wary about the togle action, as we should always know what action we
> want to do anyway. So we could just add a "bool isolate" parameter and call
> set or clear explicitly? Allows for some hypothetical DEBUG_VM checks too
> (pageblock is not already in the state we want it to be).
This function was added to follow Johannes’ suggestion of getting rid of
if statement. I can change it back, make it explicit, and add
an VM_WARN_ONCE.
Hi Johannes,
If you want the non if statement version to stay, please let me know.
Best Regards,
Yan, Zi
Powered by blists - more mailing lists