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 0CAD4372AC1 for ; Thu, 20 Nov 2025 10:43:34 +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=1763635415; cv=none; b=SZ+yN5lZtvKa3GNUlVnDhU6VIc9LsyeB9MPy5DZq2JtF+VA4TYiwa48AUlo7WC2zhajsXFEeSuUtTlch6XgP8yGcnQoYyNINAlVkH0mHZ16gz/YaOztZ7sQOk4BBBe1JlbJGdBMpwg6c/8vTenQcviH6IV0KLYVBB3i4sceIXxM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763635415; c=relaxed/simple; bh=9f/U412E7kdzklQJjAr7BwVvqYGEsPTODqZz6a3+sCc=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=bN5OK+PxAdzwfosrExeH5zjEPG6uTJXse7ifsBongRf9M+M3jw9GsnWgGhSmo9klpJqy5aWNv9ry/tKctz2wHDsQRAIi87OdUr/F79FzmrbLD49YzA4y6QRrZ7YMyg4kN3sxnz66DiyP0RPOOjDR5E46O+SfY0DOLeBU16WKZcI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=XhY/1+/7; 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="XhY/1+/7" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8BC57C4CEF1; Thu, 20 Nov 2025 10:43:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1763635414; bh=9f/U412E7kdzklQJjAr7BwVvqYGEsPTODqZz6a3+sCc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=XhY/1+/7NMweMVA4ro3CHHHMBZW3+zgGPrU9U6q/UXgeNgQ1lb+HFlvax8lV4u5gN bZKq3oMIVzfvefQrS7UDhxdgV3tNcahf8dVB5chaBxy+JVFulhZJfbV61r7y6DVZcV GHxxHeo2XmOVLRBV9pYqlwrVU4M1x7SeJhaTKE5SSSmKsAyp2JFN0zJLGctnqO1Pqb /FNYGbxoqeZTJUR7BcfDHNQhL09orBJHg/eln6I5jzoFDhFL//iOOjEal/8tk+jd2z b1ihrOs6DhNr9SEBm5QFnPiBamlJeII9RsXepsgi7pW7AGN0rulIbmYwiyEjvphNP/ L3q1Lg5kG+Xzg== Message-ID: <8fb76ed2-92e1-4e33-94ba-c1e4a21ec316@kernel.org> Date: Thu, 20 Nov 2025 11:43:26 +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> <3c332dea-ea60-4939-9843-ac7d3068c7c8@kernel.org> From: "David Hildenbrand (Red Hat)" Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 11/20/25 11:35, Balbir Singh wrote: > On 11/20/25 20:32, David Hildenbrand (Red Hat) wrote: >> 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 >> >> > > Agreed, I would like to let this patch proceed and then immediately follow up patch > along the lines of CONFIG_SHMEM as separate independent patch (independent of this > series). What do you think? Let's do it properly right away, no need to hurry that much. -- Cheers David