linux-kernel - Re: [PATCH v4 1/4] mm/page_isolation: make page isolation a standalone bit.

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: <D9V00HM1BVDZ.106ALY0AVVHFK@google.com>
Date: Tue, 13 May 2025 11:32:52 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Zi Yan <ziy@...dia.com>, David Hildenbrand <david@...hat.com>, 
	Oscar Salvador <osalvador@...e.de>, Johannes Weiner <hannes@...xchg.org>, <linux-mm@...ck.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Vlastimil Babka <vbabka@...e.cz>, 
	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>, Richard Chang <richardycc@...gle.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/4] mm/page_isolation: make page isolation a
 standalone bit.

Hi Zi,

I hope you don't mind me jumping in on a late revision like this...

On Fri May 9, 2025 at 8:01 PM UTC, Zi Yan wrote:
> During page isolation, the original migratetype is overwritten, since
> MIGRATE_* are enums and stored in pageblock bitmaps. Change
> MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
> PB_migrate_skip, so that migratetype is not lost during pageblock
> isolation. pageblock bits needs to be word aligned, so expand
> the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.

Forgive my ignorance but can you help me confirm if I'm following this -
Do you just mean that NR_PAGEBLOCK_BITS must divide the word size? Or is
there something else going on here?

> +#ifdef CONFIG_MEMORY_ISOLATION
> +	PB_migrate_isolate = 7, /* If set the block is isolated */
> +			/* set it to 7 to make pageblock bit word aligned */
> +#endif

I feel I'm always just asking for commentary so please feel free to
complain if this is annoying. But I think it would be worth adding the 
context from the commit message into the code here (or somewhere else),
e.g:

/*
 * Page isolation is represented with a separate bit, so that the other
 * bits can store the migratetype that the block had before it was
 * isolated.
 */

Just adding in that detail about the intent will help readers a lot IMO.

>  
> +unsigned long get_pageblock_migratetype(const struct page *page)
> +{
> +	unsigned long flags;
> +
> +	flags = get_pfnblock_flags_mask(page, page_to_pfn(page),
> +			MIGRATETYPE_MASK);
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	if (flags & PB_migrate_isolate_bit)
> +		return MIGRATE_ISOLATE;
> +#endif
> +	return flags;
> +}

Can we just do get_pageblock_migratetype(page, page_to_pfn(page)) here?

>  static __always_inline int get_pfnblock_migratetype(const struct page *page,
>  					unsigned long pfn)
>  {
> -	return get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
> +	unsigned long flags;
> +
> +	flags = get_pfnblock_flags_mask(page, pfn,
> +			MIGRATETYPE_MASK);
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	if (flags & PB_migrate_isolate_bit)
> +		return MIGRATE_ISOLATE;
> +#endif
> +	return flags;
>  }