Re: [PATCH v2 05/26] mm/fbatch: lru_add_del_folio()+folio_add_lru() after clear_lru()

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

On 9/9/26 11:51, Hugh Dickins wrote:
> Most callers of folio_test_clear_lru() then proceed to remove the folio
> from its lru, and add it back at the end when they're done (if still in
> use). But isolate_migratepages_block() and check_move_unevictable_pages()
> sometimes decide against, and release immediately with a folio_set_lru().
> 
> Which usually works fine: but there's now a small chance that while they
> held the folio with lru bit cleared, an lru_add fbatch drain came along,
> and had to skip that folio because its lru bit was transiently cleared
> (previously, the lru_add fbatch drain relied on finding lru bit never yet
> set). This risks leaving that folio off lru, unreclaimable until freed.

So this makes the previous patch a somewhat bisection hazard? I guess it's
acceptable given it's not fatal.

> Fix such cases by trying lru_add_del_folio() (which only takes action and
> returns true if the folio was on an lru_add fbatch), then folio_add_lru()

Oh ok, that's one detail I didn't realize on the previous patch, and
explains the name of the function. But it's still IMHO confusing.

> if it succeeded: invalidating the old fbatch slot, appending in a new one.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>

In general, LGTM.
Reviewed-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>

Nit below:

> ---
>  mm/compaction.c | 13 +++++++++++--
>  mm/vmscan.c     | 16 +++++++++-------
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index a049415512c6..9e045a90ba21 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1204,7 +1204,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  				     !cc->alloc_contig)) {
>  				low_pfn += folio_nr_pages(folio) - 1;
>  				nr_scanned += folio_nr_pages(folio) - 1;
> -				folio_set_lru(folio);
> +				if (lru_add_del_folio(folio)) {
> +					lruvec_unlock_irqrestore(locked, flags);
> +					folio_add_lru(folio);
> +					locked = NULL;
> +				} else {
> +					folio_set_lru(folio);
> +				}
>  				goto isolate_fail_put;
>  			}
>  		}
> @@ -1293,7 +1299,10 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  	if (locked)
>  		lruvec_unlock_irqrestore(locked, flags);
>  	if (folio) {
> -		folio_set_lru(folio);
> +		if (lru_add_del_folio(folio))
> +			folio_add_lru(folio);
> +		else
> +			folio_set_lru(folio);
>  		folio_put(folio);
>  	}
>  
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f11491ee9ed5..4e8d5cc34f07 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -8093,17 +8093,19 @@ void check_move_unevictable_folios(struct folio_batch *fbatch)
>  			folio_clear_unevictable(folio);
>  			lruvec_add_folio(lruvec, folio);
>  			pgrescued += nr_pages;
> +		} else if (lru_add_del_folio(folio)) {
> +			lruvec_unlock_irq(lruvec);
> +			folio_add_lru(folio);
> +			lruvec = NULL;
>  		}
> -		folio_set_lru(folio);
> +		if (lruvec)
> +			folio_set_lru(folio);
>  	}
>  
> -	if (lruvec) {
> -		__count_vm_events(UNEVICTABLE_PGRESCUED, pgrescued);
> -		__count_vm_events(UNEVICTABLE_PGSCANNED, pgscanned);
> +	if (lruvec)
>  		lruvec_unlock_irq(lruvec);
> -	} else if (pgscanned) {
> -		count_vm_events(UNEVICTABLE_PGSCANNED, pgscanned);
> -	}
> +	count_vm_events(UNEVICTABLE_PGRESCUED, pgrescued);
> +	count_vm_events(UNEVICTABLE_PGSCANNED, pgscanned);

AFAIU this is done because we can no longer rule out that !lruvec means
pgrescued is 0.
But we can still distinguish the cheaper __count_vm_events vs
count_vm_events? Probably all the same on x86, but I hear on arm64 this_cpu*
ops have a cost worth proposing rather elaborate schemes to deal with...

>  }
>  EXPORT_SYMBOL_GPL(check_move_unevictable_folios);
>