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 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--;
> +		} 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) {
>