Re: [PATCH 2/2] mm/hugetlb: refactor subpage recording

From: Zi Yan <ziy@nvidia.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Joao Martins <joao.m.martins@oracle.com>, <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	John Hubbard <jhubbard@nvidia.com>
Subject: Re: [PATCH 2/2] mm/hugetlb: refactor subpage recording
Date: Tue, 26 Jan 2021 21:50:39 -0500	[thread overview]
Message-ID: <312AECBD-CA6D-4E93-A6C1-1DF87BABD92D@nvidia.com> (raw)
In-Reply-To: <20210127022438.GQ308988@casper.infradead.org>

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On 26 Jan 2021, at 21:24, Matthew Wilcox wrote:

> On Tue, Jan 26, 2021 at 08:07:30PM -0400, Jason Gunthorpe wrote:
>> I'm looking at Matt's folio patches and see:
>>
>> +static inline struct folio *next_folio(struct folio *folio)
>> +{
>> +       return folio + folio_nr_pages(folio);
>> +}
>
> This is a replacement for places that would do 'page++'.  eg it's
> used by the bio iterator where we already checked that the phys addr
> and the struct page are contiguous.
>
>> And checking page_trans_huge_mapcount():
>>
>> 	for (i = 0; i < thp_nr_pages(page); i++) {
>> 		mapcount = atomic_read(&page[i]._mapcount) + 1;
>
> I think we are guaranteed this for transparent huge pages.  At least
> for now.  Zi Yan may have some thoughts for his work on 1GB transhuge
> pages ...

It should work for 1GB THP too. My implementation allocates 1GB pages
from cma_alloc(), which calls alloc_contig_range(). At least for now
subpages from a 1GB THP are physically contiguous.

It will be a concern if we use other ways (like migrating in-use pages)
of forming 1GB THPs. Thanks for pointing this out.

>
>> And we have the same logic in hmm_vma_walk_pud():
>>
>> 	if (pud_huge(pud) && pud_devmap(pud)) {
>> 		pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
>> 		for (i = 0; i < npages; ++i, ++pfn)
>> 			hmm_pfns[i] = pfn | cpu_flags;
>>
>> So, if page[n] does not access the tail pages of a compound we have
>> many more people who are surprised by this than just GUP.
>>
>> Where are these special rules for hugetlb compound tails documented?
>> Why does it need to be like this?
>>
>> Isn't it saner to forbid a compound and its tails from being
>> non-linear in the page array? That limits when compounds can be
>> created, but seems more likely to happen than a full mm audit to find
>> all the places that assume linearity.
>>
>> Jason

—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]