From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lgeamrelo07.lge.com (lgeamrelo07.lge.com [156.147.51.103]) (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 A59A223C4E9 for ; Fri, 21 Nov 2025 06:58:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.147.51.103 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763708298; cv=none; b=a2j9RYBjfL/OqWDi2SlMOimXbAMlbs3kBhMAtY0fw7eEUXV0N0G6Tj3vJqPanZwKbCmWGX8HYIev8ejprfWlX93atA5ZhlJyAYHEHi/3OKpS/lswhkzqPpdq3lVBWXUKLQLW6/j3fGlIxzAACY56OTL3q1WS4Om6qd6BYGTgzYk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763708298; c=relaxed/simple; bh=4vADosAv5pw9XjkLO1unNLwDgejL1dVwLEQyUTnylz4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Opk2XTNylps5ec+wfXRL9pkAJ8l2bJ22ufKSytwcAMRHRTZOPgtQCs1EFPZCUcYGyhsqzb9KMxpTyMaFCNr+ku8SGtSo31CHlftr98xuCP6d315HzZyVOEkKezTQe3hMD6C9W+9rO/FpnbIwCRFeqmg3HlTRweE/oYF57IFj9ls= 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.103 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.103 with ESMTP; 21 Nov 2025 15:58:13 +0900 X-Original-SENDERIP: 10.177.112.156 X-Original-MAILFROM: youngjun.park@lge.com Date: Fri, 21 Nov 2025 15:58:13 +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 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: On Thu, Nov 20, 2025 at 11:32:37PM +0800, Kairui Song wrote: ... > > > 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'? > > You mean the `offset <= end` check below? That's fine because the for > loops starts with `end -= nr_pages`. > That's right! I missed that. Thanks for the clarification > > > > > 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? > > That's a good suggestion indeed, I'll try to make the code cleaner > this way. Thanks! Great~ I look forward to seeing the updated version. Thanks for considering the suggestion. Youngjun Park