Re: [PATCH v3 10/18] mm: Allow non-hugetlb large folios to be batch processed

[Ryan Roberts <ryan.roberts@arm.com>]

On 09/03/2024 07:59, Ryan Roberts wrote:
> On 09/03/2024 06:09, Matthew Wilcox wrote:
>> On Fri, Mar 08, 2024 at 11:44:35AM +0000, Ryan Roberts wrote:
>>>> The thought occurs that we don't need to take the folios off the list.
>>>> I don't know that will fix anything, but this will fix your "running out
>>>> of memory" problem -- I forgot to drop the reference if folio_trylock()
>>>> failed.  Of course, I can't call folio_put() inside the lock, so may
>>>> as well move the trylock back to the second loop.
>>
>> I think this was a bad thought ...
> 
> The not-taking-folios-off-the-list thought? Yes, agreed.
> 
>>
>>> Dumping all the CPU back traces with gdb, all the cores (except one) are
>>> contending on the the deferred split lock.
>>
>> I'm pretty sure that we can call the shrinker on multiple CPUs at the
>> same time (can you confirm from the backtrace?)
> 
> Yes, the vast majority of the CPUs were in deferred_split_scan() waiting for the
> split_queue_lock.
> 
>>
>>         struct pglist_data *pgdata = NODE_DATA(sc->nid);
>>         struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>>
>> so if two CPUs try to shrink the same node, they're going to try to
>> process the same set of folios.  Which means the split will keep failing
>> because each of them will have a refcount on the folio, and ... yeah.
> 
> Ahh, ouch. So this probably explains why things started going slow for me again
> last night.
> 
>>
>> If so, we need to take the folios off the list (or otherwise mark them)
>> so that they can't be processed by more than one CPU at a time.  And
>> that leads me to this patch (yes, folio_prep_large_rmappable() is
>> now vestigial, but removing it increases the churn a bit much for this
>> stage of debugging)
> 
> Looks sensible on first review. I'll do some testing now to see if I can
> re-triger the non-NULL mapping issue. Will get back to you in the next couple of
> hours.
> 
>>
>> This time I've boot-tested it.  I'm running my usual test-suite against
>> it now with little expectation that it will trigger.  If I have time
>> I'll try to recreate your setup.
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index fd745bcc97ff..2ca033a6c3d8 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -792,8 +792,6 @@ void folio_prep_large_rmappable(struct folio *folio)
>>  {
>>  	if (!folio || !folio_test_large(folio))
>>  		return;
>> -	if (folio_order(folio) > 1)
>> -		INIT_LIST_HEAD(&folio->_deferred_list);
>>  	folio_set_large_rmappable(folio);
>>  }
>>  
>> @@ -3312,7 +3310,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>>  	struct pglist_data *pgdata = NODE_DATA(sc->nid);
>>  	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>>  	unsigned long flags;
>> -	LIST_HEAD(list);
>> +	struct folio_batch batch;
>>  	struct folio *folio, *next;
>>  	int split = 0;
>>  
>> @@ -3321,36 +3319,40 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>>  		ds_queue = &sc->memcg->deferred_split_queue;
>>  #endif
>>  
>> +	folio_batch_init(&batch);
>>  	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>> -	/* Take pin on all head pages to avoid freeing them under us */
>> +	/* Take ref on all folios to avoid freeing them under us */
>>  	list_for_each_entry_safe(folio, next, &ds_queue->split_queue,
>>  							_deferred_list) {
>> -		if (folio_try_get(folio)) {
>> -			list_move(&folio->_deferred_list, &list);
>> -		} else {
>> +		list_del_init(&folio->_deferred_list);
>> +		sc->nr_to_scan--;
>> +		if (!folio_try_get(folio)) {
>>  			/* We lost race with folio_put() */
>> -			list_del_init(&folio->_deferred_list);
>>  			ds_queue->split_queue_len--;

I think split_queue_len is getting out of sync with the number of items on the
queue? We only decrement it if we lost the race with folio_put(). But we are
unconditionally taking folios off the list here. So we are definitely out of
sync until we take the lock again below. But we only put folios back on the list
that failed to split. A successful split used to decrement this variable
(because the folio was on _a_ list). But now it doesn't. So we are always
mismatched after the first failed split?

I'll fix this up before I start testing.

>> +		} else if (folio_batch_add(&batch, folio) == 0) {
>> +			break;
>>  		}
>> -		if (!--sc->nr_to_scan)
>> +		if (!sc->nr_to_scan)
>>  			break;
>>  	}
>>  	spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
>>  
>> -	list_for_each_entry_safe(folio, next, &list, _deferred_list) {
>> +	while ((folio = folio_batch_next(&batch)) != NULL) {
>>  		if (!folio_trylock(folio))
>> -			goto next;
>> -		/* split_huge_page() removes page from list on success */
>> +			continue;
>>  		if (!split_folio(folio))
>>  			split++;
>>  		folio_unlock(folio);
>> -next:
>> -		folio_put(folio);
>>  	}
>>  
>>  	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>> -	list_splice_tail(&list, &ds_queue->split_queue);
>> +	while ((folio = folio_batch_next(&batch)) != NULL) {
>> +		if (!folio_test_large(folio))
>> +			continue;
>> +		list_add_tail(&folio->_deferred_list, &ds_queue->split_queue);
>> +	}
>>  	spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
>> +	folios_put(&batch);
>>  
>>  	/*
>>  	 * Stop shrinker if we didn't split any page, but the queue is empty.
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 1dfdc3bde1b0..14c21d06f233 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -432,6 +432,8 @@ static inline void prep_compound_head(struct page *page, unsigned int order)
>>  	atomic_set(&folio->_entire_mapcount, -1);
>>  	atomic_set(&folio->_nr_pages_mapped, 0);
>>  	atomic_set(&folio->_pincount, 0);
>> +	if (order > 1)
>> +		INIT_LIST_HEAD(&folio->_deferred_list);
>>  }
>>  
>>  static inline void prep_compound_tail(struct page *head, int tail_idx)
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 025ad1a7df7b..fc9c7ca24c4c 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1007,9 +1007,12 @@ static int free_tail_page_prepare(struct page *head_page, struct page *page)
>>  		break;
>>  	case 2:
>>  		/*
>> -		 * the second tail page: ->mapping is
>> -		 * deferred_list.next -- ignore value.
>> +		 * the second tail page: ->mapping is deferred_list.next
>>  		 */
>> +		if (unlikely(!list_empty(&folio->_deferred_list))) {
>> +			bad_page(page, "still on deferred list");
>> +			goto out;
>> +		}
>>  		break;
>>  	default:
>>  		if (page->mapping != TAIL_MAPPING) {
>>
>