linux-kernel - Re: [PATCH RFC 07/28] mm: thp: use folio_batch to handle THP splitting in deferred_split

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <235f2616-99dd-abfa-f6d1-c178d8ffb363@google.com>
Date: Mon, 5 May 2025 23:44:55 -0700 (PDT)
From: Hugh Dickins <hughd@...gle.com>
To: Johannes Weiner <hannes@...xchg.org>, 
    Muchun Song <songmuchun@...edance.com>
cc: mhocko@...nel.org, roman.gushchin@...ux.dev, shakeel.butt@...ux.dev, 
    muchun.song@...ux.dev, akpm@...ux-foundation.org, david@...morbit.com, 
    zhengqi.arch@...edance.com, yosry.ahmed@...ux.dev, nphamcs@...il.com, 
    chengming.zhou@...ux.dev, linux-kernel@...r.kernel.org, 
    cgroups@...r.kernel.org, linux-mm@...ck.org, 
    hamzamahfooz@...ux.microsoft.com, apais@...ux.microsoft.com, 
    Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH RFC 07/28] mm: thp: use folio_batch to handle THP splitting
 in deferred_split_scan()

On Wed, 30 Apr 2025, Johannes Weiner wrote:
> On Tue, Apr 15, 2025 at 10:45:11AM +0800, Muchun Song wrote:
> > The maintenance of the folio->_deferred_list is intricate because it's
> > reused in a local list.
> > 
> > Here are some peculiarities:
> > 
> >    1) When a folio is removed from its split queue and added to a local
> >       on-stack list in deferred_split_scan(), the ->split_queue_len isn't
> >       updated, leading to an inconsistency between it and the actual
> >       number of folios in the split queue.
> > 
> >    2) When the folio is split via split_folio() later, it's removed from
> >       the local list while holding the split queue lock. At this time,
> >       this lock protects the local list, not the split queue.
> > 
> >    3) To handle the race condition with a third-party freeing or migrating
> >       the preceding folio, we must ensure there's always one safe (with
> >       raised refcount) folio before by delaying its folio_put(). More
> >       details can be found in commit e66f3185fa04. It's rather tricky.
> > 
> > We can use the folio_batch infrastructure to handle this clearly. In this
> > case, ->split_queue_len will be consistent with the real number of folios
> > in the split queue. If list_empty(&folio->_deferred_list) returns false,
> > it's clear the folio must be in its split queue (not in a local list
> > anymore).
> > 
> > In the future, we aim to reparent LRU folios during memcg offline to
> > eliminate dying memory cgroups. This patch prepares for using
> > folio_split_queue_lock_irqsave() as folio memcg may change then.
> > 
> > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> 
> This is a very nice simplification. And getting rid of the stack list
> and its subtle implication on all the various current and future
> list_empty(&folio->_deferred_list) checks should be much more robust.
> 
> However, I think there is one snag related to this:
>...
> There IS a list_empty() check in the splitting code that we actually
> relied on, for cleaning up the partially_mapped state and counter:
> 
> 		    !list_empty(&folio->_deferred_list)) {
> 			ds_queue->split_queue_len--;
> 			if (folio_test_partially_mapped(folio)) {
> 				folio_clear_partially_mapped(folio);
> 				mod_mthp_stat(folio_order(folio),
> 					      MTHP_STAT_NR_ANON_PARTIALLY_MAPPED, -1);
> 			}
> 			/*
> 			 * Reinitialize page_deferred_list after removing the
> 			 * page from the split_queue, otherwise a subsequent
> 			 * split will see list corruption when checking the
> 			 * page_deferred_list.
> 			 */
> 			list_del_init(&folio->_deferred_list);
> 
> With the folios isolated up front, it looks like you need to handle
> this from the shrinker.

Good catch.  I loaded up patches 01-07/28 on top of 6.15-rc5 yesterday,
and after a good run of 12 hours on this laptop, indeed I can see
vmstat nr_anon_partially_mapped 78299, whereas it usually ends up at 0.

> 
> Otherwise this looks correct to me. But this code is subtle, I would
> feel much better if Hugh (CC-ed) could take a look as well.

However... I was intending to run it for 12 hours on the workstation,
but after 11 hours and 35 minutes, that crashed with list_del corruption,
kernel BUG at lib/list_debug.c:65! from deferred_split_scan()'s
list_del_init().

I've not yet put together the explanation: I am deeply suspicious of
the change to when list_empty() becomes true (the block Hannes shows
above is not the only such: (__)folio_unqueue_deferred_split() and
migrate_pages_batch() consult it too), but each time I think I have
the explanation, it's ruled out by folio_try_get()'s reference.

And aside from the crash (I don't suppose 6.15-rc5 is responsible,
or that patches 08-28/28 would fix it), I'm not so sure that this
patch is really an improvement (folio reference held for longer, and
list lock taken more often when split fails: maybe not important, but
I'm also not so keen on adding in fbatch myself).  I didn't spend very
long looking through the patches, but maybe this 07/28 is not essential?

Let me try again to work out what's wrong tomorrow,
Hugh