From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-177.mta0.migadu.com (out-177.mta0.migadu.com [91.218.175.177]) (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 C6337364942 for ; Fri, 7 Aug 2026 09:52:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786096353; cv=none; b=L1MjhYmkmqcfG9VpOVMW1wZ4puqC3fiw7BxgBmSJ/jZHGpLtqUqGlqKMxvfP3B7m9mf+0sTeSc2S0yPDtwcw0e8sAD//1GKSqkN4m/kQe1pVVaLv6AJy0BlyA9mW2g7894BhatbwMb2ICICHD55Mn8o6cLgzZoteBBNYfb2gI2g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786096353; c=relaxed/simple; bh=7DsSFdAEAnLMmbMkUJz+dgLkwcHKGKDz91+XhwEwvS8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PvWfm8rOBsy4KY6Du9xaYC/xbI5G9N5fmr8ONY+LSyCnB9aomYX8R1fMwP1+ChFqPZaeXz9CUkp0f2st++38CKYfnCWcl2z4xhouWXTa/Ar3krxoqDhMRqkKnVY2Dwgv8bHpTGByiOXPSjN22U3Uz/Zz1qGlyQz8DEMErL/H6lc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=GMd/eMdQ; arc=none smtp.client-ip=91.218.175.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="GMd/eMdQ" Date: Fri, 7 Aug 2026 17:52:20 +0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1786096349; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=fXDhYioTwAOUajrsTQB8n+o6j0Q1jbDcqbcrYXrO0QY=; b=GMd/eMdQNlAHrlTtKAsrYqWIjkMNNrOBQjDFDON52EqRUIqZrUNU8SRBWv/ocgmciNiYAD vP5Qe1+ODXgwvb1a97Eb94bPGNMJ3tkTD5vC0X1KpBsNOZH2Hxi95ongXzCZCYpAYKnGMR xvkPTcYVKfUIS0NQOGSQwrRZfPvn89k= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Baoquan He To: Youngjun Park Cc: Andrew Morton , Chris Li , Kairui Song , Kemeng Shi , Nhat Pham , Barry Song , her0gyugyu@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 2/2] mm/swap: scan by cluster in find_next_to_unuse() Message-ID: References: <20260806193228.458685-1-youngjun.park@lge.com> <20260806193228.458685-3-youngjun.park@lge.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: <20260806193228.458685-3-youngjun.park@lge.com> X-Migadu-Flow: FLOW_OUT On 08/07/26 at 04:32am, Youngjun Park wrote: > find_next_to_unuse() walks every offset from 0 to si->max, and swapoff > restarts that walk on each retry, so the cost scales with the size of > the device rather than with the few slots the shmem and mmlist passes > could not free. It has caused stalls before. > > The flat walk predates the swap table. Slot state now lives in a per > cluster table, and wait_for_allocation() stops all allocation before > try_to_unuse() runs, so a cluster that holds no slot in use stays that > way. Skip such a cluster instead of reading all of its entries. > > Commit dc644a073769 ("mm: add three more cond_resched() in swapoff") > answered those stalls with a cond_resched() every 256 offsets. A walk > bounded by one cluster no longer needs that counter. The loop now runs > at most SWAPFILE_CLUSTER times before it returns or reschedules, the > same bound swap_reclaim_full_clusters() already scans between > cond_resched() calls. > > The scan end is clamped to si->max, so the walk stops there rather than > running into the masked tail of the last cluster. > > ci->count is read without ci->lock, so READ_ONCE() marks the read for > KCSAN. Allocation is already stopped, so the count can only drop, and a > slot stops being counted only after its folio has left the swap cache. > An empty cluster therefore holds nothing for try_to_unuse() to act on. > > Signed-off-by: Youngjun Park > Reviewed-by: Barry Song > --- > mm/swapfile.c | 43 ++++++++++++++++++++++++++++++------------- > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index dea2d3b36e06..0d24efd32eb0 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -370,8 +370,6 @@ static void discard_swap_cluster(struct swap_info_struct *si, > } > } > > -#define LATENCY_LIMIT 256 > - > static inline bool cluster_is_empty(struct swap_cluster_info *info) > { > return info->count == 0; > @@ -2763,7 +2761,9 @@ static int unuse_mm(struct mm_struct *mm, unsigned int type) > static unsigned int find_next_to_unuse(struct swap_info_struct *si, > unsigned int prev) > { > - unsigned int i; > + struct swap_cluster_info *ci; > + unsigned long i, end; > + unsigned int ci_off; > unsigned long swp_tb; > > /* > @@ -2772,19 +2772,36 @@ static unsigned int find_next_to_unuse(struct swap_info_struct *si, > * hits are okay, and sys_swapoff() has already prevented new > * allocations from this area (while holding swap_lock). > */ > - for (i = prev + 1; i < si->max; i++) { > - swp_tb = swap_table_get(__swap_offset_to_cluster(si, i), > - i % SWAPFILE_CLUSTER); > - if (!swp_tb_is_null(swp_tb) && !swp_tb_is_bad(swp_tb)) > - break; > - if ((i % LATENCY_LIMIT) == 0) > + i = prev + 1; > + while (i < si->max) { > + ci = __swap_offset_to_cluster(si, i); > + end = min_t(unsigned long, > + ALIGN_DOWN(i, SWAPFILE_CLUSTER) + SWAPFILE_CLUSTER, > + si->max); > + > + /* > + * An empty cluster has no slot in use, so skip it whole. > + * A slot is uncounted only after its folio left the swap > + * cache, so there is nothing here for try_to_unuse() to act on. > + * Count only drops here, so a READ_ONCE() without ci->lock is > + * enough, unlike in every other cluster_is_empty() caller. > + */ > + if (!READ_ONCE(ci->count)) { > + i = end; > cond_resched(); > - } > + continue; > + } > > - if (i == si->max) > - i = 0; > + ci_off = i % SWAPFILE_CLUSTER; > + for (; i < end; ci_off++, i++) { > + swp_tb = swap_table_get(ci, ci_off); > + if (!swp_tb_is_null(swp_tb) && !swp_tb_is_bad(swp_tb)) > + return i; I would remove ci_off to save one local variable, but it's only personal preference, not strong opinion. for (; i < end; i++) { swp_tb = swap_table_get(ci, i % SWAPFILE_CLUSTER); ... } Other than the nitpick, this is a great optimization patch. Reviewed-by: Baoquan He > + } > + cond_resched(); > + } > > - return i; > + return 0; > } > > static int try_to_unuse(unsigned int type) > -- > 2.48.1 > >