From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 81C0F128839 for ; Mon, 26 Feb 2024 13:46:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708955187; cv=none; b=uxLYA8v6neRmpU+bjqEYBuPakbIeT3GZgootpynSvhoECWcWZKYrQp74mDBy6zIuCapEA1J8BNZsq+/3qAnkdK2shXUQGc98vXJBbwkRWBFMbIGou/BX15WQ0ZQ7/6nBAUxAABOKlFoRE+sWbNYURwOmAXnkei7TLp2Uiw94M1I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708955187; c=relaxed/simple; bh=x0tKBMBvabDged48OlDNnAqjAxWwHlmzi+vREigK+sU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=JyKcc0XraiVke5+hdTwhFkMyCJftpmvOXiXCYDkMAmGRW/aayzIa/q0WPjcpF4zrhplsQD/3qcuWG4JcyLNMNQOggRGrgwgZ+E3cyD9kryfaR2z27gJNbgm/Gn1jISnLdGyZ9KienfpNwDP+FOeM2cqEseMg80XEX1Z9u8L1kPA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 85F2EDA7; Mon, 26 Feb 2024 05:47:03 -0800 (PST) Received: from [10.57.67.4] (unknown [10.57.67.4]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 833013F762; Mon, 26 Feb 2024 05:46:22 -0800 (PST) Message-ID: <71fa4302-2df6-4e55-a5a8-7609476c41d4@arm.com> Date: Mon, 26 Feb 2024 13:46:20 +0000 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] madvise:madvise_cold_or_pageout_pte_range(): allow split while folio_estimated_sharers = 0 Content-Language: en-GB To: Barry Song <21cnbao@gmail.com>, akpm@linux-foundation.org, linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org, Barry Song , Yin Fengwei , Yu Zhao , David Hildenbrand , Kefeng Wang , Matthew Wilcox , Minchan Kim , Vishal Moola , Yang Shi References: <20240221085036.105621-1-21cnbao@gmail.com> From: Ryan Roberts In-Reply-To: <20240221085036.105621-1-21cnbao@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 21/02/2024 08:50, Barry Song wrote: > From: Barry Song > > The purpose is stopping splitting large folios whose mapcount are 2 or > above. Folios whose estimated_shares = 0 should be still perfect and > even better candidates than estimated_shares = 1. > > Consider a pte-mapped large folio with 16 subpages, if we unmap 1-15, > the current code will split folios and reclaim them while madvise goes > on this folio; but if we unmap subpage 0, we will keep this folio and > break. This is weird. > > For pmd-mapped large folios, we can still use "= 1" as the condition > as anyway we have the entire map for it. So this patch doesn't change > the condition for pmd-mapped large folios. > This also explains why we had been using "= 1" for both pmd-mapped and > pte-mapped large folios before commit 07e8c82b5eff ("madvise: convert > madvise_cold_or_pageout_pte_range() to use folios"), because in the > past, we used the mapcount of the specific subpage, since the subpage > had pte present, its mapcount wouldn't be 0. > > The problem can be quite easily reproduced by writing a small program, > unmapping the first subpage of a pte-mapped large folio vs. unmapping > anyone other than the first subpage. > > Fixes: 2f406263e3e9 ("madvise:madvise_cold_or_pageout_pte_range(): don't use mapcount() against large folio for sharing check") > Cc: Yin Fengwei > Cc: Yu Zhao > Cc: Ryan Roberts > Cc: David Hildenbrand > Cc: Kefeng Wang > Cc: Matthew Wilcox > Cc: Minchan Kim > Cc: Vishal Moola (Oracle) > Cc: Yang Shi > Signed-off-by: Barry Song > --- > mm/madvise.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index cfa5e7288261..abde3edb04f0 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -453,7 +453,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, > if (folio_test_large(folio)) { > int err; > > - if (folio_estimated_sharers(folio) != 1) > + if (folio_estimated_sharers(folio) > 1) > break; > if (pageout_anon_only_filter && !folio_test_anon(folio)) > break; I wonder if we should change all the instances: folio_estimated_sharers() != 1 -> folio_estimated_sharers() > 1 folio_estimated_sharers() == 1 -> folio_estimated_sharers() <= 1 It shouldn't cause a problem for the pmd case, and there are definitely other cases where it will help. e.g. madvise_free_pte_range(). Regardless: Reviewed-by: Ryan Roberts