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 4CEED2116E0 for ; Thu, 20 Nov 2025 09:32: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=1763631165; cv=none; b=ol93EP3Ep79AyllNGq1Zp0T6iAms4SI+IOJ5Sms90ymsHXBRkZtyPHYhmW0PXcqoWN3WCBQVrxgjKsWhDGagQhLqQ72/kI/yp26dohmb8hgjl2KuiWpzjXKIykNzT8M60y5xIk35nq695clLPjtINj+WjJTeuLzvwm/uDlLmVZY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763631165; c=relaxed/simple; bh=+3Av/NkAd4SGgUEuqhxitG7xEjFBYKfNAJ67LMoIu0Y=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=c30k8MvdWwoQqWwp5heSSUi24U+sjRcn8xFWEIMnj1hVsJn4n8GgFKDgnWjGyx+TYKRKbICAgd3n3qFoff2Q1IjpqcoN8yADThhKvVD8c359rhb7OA+gB7f74dDYtUSm6aEcvRh7o7qDtvcalX3s4WG4t8+OIkFdZxPVlEGe/Lw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=tEbsCNT7; 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="tEbsCNT7" Received: by smtp.kernel.org (Postfix) with ESMTPSA id EE0D2C4CEF1; Thu, 20 Nov 2025 09:32:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1763631164; bh=+3Av/NkAd4SGgUEuqhxitG7xEjFBYKfNAJ67LMoIu0Y=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=tEbsCNT7/6gD+1ivDvWJzbRfMK0JF5Hn708L0V1QUzk0H0SKcuWsKEvBto7xeStWX xQEicYSvZaFivvw4nXZoP4VfjRSB+JTztJHOg1jVyqjkXUx/TLaU+6HgiQtK3HoLHS RFBZwemGeYUdzS0Rs5O7hExIJpZN3Ny2ZysqYbpSjMczVP0Fkb+ytHmfl4G6suRVG2 iYInV3CcixdMMsKQRIXBWB0DfcXByqKz79hT/ZpaCLZcnGmX2om4OE3CBKPcIfOh2q Gif6vpp3d+1NcoUe2FCOZdU5MZJMuboA6MhZYQs0+fyws6TndBtE8mKPFiBHMnRkpP WH9Q7zLR238Ag== Message-ID: <3c332dea-ea60-4939-9843-ac7d3068c7c8@kernel.org> Date: Thu, 20 Nov 2025 10:32:36 +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] fixup: mm/huge_memory.c: introduce folio_split_unmapped To: Balbir Singh , linux-kernel@vger.kernel.org, linux-mm@kvack.org, dri-devel@lists.freedesktop.org Cc: Andrew Morton , Zi Yan , Joshua Hahn , Rakie Kim , Byungchul Park , Gregory Price , Ying Huang , Alistair Popple , Oscar Salvador , Lorenzo Stoakes , Baolin Wang , "Liam R. Howlett" , Nico Pache , Ryan Roberts , Dev Jain , Barry Song , Lyude Paul , Danilo Krummrich , David Airlie , Simona Vetter , Ralph Campbell , =?UTF-8?Q?Mika_Penttil=C3=A4?= , Matthew Brost , Francois Dugast References: <20251120030709.2933665-1-balbirs@nvidia.com> <4b87bd52-5582-4599-8a97-38843e27d0a7@kernel.org> <875a9449-fcdd-4aef-9d77-1703dd02edf0@nvidia.com> From: "David Hildenbrand (Red Hat)" Content-Language: en-US In-Reply-To: <875a9449-fcdd-4aef-9d77-1703dd02edf0@nvidia.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 11/20/25 10:25, Balbir Singh wrote: > On 11/20/25 20:09, David Hildenbrand (Red Hat) wrote: >> On 11/20/25 04:07, Balbir Singh wrote: >>> Code refactoring of __folio_split() via helper >>> __folio_freeze_and_split_unmapped() caused a regression with clang-20 >>> with CONFIG_SHMEM=n, the compiler was not able to optimize away the >>> call to shmem_uncharge() due to changes in nr_shmem_dropped. >>> Fix this by checking for shmem_mapping() prior to calling >>> shmem_uncharge(), shmem_mapping() returns false when CONFIG_SHMEM=n. >>> >>> smatch also complained about parameter end being used without >>> initialization, which is a false positive, but keep the tool happy >>> by sending in initialized parameters. end is initialized to 0. >>> >>> Add detailed documentation comments for folio_split_unmapped() >>> >>> Cc: Andrew Morton >>> Cc: David Hildenbrand >>> Cc: Zi Yan >>> Cc: Joshua Hahn >>> Cc: Rakie Kim >>> Cc: Byungchul Park >>> Cc: Gregory Price >>> Cc: Ying Huang >>> Cc: Alistair Popple >>> Cc: Oscar Salvador >>> Cc: Lorenzo Stoakes >>> Cc: Baolin Wang >>> Cc: "Liam R. Howlett" >>> Cc: Nico Pache >>> Cc: Ryan Roberts >>> Cc: Dev Jain >>> Cc: Barry Song >>> Cc: Lyude Paul >>> Cc: Danilo Krummrich >>> Cc: David Airlie >>> Cc: Simona Vetter >>> Cc: Ralph Campbell >>> Cc: Mika Penttilä >>> Cc: Matthew Brost >>> Cc: Francois Dugast >>> >>> Signed-off-by: Balbir Singh >>> --- >>>   mm/huge_memory.c | 32 ++++++++++++++++++++++---------- >>>   1 file changed, 22 insertions(+), 10 deletions(-) >>> >>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>> index 78a31a476ad3..c4267a0f74df 100644 >>> --- a/mm/huge_memory.c >>> +++ b/mm/huge_memory.c >>> @@ -3751,6 +3751,7 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n >>>       int ret = 0; >>>       struct deferred_split *ds_queue; >>>   +    VM_WARN_ON_ONCE(!mapping && end != 0); >> >> You could drop the "!= 0" > > Ack > > VM_WARN_ONE(!mapping && end); > >> >>>       /* Prevent deferred_split_scan() touching ->_refcount */ >>>       ds_queue = folio_split_queue_lock(folio); >>>       if (folio_ref_freeze(folio, 1 + extra_pins)) { >>> @@ -3919,7 +3920,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order, >>>       int nr_shmem_dropped = 0; >>>       int remap_flags = 0; >>>       int extra_pins, ret; >>> -    pgoff_t end; >>> +    pgoff_t end = 0; >>>       bool is_hzp; >>>         VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio); >>> @@ -4049,7 +4050,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order, >>>         local_irq_enable(); >>>   -    if (nr_shmem_dropped) >>> +    if (mapping && shmem_mapping(mapping) && nr_shmem_dropped) >>>           shmem_uncharge(mapping->host, nr_shmem_dropped); >> >> That looks questionable. We shouldn't add runtime check to handle buildtime things. >> >> Likely what you want is instead >> >> if (IS_ENABLED(CONFIG_SHMEM) && nr_shmem_dropped) >>     shmem_uncharge() >> > > shmem_mapping() returns false for CONFIG_SHMEM=n and shmem_mapping() checks that the mapping > is indeed for shmem ops before uncharging. Happy to change it if you like, > your version is more readable Good point, but the questionable thing is that it looks like nr_shmem_dropped could be set for non-shmem mappings, when it's really just a compiler thing. What about handling it through a proper stub so we can keep this calling code simple? diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index 5b368f9549d67..e38cb01031200 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -136,11 +136,15 @@ static inline bool shmem_hpage_pmd_enabled(void) #ifdef CONFIG_SHMEM extern unsigned long shmem_swap_usage(struct vm_area_struct *vma); +extern void shmem_uncharge(struct inode *inode, long pages); #else static inline unsigned long shmem_swap_usage(struct vm_area_struct *vma) { return 0; } +static inline void shmem_uncharge(struct inode *inode, long pages) +{ +} #endif extern unsigned long shmem_partial_swap_usage(struct address_space *mapping, pgoff_t start, pgoff_t end); @@ -194,7 +198,6 @@ static inline pgoff_t shmem_fallocend(struct inode *inode, pgoff_t eof) } extern bool shmem_charge(struct inode *inode, long pages); -extern void shmem_uncharge(struct inode *inode, long pages); #ifdef CONFIG_USERFAULTFD #ifdef CONFIG_SHMEM -- Cheers David