Re: [PATCH v2] mm/migrate: put dest folio on deferred split list if source was there.

[Zi Yan]

On 12 Mar 2024, at 14:46, Matthew Wilcox wrote:

> On Tue, Mar 12, 2024 at 02:32:43PM -0400, Zi Yan wrote:
>> On 12 Mar 2024, at 12:38, Matthew Wilcox wrote:
>>> Folios with a positive refcount are
>>> removed from the per-node or per-cgroup list _at which point there is
>>> an undocumented assumption_ that they will not be removed from the
>>> local list because they have a positive refcount.
>>
>> But that sounds very subtle if not broken. As an outsider of
>
> I merely deduced this requirement; I didn't come up with it ...
>
>> deferred_split_scan(), only !list_empty(folio->_deferred_list) is checked.
>> The condition can be true if the folio is on split_queue or
>> local list of deferred_split_scan() with elevated refcount. In that case,
>> the folio cannot be removed from the list (either split_queue or local list)
>> even if split_queue_lock is held, since local list manipulation is not under
>> split_queue_lock. This makes _deferred_list a one-way train to anyone
>> except deferred_split_scan(), namely folios can only be added into
>> _deferred_list until they are freed or split by deferred_split_scan().
>>
>> Is that intended? If yes, maybe we should document it. If not, using
>> split_queue_lock to protect local list, or more explicitly folio->_deferred_list
>> might be better?
>
> To be fair, the folio can be split by anybody as
> split_huge_page_to_list_to_order() is careful to only manipulate the
> deferred list while the refcount is frozen at 0.  I'm still trying to
> figure out where to document this behaviour of the deferred list that
> someone (for example, your good self) would actually see it.

Yes, split is not affected. For migration, hypothetically if a folio on
deferred_list wants to be migrated without split, it needs to removed
and the dst folio needs to be added during frozen phase. That seems
to be very heavy operation to just get a folio out of deferred_list.
But we can change it when we want to do that in other scenarios.

BTW, s/_deferred_list/_deferred_list_only_removable_during_scan/ works
best for me in terms of documentation. ;)


--
Best Regards,
Yan, Zi

Attachment: signature.asc
Description: OpenPGP digital signature