From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3A7CD463B60 for ; Wed, 29 Jul 2026 19:07:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785352060; cv=none; b=TQMb7LSLlaeyLyAGjWY3aKb6gJK2EzohaOvEvrJugN+o8YhmyYyWhzzYnUIOlTCCVfpbXtxp/wsJCHDrFV4zsyfZtD33Ft7vvTBND1QRHUzhM0MfrNg5YJrlhO5++5aRJX0/hscGFM6OmVUsjVQUfRO6fq4AlPpF4GgbZ7szUoU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785352060; c=relaxed/simple; bh=32cqXU+r3qMv8Ij4PNZq1NnFaGduJn+PkZIhTGT6VI4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=p0RukSD5sHuS8f7xaW5CvYzIxnu3Zs6cjMeGPDElO7L5w/JiP7qIQFfpHcGdczBoNlBACF7nWmU4sOueXXmpXXT3JQB8SC8n6jkqWBA9HrbcMMWIwAIXZlqdtsBDknNO2bRQhVHzy3ptBG6d8zuyH7adoy3WCyRSUBqUGYmaF5k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=k5L9d1+9; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="k5L9d1+9" Received: by smtp.kernel.org (Postfix) with ESMTPSA id CDA401F00A3A; Wed, 29 Jul 2026 19:07:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1785352058; bh=LSv0NhzAoI5n1gzPa2GHMIZDbJeYAamzCQ0Jv+g85nU=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=k5L9d1+9VqsYK6U/s/MDpqvUbwem2zy8pM5N/EPxUKy9+SaAHYrvCyumWSLGgQPWm UyJM2OTCky/3e2hISfTWrnx8sk2PZB/oARPFCY6WTSPGAQQmotl/vhdDMegtueRSyd Pt+NtX07YVEbfcgHWkfeDXW0M0Q6vxDxq9AWK94BL1pcDuq2uiPi2/5/waICA9Al7p CsDJzyGPWSuHh9ufrQg0zwltGkisvJBkMNCH7gHL+dNZAcMfdZqcysQwyoEnGortOM fwK3B8NhaGkMB8Qsz7UNa1/bctKaUsPHbX+TD+f4nj1C3lS3RwxrVNFSGabuOmKfpX /PUhur6wi+Inw== Date: Wed, 29 Jul 2026 22:07:27 +0300 From: Mike Rapoport To: Peter Zijlstra Cc: Dave Hansen , linux-kernel@vger.kernel.org, Andy Lutomirski , Borislav Petkov , David Hildenbrand , Ingo Molnar , Jason Gunthorpe , Juergen Gross , Kevin Tian , Kiryl Shutsemau , "Liam R. Howlett" , Lorenzo Stoakes , Lu Baolu , "H. Peter Anvin" , Shakeel Butt , Suren Baghdasaryan , Thomas Gleixner , Toshi Kani , Vlastimil Babka , Will Deacon , linux-mm@kvack.org, x86@kernel.org Subject: Re: [PATCH 3/3] x86/mm: Fix and document DEBUG_PAGEALLOC Message-ID: References: <20260729110807.797920433@infradead.org> <20260729111119.604452135@infradead.org> <20260729144838.GM651302@noisy.programming.kicks-ass.net> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260729144838.GM651302@noisy.programming.kicks-ass.net> On Wed, Jul 29, 2026 at 04:48:38PM +0200, Peter Zijlstra wrote: > On Wed, Jul 29, 2026 at 05:13:55PM +0300, Mike Rapoport wrote: > > On Wed, Jul 29, 2026 at 01:08:10PM +0200, Peter Zijlstra wrote: > > > It turns out that commit 5fce67641a3e ("x86/mm/pat: Don't gate > > > cpa_lock on debug_pagealloc_enabled()") was a little too quick to > > > remove the debug_pagealloc exception for cpa_lock. > > > > > > Notably __kernel_map_pages() is used by the page-allocator from any > > > context the page-allocator itself is used, which violates the cpa_lock > > > rules. > > > > > > Re-instate the exception, except make it specific to the > > > __kernel_map_pages() such that any other cpa() usage is still fully > > > serialized by cpa_lock. Also note that since cpa() should not be used > > > on memory that isn't allocated, the page-allocator locking and cpa are > > > infact mutually exclusive and all cpa usage in fully serialized. > > > > > > Add a comment explaining this and other 'funnies' surrounding > > > DEBUG_PAGEALLOC, including how pgd_lock is not affected and the TLB > > > trickery. > > > > > > Fixes: 5fce67641a3e ("x86/mm/pat: Don't gate cpa_lock on debug_pagealloc_enabled()") > > > Signed-off-by: Peter Zijlstra (Intel) > > > > > > + /* > > > + * DEBUG_PAGEALLOC is special; it is called from any context the > > > + * page-allocator is, which violates the normal cpa_lock locking > > > + * rules. > > > + * > > > + * However, since it is part of the page-allocator, things are still > > > + * properly serialized by the page-allocator locking and the fact that > > > + * when a page is owned by the page-allocator, it isn't owned by > > > + * anybody else. That is, you *SHOULD* not be calling cpa() on memory > > > > *SHOULD NOT* ? > > Well yeah, d'0h. > > > > + * that isn't allocated. > > > + * > > > + * Additionally, DEBUG_PAGEALLOC ensures (per probe_page_size_mask()) > > > + * that the kernel mapping is 4k pages, therefore there are no large > > > + * pages to split/collapse. > > > + * > > > + * Furthermore, the page-allocator strictly manages pages that > > > + * *exist*, avoiding pgd_lock. > > > + * > > > + * Therefore, it is safe to not take cpa_lock. > > > + */ > > > + if (debug_pagealloc_enabled() && (cpa->flags & CPA_DEBUG_PAGEALLOC)) > > > + lock = false; > > > + > > > while (rempages) { > > > /* > > > * Store the remaining nr of pages for the large page > > > @@ -2008,9 +2033,12 @@ static int __change_page_attr_set_clr(st > > > if (cpa->flags & (CPA_ARRAY | CPA_PAGES_ARRAY)) > > > cpa->numpages = 1; > > > > > > - spin_lock(&cpa_lock); > > > - ret = __change_page_attr(cpa, primary); > > > - spin_unlock(&cpa_lock); > > > + if (lock) { > > > + guard(spinlock)(&cpa_lock); > > > + ret = __change_page_attr(cpa, primary); > > > + } else { > > > + ret = __change_page_attr(cpa, primary); > > > + } > > > > This does make DEBUG_PAGEALLOC exception more explicit *here*, but OTOH the > > spin_(un)lock(&cpa_lock) in split_large_page() becomes confusing. > > So as the comment states, with DEBUG_PAGEALLOC there are no large pages, > so you should never hit split_large_page(). > > It is the same as pgd_lock; that isn't guarded anywhere either, and > works by the same reasons; DEBUG_PAGEALLOC isn't ever supposed to hit > those paths. > > > I like my version with your comments added there more as it localizes the > > DEBUG_PAGEALLOC exception in the lock wrappers. > > So I don't like removing cpa_lock entirely; it is still serializing cpa I meant static inline cpa_lock(), not removing it entirely. With your comment why DEBUG_PAGEALLOC is special. > usage, even though it isn't as critical on 4k only. Having cpa behave > significantly different for DEBUG_PAGEALLOC just seems like a very dodgy > situation. > > And again, pdg_lock is in the same spot. It all works because the code > 'magically' never hits the pgd_lock taking paths. > > > > if (ret) > > > goto out; > > > > > > @@ -2661,15 +2689,23 @@ void __kernel_map_pages(struct page *pag > > > * and hence no memory allocations during large page split. > > > */ > > > > I'd also return early and maybe even WARN if !debug_pagealloc_enabled(). > > The callsites be like: > > if (debug_pagealloc_enabled_static()) > __kernel_map_pages(); Yes, but there might appear new callsites (I really hope not). It's anyway really slow path so an extra if won't make a difference but with it it's clearer that __kernel_map_pages() is for debug only. > > > if (enable) > > > - __set_pages_p(page, numpages); > > > + __set_pages_p(page, numpages, CPA_DEBUG_PAGEALLOC); > > > else > > > - __set_pages_np(page, numpages); > > > + __set_pages_np(page, numpages, CPA_DEBUG_PAGEALLOC); -- Sincerely yours, Mike.