Re: [PATCH hotfix] mm: fix crashes from deferred split racing folio migration

[Andrew Morton]

On Sat, 6 Jul 2024 14:29:00 -0700 (PDT) Hugh Dickins <hughd@xxxxxxxxxx> wrote:

> 
> What you show above is exactly what I had when I was originally testing
> over the top of mm-everything (well, not quite exactly, I don't think I
> bothered with the data_race()). But I grew to feel that probably everyone
> else would be happier with less of those internals _deferred_list and
> __folio_undo_large_rmappable() spread about.
> 
> There are many ways to play it. I had also considered doing it Zi Yan's
> way, freezing always in the !mapping case as well as in the mapping case:
> what overhead it adds would probably get lost amidst all the other overhead
> of page migration. It will not be surprising if changes come later requiring
> us always to freeze in the anon !swapcache case too, it always seemed a bit
> surprising not to need freezing there. But for now I decided it's best to
> keep the freezing to the case where it's known to be needed (but without
> getting into __s).
> 
> Many ways to play it, and I've no objection if someone then changes it
> around later; but we've no need to depart from what Andrew already has.
> 
> Except, he did ask one of us to send along the -fix removing the unnecessary
> checks before its second folio_undo_large_rmappable() once your refactor
> patch goes in: here it is below.

Grabbed, thanks.

> [I guess this is the wrong place to say so, but folio_undo_large_rmappable()
> is a dreadful name: it completely obscures what the function actually does,
> and gives the false impression that the folio would be !large_rmappable
> afterwards. I hope that one day the name gets changed to something like
> folio_unqueue_deferred_split() or folio_cancel_deferred_split().]

Naming is important, but so also is commentary. 
folio_undo_large_rmappable() lacks any.

> [PATCH] mm: refactor folio_undo_large_rmappable() fix
> 
> Now that folio_undo_large_rmappable() is an inline function checking
> order and large_rmappable for itself (and __folio_undo_large_rmappable()
> is now declared even when CONFIG_TRANASPARENT_HUGEPAGE is off) there is
> no need for folio_migrate_mapping() to check large and large_rmappable
> first (in the mapping case when it has had to freeze anyway).
> 
> ...
>
> For folding in to mm-unstable's "mm: refactor folio_undo_large_rmappable()",
> unless I'm too late and it's already mm-stable (no problem, just a cleanup).

Missed the mm-stable mergification by >that much<.  I'll queue it
separately, thanks.