Received-SPF: pass (google.com: domain of linux-kernel+bounces-184323-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 21 May 2024 01:40:26 +0100
From: Matthew Wilcox <willy@infradead.org>
To: Sidhartha Kumar <sidhartha.kumar@oracle.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	akpm@linux-foundation.org, vishal.moola@oracle.com,
	muchun.song@linux.dev, peterx@redhat.com, osalvador@suse.de
Subject: Re: [PATCH] mm/hugetlb: remove {Set,Clear}Hpage macros
Message-ID: <ZkvteqofTbUK2XAu@casper.infradead.org>
References: <20240520224407.110062-1-sidhartha.kumar@oracle.com>
 <ZkvdAGyua4daHg_S@casper.infradead.org>
 <26e49614-ddb5-4d4c-97c1-a3a8c06d5bb5@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <26e49614-ddb5-4d4c-97c1-a3a8c06d5bb5@oracle.com>

On Mon, May 20, 2024 at 04:44:53PM -0700, Sidhartha Kumar wrote:
> On 5/20/24 4:30 PM, Matthew Wilcox wrote:
> > On Mon, May 20, 2024 at 03:44:07PM -0700, Sidhartha Kumar wrote:
> > > All users have been converted to use the folio version of these macros,
> > > we can safely remove the page based interface.
> > 
> > Yay!
> > 
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> There is only one remaining user of page-based Test version of these macros.
> 
> in mm/memory-hotplug.c:
> 
> 		if (!PageHuge(page))
> 			continue;
> 		head = compound_head(page);
> 		/*
> 		 * This test is racy as we hold no reference or lock.  The
> 		 * hugetlb page could have been free'ed and head is no longer
> 		 * a hugetlb page before the following check.  In such unlikely
> 		 * cases false positives and negatives are possible.  Calling
> 		 * code must deal with these scenarios.
> 		 */
> 		if (HPageMigratable(head))
> 			goto found;
> 		skip = compound_nr(head) - (pfn - page_to_pfn(head));
> 
> 
> I've previously sent a patch to convert this to folios[1] but got feedback
> that it was unsafe. But I'm not sure why replacing compound_head() with
> page_folio() and using folio_test_hugetlb_migratable(folio) rather than
> HPageMigratable(head) changes the existing behavior. With no reference or
> lock, can't the head pointer also be moved and no longer be a part of page
> like the comment states. So would the folio conversion just be maintaining
> this level of existing un-safety that the calling code should handle
> anyways?

To be fair, that wasn't the last thing I said about that ...

https://lore.kernel.org/lkml/Y89DK23hYiLtgGNk@casper.infradead.org/

and looked like David agreed that this was a case where false
postive/negative was fine; we were just looking to be right most of
the time.