Re: [PATCH RESEND] mm: drop lruvec->lru_lock if contended when skipping folio

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 19 Aug 2024 19:46:48 +0100 Usama Arif <usamaarif642@gmail.com> wrote:

> lruvec->lru_lock is highly contended and is held when calling
> isolate_lru_folios. If the lru has a large number of CMA folios
> consecutively, while the allocation type requested is not MIGRATE_MOVABLE,
> isolate_lru_folios can hold the lock for a very long time while it
> skips those. vmscan_lru_isolate tracepoint showed that skipped can go
> above 70k in production and lockstat shows that waittime-max is x1000
> higher without this patch.
> This can cause lockups [1] and high memory pressure for extended periods of
> time [2]. Hence release the lock if its contended when skipping a folio to
> give other tasks a chance to acquire it and not stall.
> 
> ...
>
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1695,8 +1695,14 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
>  		if (folio_zonenum(folio) > sc->reclaim_idx ||
>  				skip_cma(folio, sc)) {
>  			nr_skipped[folio_zonenum(folio)] += nr_pages;
> -			move_to = &folios_skipped;
> -			goto move;
> +			list_move(&folio->lru, &folios_skipped);
> +			if (!spin_is_contended(&lruvec->lru_lock))
> +				continue;
> +			if (!list_empty(dst))
> +				break;
> +			spin_unlock_irq(&lruvec->lru_lock);
> +			cond_resched();
> +			spin_lock_irq(&lruvec->lru_lock);
>  		}

Oh geeze ugly thing.  Must we do this?

The games that function plays with src, dst and move_to are a bit hard
to follow.  Some tasteful comments explaining what's going on would
help.

Also that test of !list_empty(dst).  It would be helpful to comment the
dynamics which are happening in this case - why we're testing dst here.