On 20 Mar 2024, at 12:02, Matthew Wilcox wrote:
> On Tue, Mar 19, 2024 at 09:45:11PM -0400, Zi Yan wrote:
>> +++ b/mm/migrate.c
>> @@ -1654,25 +1654,65 @@ static int migrate_pages_batch(struct list_head *from,
>>
>> /*
>> * Large folio migration might be unsupported or
>> - * the allocation might be failed so we should retry
>> - * on the same folio with the large folio split
>> + * the folio is on deferred split list so we should
>> + * retry on the same folio with the large folio split
>> * to normal folios.
>> *
>> * Split folios are put in split_folios, and
>> * we will migrate them after the rest of the
>> * list is processed.
>> */
>> - if (!thp_migration_supported() && is_thp) {
>> - nr_failed++;
>> - stats->nr_thp_failed++;
>> - if (!try_split_folio(folio, split_folios)) {
>> - stats->nr_thp_split++;
>> - stats->nr_split++;
>> + if (is_thp) {
>> + bool is_on_deferred_list = false;
>> +
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> + /*
>> + * Check without taking split_queue_lock to
>> + * reduce locking overheads. The worst case is
>> + * that if the folio is put on the deferred
>> + * split list after the check, it will be
>> + * migrated and not put back on the list.
>> + * The migrated folio will not be split
>> + * via shrinker during memory pressure.
>> + */
>> + if (!data_race(list_empty(&folio->_deferred_list))) {
>> + struct deferred_split *ds_queue;
>> + unsigned long flags;
>> +
>> + ds_queue =
>> + get_deferred_split_queue(folio);
>> + spin_lock_irqsave(&ds_queue->split_queue_lock,
>> + flags);
>> + /*
>> + * Only check if the folio is on
>> + * deferred split list without removing
>> + * it. Since the folio can be on
>> + * deferred_split_scan() local list and
>> + * removing it can cause the local list
>> + * corruption. Folio split process
>> + * below can handle it with the help of
>> + * folio_ref_freeze().
>> + */
>> + is_on_deferred_list =
>> + !list_empty(&folio->_deferred_list);
>> + spin_unlock_irqrestore(&ds_queue->split_queue_lock,
>> + flags);
>> + }
>> +#endif
>> + if (!thp_migration_supported() ||
>> + is_on_deferred_list) {
>> + nr_failed++;
>> + stats->nr_thp_failed++;
>> + if (!try_split_folio(folio,
>> + split_folios)) {
>> + stats->nr_thp_split++;
>> + stats->nr_split++;
>> + continue;
>> + }
>> + stats->nr_failed_pages += nr_pages;
>> + list_move_tail(&folio->lru, ret_folios);
>> continue;
>> }
>> - stats->nr_failed_pages += nr_pages;
>> - list_move_tail(&folio->lru, ret_folios);
>> - continue;
>> }
>
> I don't think we need to try quite this hard. I don't think we need
> to take the lock to be certain if it's on the deferred list -- is
> there anything preventing the folio being added to the deferred list
> after we drop the lock?
No. OK, I will use the less hard version.
>
> I also don't think we should account this as a thp split since those
> are treated by callers as failures. So maybe this?
I think we need to match the stats with code behavior, otherwise userspace
caller can get confused by the results, where only a subset of a folio
is migrated and split stats and failure stats are not bumped accordingly.
>
> +++ b/mm/migrate.c
> @@ -1652,6 +1652,17 @@ static int migrate_pages_batch(struct list_head *from,
>
> cond_resched();
>
> + /*
> + * The rare folio on the deferred split list should
> + * be split now. It should not count as a failure.
> + */
> + if (nr_pages > 2 &&
> + !list_empty(&folio->_deferred_list)) {
> + if (try_split_folio(folio, from) == 0) {
> + is_large = is_thp = false;
> + nr_pages = 1;
> + }
> + }
> /*
> * Large folio migration might be unsupported or
> * the allocation might be failed so we should retry
--
Best Regards,
Yan, Zi
Attachment:
signature.asc
Description: OpenPGP digital signature
