From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 89AE3CA52 for ; Wed, 22 Jul 2026 12:50:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784724622; cv=none; b=saOv9T/LzHNS8En6A6IOv6NZ15mjytCjCocV5ORoxq3YMkb89TbxGiAqr3QY6vTHz2p0rqSbpRAUT4DTs+yMRDq8XrrVUJfqNdePCdAsuEQgVH2GZnyVbhBipeRZW05RpI7L+YxMYav3/c4a9beKXeHzBBx7cloflW4kJXvglSs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784724622; c=relaxed/simple; bh=HzzF+p3bSp6xGFXRYCrKfonY+lTQG1v570U8tpxU9c0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=RJEBjqkhKELwKcMQsH8kVqabJRoWWSPpdGqjwCftCjBmHBu6NFklTohS7lwcp6eLcOXt/KZi7dbdt10DesEADmCp5c3YAviC4Qa39ZD0oibWT14L+TBj14sS1xdpO9BDSGqfKPlQFPo2eiPoveec9UknQSp4YtM0mlNH45nF9mU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=km4jXblX; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="km4jXblX" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E2C301F000E9; Wed, 22 Jul 2026 12:50:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1784724621; bh=KeKyLY8p+R3goFIELc8d+Itcavb+BZ5wsH5llzH8q7I=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=km4jXblXtMqHV54MzRwH//7c5eqW9dy71GCxJw1dDeoS5/10TgeXzFE8aWx0SG0Lk l6wyHQpZOltPFilCAvzEdJFssBLo4+ge0lCR2xOee7VQKY1qwJpEBGXr5o5AYnv5wJ 3+DVeiiDhWoq1jt/0CdSprmeHH2pjcSkvvFyM8OsV+VPdF3hFUjXQcNNukURp7Cf2o +sijjxZWn045XB5E7xXD7leQicASinabMIp4TSetHnSKOHq7515XlkpQnA4JthGdlh PANB2lfl6Q7E7SV3LVwT1VByU1IPFPeb2OKvcH4ng+Gzd+yNr97J4ZQKZFRLJEL1Bm 6ZVtIXrHtYCOA== Date: Wed, 22 Jul 2026 13:50:05 +0100 From: "Lorenzo Stoakes (ARM)" To: jiale yao <19888972804@163.com> Cc: Andrew Morton , David Hildenbrand , "Liam R. Howlett" , Vlastimil Babka , Mike Rapoport , Suren Baghdasaryan , Michal Hocko , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm/page_idle: call folio_test_lru() after folio_get() Message-ID: References: <20260722092642.1123347-1-yaojiale02@163.com> <75a79b54.8930.19f898e1ed6.Coremail.19888972804@163.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: <75a79b54.8930.19f898e1ed6.Coremail.19888972804@163.com> Nooooo :) this is not how you send patches. Also you should _reply_ to review comments, not just send another patch with no reply. Comms is king. Also - don't respin so quick. Wait a day. Then send a v2, NOT in reply to anything. Really best way is to use b4, docs at https://b4.docs.kernel.org/en/latest/contributor/prep.html and etc. But you can also do something like: git format-patch -v2 HEAD~1 scripts/checkpatch.pl scripts/get_maintainer.pl git send-email --to="(andrew)" --cc="" On Wed, Jul 22, 2026 at 07:20:20PM +0800, jiale yao wrote: > page_idle_get_folio() speculatively calls folio_test_lru() before > folio_try_get(). The folio can get freed and reallocated to a tail page > in the meantime. In that case, VM_BUG_ON_PGFLAGS() in > const_folio_flags() can be triggered. Remove the speculative call. > > This is a sibling-path bug: damon_get_folio() was copied from this > function with the same flawed pattern. Commit d6b8b02a27b3 > ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed > damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN > (strict mode) confirms the data race on the folio flags: > > BUG: KCSAN: data-race in ... / percpu_counter_add_batch > page_idle_get_folio+0x7a/0x2d0 > page_idle_bitmap_read+0xc9/0x220 > > Signed-off-by: Jiale Yao > Reviewed-by: Lorenzo Stoakes (ARM) this should really be backported, so good to find the right commit to use as a Fixes: here. Also then add Cc: so it gets send for backporting. But _please_ wait a day before sending the v2 :) > --- Also in the v2 put a list of changes here (will not be included in commit msg) with links to previous versions on lore. > mm/page_idle.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/page_idle.c b/mm/page_idle.c > index 9c67cbac2965..b6c26a0ba3d5 100644 > --- a/mm/page_idle.c > +++ b/mm/page_idle.c > @@ -40,7 +40,7 @@ static struct folio *page_idle_get_folio(unsigned long pfn) > return NULL; > > folio = page_folio(page); > - if (!folio_test_lru(folio) || !folio_try_get(folio)) > + if (!folio_try_get(folio)) > return NULL; > if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) { > folio_put(folio); > -- > 2.34.1 > > > > > > At 2026-07-22 17:56:13, "Lorenzo Stoakes (ARM)" wrote: > >On Wed, Jul 22, 2026 at 05:26:42PM +0800, Jiale Yao wrote: > >> page_idle_get_folio() speculatively calls folio_test_lru() before > >> folio_try_get(). The folio can get freed and reallocated to a tail page > >> in the meantime. In that case, VM_BUG_ON_PGFLAGS() in > >> const_folio_flags() can be triggered. Remove the speculative call. > >> > >> Also mark the folio_test_lru() check right after folio_try_get() success > >> as no more unlikely. > > > >Slightly strange wording but not sure why you're doing that? It is > >generally unlikely a given folio will be !LRU right? > > > >> > >> This is a sibling-path bug: damon_get_folio() was copied from this > >> function with the same flawed pattern. Commit d6b8b02a27b3 > >> ("mm/damon/ops-common: call folio_test_lru() after folio_get()") fixed > >> damon_get_folio(), but page_idle_get_folio() was left unfixed. KCSAN > >> (strict mode) confirms the data race on the folio flags: > >> > >> BUG: KCSAN: data-race in ... / percpu_counter_add_batch > >> page_idle_get_folio+0x7a/0x2d0 > >> page_idle_bitmap_read+0xc9/0x220 > >> > >> Signed-off-by: Jiale Yao > > > >Yeah generally this seems obviously correct (TM), if we can't be sure a > >folio is kept around any other way to the extent we're doing > >folio_try_get() we should gate any actual interactions with the folio on > >succeeding the get first...! > > > >With the unlikely thing changed, LGTM so: > > > >Reviewed-by: Lorenzo Stoakes (ARM) > > > >> --- > >> mm/page_idle.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/mm/page_idle.c b/mm/page_idle.c > >> index 9c67cbac2965..29ee18f0e8ce 100644 > >> --- a/mm/page_idle.c > >> +++ b/mm/page_idle.c > >> @@ -40,9 +40,9 @@ static struct folio *page_idle_get_folio(unsigned long pfn) > >> return NULL; > >> > >> folio = page_folio(page); > >> - if (!folio_test_lru(folio) || !folio_try_get(folio)) > > > >I guess this was meant as a racey check... > > > >> + if (!folio_try_get(folio)) > >> return NULL; > >> - if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) { > >> + if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio)) { > > > >As above, not sure why you're changing this? Seems unrelated, I'd just keep > >it as it was. > > > >> folio_put(folio); > >> folio = NULL; > >> } > >> -- > >> 2.34.1 > >> > > > >Cheers, Lorenzo Thanks, Lorenzo