From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (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 E46254C9003 for ; Wed, 29 Jul 2026 14:51:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785336686; cv=none; b=QTWIYLf9uD7nblZjQLly1w8D7UoAO0qkXdW80+V6yvjbK9Yss8H1fzSlT3fcUZb3b3JkfqnUFAyMhZi8FNkfnlvii0owzvjN3Vogu0DUdhKyLA2ZsMrX8a37Z0QHlGURjiujNtoD6njraCTZLbpKezHfBy1+8VrrtE7EqbM1+9I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785336686; c=relaxed/simple; bh=S4SKEkw7LgAwGFfcHivIExLNhk2J76au7BzUy9Bb9EI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PcprjUAJF9CvJQDxRNX0UiFJ9vBcPodnfl9FYwuZPNl5+J/ALK25n26cn8aeaVsRp11JrWJ/h4uQqyUovAitgjN94c5bPWdWXUkfD4QrktdbwaWjSqzkwJ5sTwg7xNR4Q+OLB3SQZIlABMXEOOTWmOlRk8/SNApBRWBi3UASWC0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org; spf=pass smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=phZVmtHs; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="phZVmtHs" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=aHXgeatDUGs2QMlgNfa/xZvYZWtxm9XsdP0TtXFZ9dU=; b=phZVmtHsb+NwjYJh2REWWSJasA gKTFDXBOtzlkaA7XVdUsm+D0+ycEQjQuvIUqUCq+LG2Old61PKZFX8+IV0PPREhF4iXWWfGFkpY7T liJNRMKKb2YY7kE4PaeHOF35Yy46QCF/C/P3TrN3ytYsYd6JRB674syS4MmqWXC8l2xUqor9j33T2 HishjnvdxiLLaWCoGnW+Oevs1uMK6smmiXm3mXRhcFK5ob1hX8b3DE2t0gjq0otH8RIvfzzCvQ1Ro /jMH4odRDDzbZDzlEVRq/PdzUC4wahhQNEQ/p/l52/7X8hbUYoGKbEnHV15gYtBgyNmU13PKxw2cs cBD9Hlyg==; Received: from 77-249-17-252.cable.dynamic.v4.ziggo.nl ([77.249.17.252] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.99.1 #2 (Red Hat Linux)) id 1wp5aK-00000007mYk-0Rm3; Wed, 29 Jul 2026 14:48:40 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id 7C3A9300882; Wed, 29 Jul 2026 16:48:38 +0200 (CEST) Date: Wed, 29 Jul 2026 16:48:38 +0200 From: Peter Zijlstra To: Mike Rapoport 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: <20260729144838.GM651302@noisy.programming.kicks-ass.net> References: <20260729110807.797920433@infradead.org> <20260729111119.604452135@infradead.org> 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: 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 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(); > > 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);