From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lgeamrelo03.lge.com (lgeamrelo03.lge.com [156.147.51.102]) (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 AA84E21B9FD for ; Thu, 20 Nov 2025 06:47:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.147.51.102 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763621278; cv=none; b=iNJXL9ysrdFj7AGdro25FIADB1em7nSLTaz/paFxF/o6X80XwQMVU/1K4iBd8EVQ3/mE6gfxJbAFm8Kf2JTjWBtWrSvASYmW5lN7IFk/+og39Dox0zpVXowQBaY5AUjXN3Vhwm6gnj/Ab1FzNEaHKYUyQXFVq7opcKSBGB5D6IM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763621278; c=relaxed/simple; bh=n3mccwcN3JMNICaWkSueZzhisN1wLbe2P/kg6k9tLZY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Jil3U0ZaR7MUvOKcn8w6AScg9R5RwAv5g7F9wWgl7uiiYaIkoJakV3K4V0PYFvY5obbxqOHVQ9Z0TuIEIJBjfseM2bWHTYbY7yVC58U/H3FyP+ahk/zDWcTLaGy1A94YwWTFqFkoN8ErcMphK8VkSw53zND85AYk3q0nEmt9tNs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lge.com; spf=pass smtp.mailfrom=lge.com; arc=none smtp.client-ip=156.147.51.102 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lge.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lge.com Received: from unknown (HELO yjaykim-PowerEdge-T330) (10.177.112.156) by 156.147.51.102 with ESMTP; 20 Nov 2025 15:47:47 +0900 X-Original-SENDERIP: 10.177.112.156 X-Original-MAILFROM: youngjun.park@lge.com Date: Thu, 20 Nov 2025 15:47:47 +0900 From: YoungJun Park To: Kairui Song Cc: linux-mm@kvack.org, Andrew Morton , Baoquan He , Barry Song , Chris Li , Nhat Pham , Yosry Ahmed , David Hildenbrand , Johannes Weiner , Hugh Dickins , Baolin Wang , Ying Huang , Kemeng Shi , Lorenzo Stoakes , "Matthew Wilcox (Oracle)" , linux-kernel@vger.kernel.org, Kairui Song Subject: Re: [PATCH v2 10/19] mm, swap: consolidate cluster reclaim and check logic Message-ID: References: <20251117-swap-table-p2-v2-0-37730e6ea6d5@tencent.com> <20251117-swap-table-p2-v2-10-37730e6ea6d5@tencent.com> 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: <20251117-swap-table-p2-v2-10-37730e6ea6d5@tencent.com> On Mon, Nov 17, 2025 at 02:11:51AM +0800, Kairui Song wrote: > From: Kairui Song > > Swap cluster cache reclaim requires releasing the lock, so some extra > checks are needed after the reclaim. To prepare for checking swap cache > using the swap table directly, consolidate the swap cluster reclaim and > check the logic. > > Also, adjust it very slightly. By moving the cluster empty and usable > check into the reclaim helper, it will avoid a redundant scan of the > slots if the cluster is empty. > > And always scan the whole region during reclaim, don't skip slots > covered by a reclaimed folio. Because the reclaim is lockless, it's > possible that new cache lands at any time. And for allocation, we want > all caches to be reclaimed to avoid fragmentation. And besides, if the > scan offset is not aligned with the size of the reclaimed folio, we are > skipping some existing caches. > > There should be no observable behavior change, which might slightly > improve the fragmentation issue or performance. > > Signed-off-by: Kairui Song > --- > mm/swapfile.c | 45 +++++++++++++++++++++++---------------------- > 1 file changed, 23 insertions(+), 22 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index d2e60734ce8f..d57e83a4d0a7 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -778,42 +778,50 @@ static int swap_cluster_setup_bad_slot(struct swap_cluster_info *cluster_info, > return 0; > } > > -static bool cluster_reclaim_range(struct swap_info_struct *si, > - struct swap_cluster_info *ci, > - unsigned long start, unsigned long end) > +static unsigned int cluster_reclaim_range(struct swap_info_struct *si, > + struct swap_cluster_info *ci, > + unsigned long start, unsigned int order) > { > + unsigned int nr_pages = 1 << order; > + unsigned long offset = start, end = start + nr_pages; > unsigned char *map = si->swap_map; > - unsigned long offset = start; > int nr_reclaim; > > spin_unlock(&ci->lock); > do { > switch (READ_ONCE(map[offset])) { > case 0: > - offset++; > break; > case SWAP_HAS_CACHE: > nr_reclaim = __try_to_reclaim_swap(si, offset, TTRS_ANYWAY); > - if (nr_reclaim > 0) > - offset += nr_reclaim; > - else > + if (nr_reclaim < 0) > goto out; > break; > default: > goto out; > } > - } while (offset < end); > + } while (++offset < end); > out: > spin_lock(&ci->lock); > + > + /* > + * We just dropped ci->lock so cluster could be used by another > + * order or got freed, check if it's still usable or empty. > + */ > + if (!cluster_is_usable(ci, order)) > + return SWAP_ENTRY_INVALID; > + if (cluster_is_empty(ci)) > + return cluster_offset(si, ci); > + > /* > * Recheck the range no matter reclaim succeeded or not, the slot > * could have been be freed while we are not holding the lock. > */ > for (offset = start; offset < end; offset++) > if (READ_ONCE(map[offset])) > - return false; > + return SWAP_ENTRY_INVALID; > > - return true; > + return start; > } > > static bool cluster_scan_range(struct swap_info_struct *si, > @@ -901,7 +909,7 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si, > unsigned long start = ALIGN_DOWN(offset, SWAPFILE_CLUSTER); > unsigned long end = min(start + SWAPFILE_CLUSTER, si->max); The Original code. I'm wondering if there's an off-by-one error here. Looking at the code below, it seems the design allows the end offset to go through the logic as well. Shouldn't it be 'start + SWAPFILE_CLUSTER - 1' and 'si->max - 1'? > unsigned int nr_pages = 1 << order; > - bool need_reclaim, ret; > + bool need_reclaim; > > lockdep_assert_held(&ci->lock); > > @@ -913,20 +921,13 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si, > if (!cluster_scan_range(si, ci, offset, nr_pages, &need_reclaim)) > continue; > if (need_reclaim) { > - ret = cluster_reclaim_range(si, ci, offset, offset + nr_pages); > - /* > - * Reclaim drops ci->lock and cluster could be used > - * by another order. Not checking flag as off-list > - * cluster has no flag set, and change of list > - * won't cause fragmentation. > - */ > + found = cluster_reclaim_range(si, ci, offset, order); > if (!cluster_is_usable(ci, order)) > goto out; This check resolves the issue I mentioned in my previous review. > - if (cluster_is_empty(ci)) > - offset = start; > /* Reclaim failed but cluster is usable, try next */ > - if (!ret) > + if (!found) > continue; > + offset = found; > } > if (!cluster_alloc_range(si, ci, offset, usage, order)) > break; I think the reason cluster_is_usable() is checked redundantly here is because cluster_reclaim_range() returns an unsigned int (offset), making it impossible to distinguish error values. What if we make offset an output parameter (satisfying the assumption that it can be changed in reclaim_range) and return an error value instead? This would eliminate the redundant cluster_is_usable() check and simplify the logic. Also, the consecutive "offset = found, found = offset" is a bit confusing, and this approach could eliminate that as well. What do you think? Thnaks! Youngjun Park > -- > 2.51.2 > >