Message-ID: <dc433a44-0653-4554-a31e-cddd31135977@nvidia.com>
Date: Wed, 21 Jan 2026 14:51:29 +1100
From: Balbir Singh <balbirs@...dia.com>
To: Zi Yan <ziy@...dia.com>, Jason Gunthorpe <jgg@...dia.com>
Cc: Matthew Wilcox <willy@...radead.org>, Alistair Popple
 <apopple@...dia.com>, Matthew Brost <matthew.brost@...el.com>,
 Vlastimil Babka <vbabka@...e.cz>, Francois Dugast
 <francois.dugast@...el.com>, intel-xe@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, adhavan Srinivasan <maddy@...ux.ibm.com>,
 Nicholas Piggin <npiggin@...il.com>, Michael Ellerman <mpe@...erman.id.au>,
 "Christophe Leroy (CS GROUP)" <chleroy@...nel.org>,
 Felix Kuehling <Felix.Kuehling@....com>,
 Alex Deucher <alexander.deucher@....com>,
 Christian König <christian.koenig@....com>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Lyude Paul <lyude@...hat.com>, Danilo Krummrich <dakr@...nel.org>,
 David Hildenbrand <david@...nel.org>, Oscar Salvador <osalvador@...e.de>,
 Andrew Morton <akpm@...ux-foundation.org>, Leon Romanovsky
 <leon@...nel.org>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 "Liam R . Howlett" <Liam.Howlett@...cle.com>, Mike Rapoport
 <rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>,
 Michal Hocko <mhocko@...e.com>, linuxppc-dev@...ts.ozlabs.org,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 amd-gfx@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
 linux-mm@...ck.org, linux-cxl@...r.kernel.org
Subject: Re: [PATCH v6 1/5] mm/zone_device: Reinitialize large zone device
 private folios

On 1/20/26 13:50, Zi Yan wrote:
> On 19 Jan 2026, at 17:15, Balbir Singh wrote:
> 
>> On 1/20/26 07:35, Jason Gunthorpe wrote:
>>> On Mon, Jan 19, 2026 at 03:09:00PM -0500, Zi Yan wrote:
>>>>> diff --git a/mm/internal.h b/mm/internal.h
>>>>> index e430da900430a1..a7d3f5e4b85e49 100644
>>>>> --- a/mm/internal.h
>>>>> +++ b/mm/internal.h
>>>>> @@ -806,14 +806,21 @@ static inline void prep_compound_head(struct page *page, unsigned int order)
>>>>>  		atomic_set(&folio->_pincount, 0);
>>>>>  		atomic_set(&folio->_entire_mapcount, -1);
>>>>>  	}
>>>>> -	if (order > 1)
>>>>> +	if (order > 1) {
>>>>>  		INIT_LIST_HEAD(&folio->_deferred_list);
>>>>> +	} else {
>>>>> +		folio->mapping = NULL;
>>>>> +#ifdef CONFIG_MEMCG
>>>>> +		folio->memcg_data = 0;
>>>>> +#endif
>>>>> +	}
>>>>
>>>> prep_compound_head() is only called on >0 order pages. The above
>>>> code means when order == 1, folio->mapping and folio->memcg_data are
>>>> assigned NULL.
>>>
>>> OK, fair enough, the conditionals would have to change and maybe it
>>> shouldn't be called "compound_head" if it also cleans up normal pages.
>>>
>>>>>  static inline void prep_compound_tail(struct page *head, int tail_idx)
>>>>>  {
>>>>>  	struct page *p = head + tail_idx;
>>>>>
>>>>> +	p->flags.f &= ~0xffUL;	/* Clear possible order, page head */
>>>>
>>>> No one cares about tail page flags if it is not checked in check_new_page()
>>>> from mm/page_alloc.c.
>>>
>>> At least page_fixed_fake_head() does check PG_head in some
>>> configurations. It does seem safer to clear it. Possibly order is
>>> never used, but it is free to clear it.
>>>
>>>>> -	if (order)
>>>>> -		prep_compound_page(page, order);
>>>>> +	prep_compound_page(page, order);
>>>>
>>>> prep_compound_page() should only be called for >0 order pages. This creates
>>>> another weirdness in device pages by assuming all pages are
>>>> compound.
>>>
>>> OK
>>>
>>>>> +	folio = page_folio(page);
>>>>> +	folio->pgmap = pgmap;
>>>>> +	folio_lock(folio);
>>>>> +	folio_set_count(folio, 1);
>>>>
>>>> /* clear possible previous page->mapping */
>>>> folio->mapping = NULL;
>>>>
>>>> /* clear possible previous page->_nr_pages */
>>>> #ifdef CONFIG_MEMCG
>>>> 	folio->memcg_data = 0;
>>>> #endif
>>>
>>> This is reasonable too, but prep_compound_head() was doing more than
>>> that, it is also clearing the order, and this needs to clear the head
>>> bit.  That's why it was apppealing to reuse those functions, but you
>>> are right they are not ideal.
> 
> PG_head is and must be bit 6, that means the stored order needs to be
> at least 2^6=64 to get it set. Who allocates a folio with that large order?
> This p->flags.f &= ~0xffUL thing is unnecessary. What really needs
> to be done is folio->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP to make
> sure the new folio flags are the same as newly allocated folios
> from core MM page allocator.
> 
>>>
>>> I suppose we want some prep_single_page(page) and some reorg to share
>>> code with the other prep function.
> 
> This is just an unnecessary need due to lack of knowledge of/do not want
> to investigate core MM page and folio initialization code.
> 
>>>
>>
>> There is __init_zone_device_page() and __init_single_page(),
>> it does zero out the page and sets the zone, pfn, nid among other things.
>> I propose we use the current version with zone_device_free_folio() as is.
>>
>> We can figure out if __init_zone_device_page() can be reused or refactored
>> for the purposes to doing this with core MM API's
>>
>>
>>>> This patch mixed the concept of page and folio together, thus
>>>> causing confusion. Core MM sees page and folio two separate things:
>>>> 1. page is the smallest internal physical memory management unit,
>>>> 2. folio is an abstraction on top of pages, and other abstractions can be
>>>>    slab, ptdesc, and more (https://kernelnewbies.org/MatthewWilcox/Memdescs).
>>>
>>> I think the users of zone_device_page_init() are principally trying to
>>> create something that can be installed in a non-special PTE. Meaning
>>> the output is always a folio because it is going to be read as a folio
>>> in the page walkers.
>>>
>>> Thus, the job of this function is to take the memory range starting at
>>> page for 2^order and turn it into a single valid folio with refcount
>>> of 1.
>>>
>>>> If device pages have to initialize on top of pages with obsolete states,
>>>> at least it should be first initialized as pages, then as folios to avoid
>>>> confusion.
>>>
>>> I don't think so. It should do the above job efficiently and iterate
>>> over the page list exactly once.
> 
> folio initialization should not iterate over any page list, since folio is
> supposed to be treated as a whole instead of individual pages.
> 
> Based on my understanding,
> 
> folio->mapping = NULL;
> folio->memcg_data = 0;
> folio->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> 
> should be enough.
> 

I think it should be enough as well, worst case memcg_data is aliased with
slab_obj_exts, but we don't expect zone device folios to have slab_obj_exts
to be set.

folio->memcg_data needs to be under an #ifdef CONFIG_MEMCG and folio->mapping
was set to NULL during previous free (one could assume it's unchanged)


> if (order)
> 	folio_set_large_rmappable(folio);
> 
> is done at zone_device_folio_init().
> 



Balbir

Powered by blists - more mailing lists