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 4EC092F6180 for ; Thu, 20 Nov 2025 09:09:21 +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=1763629762; cv=none; b=lVvzbW2qKQwl7PU63LP/618XUbBjV1Mqb3tFa7aON3BdtYUDzZUj5xMJet2Lv5gBgoKY5qiQf3nNMhVrYuNk4t8fzBRIDey2zPItgzzxoKMFMBBzzEOLCPywGovjO9015Zip/Z3j6Yyfif0CWluHmm23wM3++VVdp7BpRB5o04A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763629762; c=relaxed/simple; bh=3LRBNuFqcIWNv6+llg0BKBD85RcO8c2uV+3xfhb2i7E=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=RbzJRHSD0VkwhXD/B33AoEYRloLRExlhblfm0F4QIQ+zQPZJCuJ4ri8RuEgDuyW62YdrhAMvzUBN+b00GgR0YadW3/Tto5E8+Y7/kSdTkXcUuqWFUA+5CJWil09pFhX8BFcHTxAwPQJHlAYGifr/3WdiozJOi4vDbD9O1iTQ14E= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=YFgfR4YE; 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="YFgfR4YE" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 71202C116B1; Thu, 20 Nov 2025 09:09:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1763629760; bh=3LRBNuFqcIWNv6+llg0BKBD85RcO8c2uV+3xfhb2i7E=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=YFgfR4YEhqqRgIYW5R3th1AbIc7N4bMKYuIyuo1OQ6cSObvbe8AQcGnngD39IwZdr ++v65KV1XOH9vDz4/dcQ/BPGftq92Q1iHqergUHm66NR3WzzIuWKPJyau97h29YT5z N981+SHwilrL7xh8KcpDzkcM2lHZ+HWwnz9Jcp9KTn9Cqas7AKLACuhs/W3IXUeS/t ji75XIawgZATeYoXZVyT3RP4lmzWOyx8moZ0jZRxAKY+Dcuwj+9F25LTDd/uQ6k+Mu r63xXelNelaLHfk+75Fl8thTejwKggktdAEU3rQ/gXWAA4ygAbM/UeaREjq/g1fj0V yNyTSc7Ygu6RQ== Message-ID: <4b87bd52-5582-4599-8a97-38843e27d0a7@kernel.org> Date: Thu, 20 Nov 2025 10:09:11 +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> From: "David Hildenbrand (Red Hat)" Content-Language: en-US In-Reply-To: <20251120030709.2933665-1-balbirs@nvidia.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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" > /* 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() > > if (!ret && is_anon && !folio_is_device_private(folio)) > @@ -4092,16 +4093,27 @@ static int __folio_split(struct folio *folio, unsigned int new_order, > return ret; > } > > -/* > - * This function is a helper for splitting folios that have already been unmapped. > - * The use case is that the device or the CPU can refuse to migrate THP pages in > - * the middle of migration, due to allocation issues on either side > +/** > + * folio_split_unmapped() - split a large anon folio that is already unmapped > + * @folio: folio to split > + * @new_order: the order of folios after split > + * > + * This function is a helper for splitting folios that have already been > + * unmapped. The use case is that the device or the CPU can refuse to migrate > + * THP pages in the middle of migration, due to allocation issues on either > + * side. > + * > + * anon_vma_lock is not required to be held, mmap_read_lock() or > + * mmap_write_lock() should be held. @folio is expected to be locked by the > + * caller. device-private and non device-private folios are supported along > + * with folios that are in the swapcache. @folio should also be unmapped and > + * isolated from LRU (if applicable) > * > - * The high level code is copied from __folio_split, since the pages are anonymous > - * and are already isolated from the LRU, the code has been simplified to not > - * burden __folio_split with unmapped sprinkled into the code. > + * Upon return, the folio is not remapped, split folios are not added to LRU, > + * free_folio_and_swap_cache() is not called, and new folios remain locked. > * > - * None of the split folios are unlocked > + * Return: 0 on success, -EAGAIN if the folio cannot be split (e.g., due to > + * insufficient reference count or extra pins). Sounds much better to me, thanks. -- Cheers David