From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 0FAA82D94AF; Thu, 4 Dec 2025 11:52:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764849166; cv=none; b=aU+15y7VoybUVtrS0Rwv9d/sRCJ4xtm2sc8Iz6ZbuHVpUucgzWsp59EzHunAWLHfAE+0YgVFzP06lOZ/+mHA1Aq+Fn7MZ/1vCxi51ReI5Z3FWcRBm63HP6MAiAQVoVN59qSdj8hMd+8Inv6sPe/rBpxlCNBZGlmz46z3k0pZIU8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764849166; c=relaxed/simple; bh=u1Zp6G/Tnzzf/H5X3ENLn84JA+wr9i7r3UFkx+PY1cA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=EiFCZcczgkFzrmfVs2jTvnWotjF16VlcnARM/m3hvjUKgbrvOyflRW2o5AqJI/EC/XY6OH+Ymk8kEA6UFtdaE3gX5XVNwlqQq6CV5KnhEBw2tHyaLw5Of/fsdwap+6AHyrr+vUxrfoQCj000pdDPJdeGLlOe7nIIYju3hR9jWcM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ISfT7eeA; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ISfT7eeA" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 68460C4CEFB; Thu, 4 Dec 2025 11:52:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1764849165; bh=u1Zp6G/Tnzzf/H5X3ENLn84JA+wr9i7r3UFkx+PY1cA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ISfT7eeAMaUkykWFftAF34p60f1XrKRSCDcOgF5EQ7sjgQQQ6kR+ZfcxZf3nREGvz 3ImEd584csKzW2sprI2y59/eBKWA2YlWw7s0K9vR4XEpOPMmlPP8wvttqFfXSE4zN6 oekU1nlMo+H/Bg6n4zYp/bTOGvrxv/gOpiDiNlxCA9Y4OE+4DDcmSt+S8/kIsT/ev5 Uux8ESGp7F4HLNoZUmXHGJYu5mcFtULBpaUzDu9+oyUtKeFcenBNNa96Vy0OfGm27t Z0EcKxK9WF0aNa8nPuC5BwJT+8bBhjdQ74Oz3kptWtnS35Y+COG+101Ok84TWX356P FasrAKF5f+8MA== Message-ID: <93d04ef8-0364-4013-8839-ba599d930cb2@kernel.org> Date: Thu, 4 Dec 2025 12:52:32 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 08/12] mm: enable lazy_mmu sections to nest To: Anshuman Khandual , Kevin Brodsky , linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org, Alexander Gordeev , Andreas Larsson , Andrew Morton , Boris Ostrovsky , Borislav Petkov , Catalin Marinas , Christophe Leroy , Dave Hansen , "David S. Miller" , David Woodhouse , "H. Peter Anvin" , Ingo Molnar , Jann Horn , Juergen Gross , "Liam R. Howlett" , Lorenzo Stoakes , Madhavan Srinivasan , Michael Ellerman , Michal Hocko , Mike Rapoport , Nicholas Piggin , Peter Zijlstra , "Ritesh Harjani (IBM)" , Ryan Roberts , Suren Baghdasaryan , Thomas Gleixner , Venkat Rao Bagalkote , Vlastimil Babka , Will Deacon , Yeoreum Yun , linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org References: <20251124132228.622678-1-kevin.brodsky@arm.com> <20251124132228.622678-9-kevin.brodsky@arm.com> <2dfd54d7-fe2a-4921-85ff-a581392a777a@arm.com> From: "David Hildenbrand (Red Hat)" Content-Language: en-US In-Reply-To: <2dfd54d7-fe2a-4921-85ff-a581392a777a@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Some comments from my side: >> static inline void arch_enter_lazy_mmu_mode(void) >> { >> - /* >> - * lazy_mmu_mode is not supposed to permit nesting. But in practice this >> - * does happen with CONFIG_DEBUG_PAGEALLOC, where a page allocation >> - * inside a lazy_mmu_mode section (such as zap_pte_range()) will change >> - * permissions on the linear map with apply_to_page_range(), which >> - * re-enters lazy_mmu_mode. So we tolerate nesting in our >> - * implementation. The first call to arch_leave_lazy_mmu_mode() will >> - * flush and clear the flag such that the remainder of the work in the >> - * outer nest behaves as if outside of lazy mmu mode. This is safe and >> - * keeps tracking simple. >> - */ >> - >> set_thread_flag(TIF_LAZY_MMU);> } > > Should not platform specific changes be deferred to subsequent patches until > nesting is completely enabled in generic first ? Although no problem as such > but would be bit cleaner. This could indeed be done in a separate patch. But I also don't see a problem with updating the doc in this patch. > >> >> diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h >> index a82aa80c0ba4..11bf319d78ec 100644 >> --- a/include/linux/mm_types_task.h >> +++ b/include/linux/mm_types_task.h >> @@ -88,4 +88,9 @@ struct tlbflush_unmap_batch { >> #endif >> }; >> >> +struct lazy_mmu_state { >> + u8 enable_count; >> + u8 pause_count; >> +}; >> + > > Should not this be wrapped with CONFIG_ARCH_HAS_LAZY_MMU_MODE as the task_struct > element 'lazy_mmu_state' is only available with the feature. No strong opinion; the compiler will ignore it either way. And less ifdef is good, right? :) ... and there is nothing magical in there that would result in other dependencies. > Besides, is a depth > of 256 really expected here ? 4 bits for each element would not be sufficient for > a depth of 16 ? We could indeed use something like struct lazy_mmu_state { u8 enable_count : 4; u8 pause_count : 4; }; but then, the individual operations on enable_count/pause_count need more instructions. Further, as discussed, this 1 additional byte barely matters given the existing size of the task struct. No strong opinion. > >> */ >> #ifdef CONFIG_ARCH_HAS_LAZY_MMU_MODE >> +/** >> + * lazy_mmu_mode_enable() - Enable the lazy MMU mode. >> + * >> + * Enters a new lazy MMU mode section; if the mode was not already enabled, >> + * enables it and calls arch_enter_lazy_mmu_mode(). >> + * >> + * Must be paired with a call to lazy_mmu_mode_disable(). >> + * >> + * Has no effect if called: >> + * - While paused - see lazy_mmu_mode_pause() >> + * - In interrupt context >> + */ >> static inline void lazy_mmu_mode_enable(void) >> { >> - if (in_interrupt()) >> + struct lazy_mmu_state *state = ¤t->lazy_mmu_state; >> + >> + if (in_interrupt() || state->pause_count > 0) >> return; >> >> - arch_enter_lazy_mmu_mode(); >> + VM_WARN_ON_ONCE(state->enable_count == U8_MAX); >> + >> + if (state->enable_count++ == 0) >> + arch_enter_lazy_mmu_mode(); > > When lazy_mmu_mode_enable() gets called for the first time with state->enable_count as 0, > then arch_enter_lazy_mmu_mode() will not get called ? Bit confused. state->enable_count++ returns the old value (0). Are you thinking of ++state->enable_count? But maybe I misudnerstood your concern. [...] >> +/** >> + * lazy_mmu_mode_pause() - Resume the lazy MMU mode. >> + * >> + * Resumes the lazy MMU mode; if it was active at the point where the matching >> + * call to lazy_mmu_mode_pause() was made, re-enables it and calls >> + * arch_enter_lazy_mmu_mode(). >> + * >> + * Must match a call to lazy_mmu_mode_pause(). >> + * >> + * Has no effect if called: >> + * - While paused (inside another pause()/resume() pair) >> + * - In interrupt context >> + */ >> static inline void lazy_mmu_mode_resume(void) >> { >> + struct lazy_mmu_state *state = ¤t->lazy_mmu_state; >> + >> if (in_interrupt()) >> return; >> >> - arch_enter_lazy_mmu_mode(); >> + VM_WARN_ON_ONCE(state->pause_count == 0); >> + >> + if (--state->pause_count == 0 && state->enable_count > 0) >> + arch_enter_lazy_mmu_mode(); >> } > > Should not state->pause/enable_count tests and increment/decrement be handled > inside include/linux/sched via helpers like in_lazy_mmu_mode() ? This is will > ensure cleaner abstraction with respect to task_struct. I don't think this is required given that this code here implements CONFIG_ARCH_HAS_LAZY_MMU_MODE support. -- Cheers David