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

[Matthew Wilcox <willy@infradead.org>]

On Fri, Mar 08, 2024 at 12:09:38PM +0000, Ryan Roberts wrote:
> On 08/03/2024 11:44, Ryan Roberts wrote:
> > Dumping all the CPU back traces with gdb, all the cores (except one) are
> > contending on the the deferred split lock.
> > 
> > A couple of thoughts:
> > 
> >  - Since we are now taking a maximum of 15 folios into a batch,
> > deferred_split_scan() is called much more often (in a tight loop from
> > do_shrink_slab()). Could it be that we are just trying to take the lock so much
> > more often now? I don't think it's quite that simple because we take the lock
> > for every single folio when adding it to the queue, so the dequeing cost should
> > still be a factor of 15 locks less.
> > 
> > - do_shrink_slab() might be calling deferred_split_scan() in a tight loop with
> > deferred_split_scan() returning 0 most of the time. If there are still folios on
> > the deferred split list but deferred_split_scan() was unable to lock any folios
> > then it will return 0, not SHRINK_STOP, so do_shrink_slab() will keep calling
> > it, essentially live locking. Has your patch changed the duration of the folio
> > being locked? I don't think so...
> > 
> > - Ahh, perhaps its as simple as your fix has removed the code that removed the
> > folio from the deferred split queue if it fails to get a reference? That could
> > mean we end up returning 0 instead of SHRINK_STOP too. I'll have play.
> > 
> 
> I tested the last idea by adding this back in:
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index d46897d7ea7f..50b07362923a 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3327,8 +3327,12 @@ static unsigned long deferred_split_scan(struct shrinker
> *shrink,
>         /* 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))
> +               if (!folio_try_get(folio)) {
> +                       /* We lost race with folio_put() */
> +                       list_del_init(&folio->_deferred_list);
> +                       ds_queue->split_queue_len--;
>                         continue;
> +               }
>                 if (folio_batch_add(&batch, folio) == 0) {
>                         --sc->nr_to_scan;
>                         break;
> 
> The test now gets further than where it was previously getting live-locked, but

If the deferred_split_lock contention comes back, we can fix
split_huge_page_to_list() to only take the lock if the page is on the
list.  Right now, it takes it unconditionally which could be avoided.
I'm not going to send a patch just yet to avoid confusion.