Received-SPF: pass (google.com: domain of linux-kernel+bounces-96174-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 7 Mar 2024 16:16:55 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: peterx@redhat.com
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Muchun Song <muchun.song@linux.dev>,
	Matthew Wilcox <willy@infradead.org>,
	Mike Rapoport <rppt@kernel.org>,
	Christophe Leroy <christophe.leroy@csgroup.eu>, x86@kernel.org,
	sparclinux@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH RFC 04/13] mm/x86: Change pXd_huge() behavior to exclude
 swap entries
Message-ID: <20240307201655.GF9179@nvidia.com>
References: <20240306104147.193052-1-peterx@redhat.com>
 <20240306104147.193052-5-peterx@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240306104147.193052-5-peterx@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?U0pwSFhuZklCNENRWS9mVVNlOUFOVDBPdnFyOEJkYVV5cTJlL0VmNjVBZlpj?=
 =?utf-8?B?UWZDMVY3SDdWZnptNFlrYjJoajVFNC9JSVFhdDBPZm5ZNE1ITjVtNzlxaS9O?=
 =?utf-8?B?cVAzcU5OUFZXYis0Y1NnZm1wbFJqMHI3S1VYdktCMDhUaXhLbSthc0tiTHdD?=
 =?utf-8?B?RkdjUzk3MTM1SFBzekxVdVJjZ3A5WjhkS3Fxa0N6eWo0emtoT1gvMG5yMG5J?=
 =?utf-8?B?cWlvdnFPZXFLTytnNTAwOHYxUzFjMGlIRGxGY0RtdThTR2JsQlNqQytwMEtr?=
 =?utf-8?B?dWpSOGVicWxkdEdyRXpad3NaU2I5Zktvcjd5QWJnSW5PUkR1aU5RdGtNNVh5?=
 =?utf-8?B?elRCVVdnQ0Y5b2cxZURwSGNPZi91V2NCNjY3N3RUMnAyRnpVZXYzb1cySjds?=
 =?utf-8?B?Rm91Vkl0YXdZbHFYbmoranAwWWM3TEF5a3hHOEkvMExCSDRrNmlnOHhna05m?=
 =?utf-8?B?UEhVdVd6Vzc1UXRQdmdNMTJZaGptdlpFeEs2Q3ZJNjB2Wlh2S0IvZjNxSUFR?=
 =?utf-8?B?dCtGd1VrT0RFREFMYTI4eTZIbjM2N2twUXFYWjNwMmw5U1cyeXNrY0xya2xY?=
 =?utf-8?B?elM3SDVTWUxDUk5iZG1JL1lkZ2loMzlkNmZVaGhIcXY4QUJicXpWTlRKYnl5?=
 =?utf-8?B?d2QxQjRDbDArZUcwK0RpM1RCa21aaCswZGdPRldLVXVtaGVRdDdmSWxGTm5K?=
 =?utf-8?B?SVNJQk9CYkNMT2pCUXIzdXo0cWI2cERURUp6VXVZS1NsWVVWWE1xTEpybGR6?=
 =?utf-8?B?aTdBU0pLK1lMbkxReTh6U2F6SUpONkFNRXUyZEFyNytFS0grNXF0MHlOT1pL?=
 =?utf-8?B?Vi8zclUwMG93MXF0ZDlsRk9JYUVhNXFhRGp6UnJTOGk1Qkppb0tHZUllRVR6?=
 =?utf-8?B?RmFRR1B3WStuMDJHZUdZZnNBN25xSzJKZEVmOW9oeVd1aW92ejRVWXErZDJL?=
 =?utf-8?B?MnVpN0pLdWU1eVBTT1ROQjZBOFM5TysxcTB3TXM0WU1iemxIRlcrdXlLTWZu?=
 =?utf-8?B?WmdSZlZScHo4OXdaR0kvM0lkZzFvTXhteWhkaDhKRTJJK1N4eTNGUDZqNmxM?=
 =?utf-8?B?clJkQ3NYbHJlblFWSFd6V205UWhqVm53VUZTTzZvaWxpQndCbVZydXNnQkxr?=
 =?utf-8?B?amF3Mkhlcmc3bGNIbkJnQjRIUVRQWFJlVWh4NVBNUUlBYzJUcXJ6dWJiMWVD?=
 =?utf-8?B?MkdwaXR2UHhWU2tDa1dMWTEvdWk4S3l1OUIwVFYzRElmOUFabHFmYjZ5czBk?=
 =?utf-8?B?MEZMS0F6RTg1Rk02SUNYdzh4enBFUWF6NGxFZjZaVU9nVjQrc2NxUTRYQi9h?=
 =?utf-8?B?S04xYkNZaDdQQ1dpU3pZQ3U0WlhQM0NWUHk3cjM4SGUvUXlGczVKVzM3WUFC?=
 =?utf-8?B?Mm1BcStNNW9YNVhXTld6OWdlVzlWN1RIdnFwZ3RoNFhLQUtXYkFRcHJTQWo2?=
 =?utf-8?B?TmMybU5xaWdwdjErUmI5S3FRRmphZjJUbVpNUXRyM01vU2pjUEtqR003YmxL?=
 =?utf-8?B?NEIwNGdtallXWWtXN0M5N3ZMbTVCMjdhSTlmMFJZK3hQWWV0RWdpeUZJbW5G?=
 =?utf-8?B?UnF5TW9KVExINFcwMHhsYWVtOE82VHFoajVGRW5DV1UxeExlMURYQ3UvY2tV?=
 =?utf-8?B?ZDJXdlpXektTeUhVTlpObC9kZ0JLOGZzZkdWTXZEUmx5K1JnQUU1TDZEd0pI?=
 =?utf-8?B?WXNUNXZJalhBN21yUjVodnhTdUlVb0tFMk1HN0lqS1pZUUtBSUUxaHFoOGF5?=
 =?utf-8?B?THhyYUl5b1NEWkYrRW1wZytRRVAvWFNYczJ2ZVdseWM2SGg5YnBnNGVMQ3NI?=
 =?utf-8?B?cDJlSzRaOCtVSk9MNW9uMmhpbUpsR0ZGeE1MajAyU3RFa0NnVHYrWGowTmtl?=
 =?utf-8?B?aVFoMk1yb2wwaVRUSG5NOFJsNFNCREdTMnFzeWdKZWJ1bTBhVlVGeURJYVhR?=
 =?utf-8?B?a21NVkE0Q3R3RHhydmp6dXcxeGQyMko0YnFpdjZoTVprRVIvc3N5YnFYWXk4?=
 =?utf-8?B?d3hTY1hYenc1ekFnU29UbWpEdVdxbUJFcUNqSVVncHNpSklVemFpTnpUVFZB?=
 =?utf-8?B?eUdXV3ZTblkwU2F6T1RSNUwwb1JPcjd0dTA0MGthM3pyQ1ZCQnRKazFwNUJ4?=
 =?utf-8?Q?w/zQ=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5f5be981-b8a3-4f7f-016a-08dc3ee38a08
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3849.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2024 20:16:57.5340
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: z5+PuvTjZrLgdSDdJIvRa5G12f22nrcc9yreOm+GTWyDuyBejV1Cd0ZX5bcGF6GP
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4396

On Wed, Mar 06, 2024 at 06:41:38PM +0800, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> This patch partly reverts below commits:
> 
> 3a194f3f8ad0 ("mm/hugetlb: make pud_huge() and follow_huge_pud() aware of non-present pud entry")
> cbef8478bee5 ("mm/hugetlb: pmd_huge() returns true for non-present hugepage")
> 
> Right now, pXd_huge() definition across kernel is unclear. We have two
> groups that think differently on swap entries:
> 
>   - x86/sparc:     Allow pXd_huge() to accept swap entries
>   - all the rest:  Doesn't allow pXd_huge() to accept swap entries
> 
> This is so confusing.  Since the sparc helpers seem to be added in 2016,
> which is after x86's (2015), so sparc could have followed a trend.  x86
> proposed such swap handling in 2015 to resolve hugetlb swap entries hit in
> GUP, but now GUP guards swap entries with !pXd_present() in all layers so
> we should be safe.
> 
> We should define this API properly, one way or another, rather than keep
> them defined differently across archs.
> 
> Gut feeling tells me that pXd_huge() shouldn't include swap entries, and it
> turns out that I am not the only one thinking so, the question was raised
> when the current pmd_huge() for x86 was proposed by Ville Syrjälä:
> 
> https://lore.kernel.org/all/Y2WQ7I4LXh8iUIRd@intel.com/
> 
>   I might also be missing something obvious, but why is it even necessary
>   to treat PRESENT==0+PSE==0 as a huge entry?
> 
> It is also questioned when Jason Gunthorpe reviewed the other patchset on
> swap entry handlings:
> 
> https://lore.kernel.org/all/20240221125753.GQ13330@nvidia.com/
> 
> Revert its meaning back to original.  It shouldn't have any functional
> change as we should be ready with guards on !pXd_present() explicitly
> everywhere.
> 
> Note that I also dropped the "#if CONFIG_PGTABLE_LEVELS > 2", it was there
> probably because it was breaking things when 3a194f3f8ad0 was proposed,
> according to the report here:
> 
> https://lore.kernel.org/all/Y2LYXItKQyaJTv8j@intel.com/
> 
> Now we shouldn't need that.
> 
> Instead of reverting to _PAGE_PSE raw check, leverage pXd_leaf().
> 
> Cc: Naoya Horiguchi <naoya.horiguchi@nec.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: x86@kernel.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/mm/hugetlbpage.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)

I think this is the right thing to do, callers should be more directly
sensitive to swap entries not back into it indirectly from a helper
like this.

Jason