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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/7/7 10:11, Andrew Morton wrote:
> On Sat, 6 Jul 2024 14:29:00 -0700 (PDT) Hugh Dickins <hughd@google.com> 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.

Maybe some helper to check whether or not we should unqueue the
defferred_list, but it out of scope in this patch, and maybe not worth it

>>
>> 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.

OK, let's keep it simple, thank your for pushing it out.
> 
> 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.