Re: [PATCH hotfix 1/2] mm/thp: fix deferred split queue not partially_mapped

[Hugh Dickins <hughd@google.com>]

On Fri, 25 Oct 2024, Zi Yan wrote:
> 
> Thank you a lot for taking the time to check the locked version. Looking
> forward to the result.

The locked version worked fine (I did see some unusual filesystem on loop
errors on this laptop, but very much doubt that was related to the extra
deferred split queue locking).  But I do still prefer the original version.

> BTW, I am not going to block this patch since it fixes the bug.

Thanks!  I'll put out the same as v2 1/2.

> 
> The tricky part in deferred_list_scan() is always the use of
> folio->_deferred_list without taking split_queue_lock. I am thinking about
> use folio_batch to store the out-of-split_queue folios, so that _deferred_list
> will not be touched when these folios are tried to be split. Basically,
> 
> 1. loop through split_queue and move folios to a folio_batch until the
>    folio_batch is full;
> 2. loop through the folio_batch to try to split each folio;
> 3. move the remaining folios back to split_queue.
> 
> With this approach, split_queue_lock might be taken more if there are
> more than 31 (folio_batch max size) folios on split_queue and split_queue_lock
> will be held longer in step 3, since the remaining folios need to be
> added back to split_queue one by one instead of a single list splice.
> 
> Let me know your thoughts. I can look into this if this approach sounds
> promising. Thanks.

TBH, I just don't see the point: we have a working mechanism, and
complicating it to rely on more infrastructure, just to reach the
same endpoint, seems a waste of developer time to me.  It's not as
if a folio_batch there would make the split handling more efficient.

It would disambiguate the list_empty(), sure; but as it stands,
there's nothing in the handling which needs that disambiguated.

I can half-imagine that a folio_batch might become a better technique,
if we go on to need less restricted use of the deferred split queue:
if for some reason we grow to want to unqueue a THP while it's still
in use (as memcg move wanted in 2/2, but was not worth recoding for).

But I'm not sure whether a folio_batch would actually help there,
and I wouldn't worry about it unless there's a need.

Hugh