RE: [RFC PATCH] mm/mglru: dynamically protect readahead fault folios under refault pressure

[Ababneh, Ehab]

Hi Barry, Kairui,

Thanks for the feedback and suggestions.

> -----Original Message-----
> From: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> Sent: Tuesday, September 1, 2026 3:05 PM
> To: ryncsn@xxxxxxxxx; Ababneh, Ehab <ehab.ababneh@xxxxxxxxx>
> Cc: akpm@xxxxxxxxxxxxxxxxxxxx; axelrasmussen@xxxxxxxxxx;
> baohua@xxxxxxxxxx; kasong@xxxxxxxxxxx; lance.yang@xxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; qi.zheng@xxxxxxxxx;
> shakeel.butt@xxxxxxxxx; weixugc@xxxxxxxxxx; yuanchu@xxxxxxxxxx;
> yuzhao@xxxxxxxxxx
> Subject: Re: [RFC PATCH] mm/mglru: dynamically protect readahead fault
> folios under refault pressure
> 
> On Wed, Sep 2, 2026 at 2:21 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> >
> > On Tue, Sep 01, 2026 at 11:06:43AM +0800, Ehab Ababneh wrote:
> > > Commit 6cbdd9726fb5 ("mm/mglru: use folio_mark_accessed to replace
> > > folio_set_active") introduced a regression for workloads that rely
> > > on readahead to keep sequential file access efficient.
> > >
> > > The problem is that MGLRU can place fault-path file folios in older
> > > generations, so memory pressure can reclaim readahead folios before
> > > the workload touches them. In our Cassandra read benchmark, this
> > > raised p99 latency to about 9.2-9.5 ms and cut throughput to roughly
> > > 41.8k-43.6k op/s; the revert restored the workload to about 5.5-5.6
> > > ms and 51.9k-53.1k op/s.
> > >
> > > Readahead is important for sequential I/O and mmap scans, but it
> > > should not be retained when the workload does not benefit from it.
> > > The goal is to keep the optimization without keeping readahead pages alive
> forever.
> > >
> > > This patch provides a middle ground: keep the original behavior by
> > > default, but temporarily protect fault-path file folios when
> > > repeated file refaults show that readahead is actually helping.
> > >
> > > The mechanism is dynamic and self-tuning:
> > >
> > > - add a per-lruvec readahead/refault credit
> > > - accumulate credit on file refaults in the MGLRU refault path
> > > - consume credit in folio_add_lru() for fault-path file folios
> > > - keep the folio active while credit is available, and otherwise let
> > > the
> > >   original behavior stand
> > > - decay/reset the credit as generations advance and when an lruvec
> > > is
> > >   initialized
> > >
> > > This means we only protect fault-path file folios when refault
> > > pressure shows that the workload is actively benefiting from
> > > readahead. If the workload does not need that protection, the
> > > original optimization remains intact and we do not keep readahead pages
> around unnecessarily.
> > >
> > > Benchmark results for the Cassandra read workload
> > > (4 nodes, 720s, 100 readers):
> > >
> > > - with commit 6cbdd9726fb5 ("mm/mglru: use folio_mark_accessed to
> > >   replace folio_set_active"):
> > >   p99 ~9.2-9.5 ms, throughput ~41.8k-43.6k op/s
> > > - with revert of commit 6cbdd9726fb5 ("mm/mglru: use
> > > folio_mark_accessed to
> > >   replace folio_set_active"):
> > >   p99 ~5.5-5.6 ms, throughput ~51.9k-53.1k op/s
> > > - with this fix: p99 ~5.8 ms, throughput ~51.9k-52.7k op/s
> >
> [...]
> >
> > Just an idea. For an short term and easy fix, what if we simply revert
> > than, then only protect in_fault && folio_test_swapbacked folios with
> > PG_active?
> 
> Hi Ehab and Kairui,
> 
> Thanks very much for your reports and discussion.
> 
> I'm not quite sure we want to revert it entirely, as that would immediately
> regress the workloads improved by the commit, such as the kernel build.
> Also, for example, Kairui's cover letter mentioned the LevelDB benchmark:
>  "I also retested the LevelDB benchmark from the cache_ext paper [5].
> Interestingly, mainline MGLRU already beats CLRU on this one after a recent
> change in lru_gen_folio_seq that bumps new folios with refs == 1 to the
> second-oldest generation."
> 
> I guess we could instead try to mitigate the cases where some workloads are
> negatively affected while preserving the original intention. Does the fix below
> address both of your cases, or is Ehab's case actually different from Kairui's?
> (The kernel-build test on my machine looks quite positive. It not only
> preserves the original optimization, but even provides further gains.)
> 

I agree that reverting the commit that caused the regression is not the
optimal path. I expect there are many workloads and scenarios that
benefit from the behavior introduced by that commit, so reverting it
could unnecessarily regress those workloads.

I will run the Cassandra benchmark with Kairui's MGLRU-FG patches to see
whether they address the issue I am seeing. I will send the results when
they are ready.

Thanks,
Ehab

> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h index
> 621c8653d8f7..91396c796a34 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
> @@ -239,7 +239,8 @@ static inline unsigned long lru_gen_folio_seq(const
> struct lruvec *lruvec,
>  	 * |<---------------------------- MAX_NR_GENS ---------------------------->|
>  	 */
>  	if (folio_test_active(folio))
> -		gen = MIN_NR_GENS - folio_test_workingset(folio);
> +		gen = MIN_NR_GENS - folio_test_workingset(folio) +
> +		      (type ? !folio_test_workingset(folio) : 0);
>  	else if (reclaiming)
>  		gen = MAX_NR_GENS;
>  	else if ((!folio_is_file_lru(folio) && !folio_test_swapcache(folio)) ||
> @@ -247,7 +248,7 @@ static inline unsigned long lru_gen_folio_seq(const
> struct lruvec *lruvec,
>  		  (folio_test_dirty(folio) || folio_test_writeback(folio))))
>  		gen = MIN_NR_GENS;
>  	else
> -		gen = MAX_NR_GENS - (folio_test_workingset(folio) ||
> folio_test_referenced(folio));
> +		gen = MAX_NR_GENS - folio_test_workingset(folio);
> 
>  	return max(READ_ONCE(lrugen->max_seq) - gen + 1,
> READ_ONCE(lrugen->min_seq[type]));
>  }
> diff --git a/mm/folio.c b/mm/folio.c
> index c02dcea9c03c..2fd835b3b50c 100644
> --- a/mm/folio.c
> +++ b/mm/folio.c
> @@ -470,20 +470,10 @@ void folio_add_lru(struct folio *folio)
>  			folio_test_unevictable(folio), folio);
>  	VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
> 
> -	/*
> -	 * For refaulted workingset folios, set PG_active so they
> -	 * can be added to active generations.
> -	 * For prefaulted file folios, folio_mark_accessed() sets
> -	 * PG_referenced so lru_gen_folio_seq() places them into
> -	 * the second oldest generation.
> -	 */
> +	/* see the comment in lru_gen_folio_seq() */
>  	if (lru_gen_enabled() && !folio_test_unevictable(folio) &&
> -	    lru_gen_in_fault() && !(current->flags & PF_MEMALLOC)) {
> -		if (folio_test_workingset(folio))
> -			folio_set_active(folio);
> -		else if (!folio_test_referenced(folio))
> -			folio_mark_accessed(folio);
> -	}
> +	    lru_gen_in_fault() && !(current->flags & PF_MEMALLOC))
> +		folio_set_active(folio);
> 
>  	folio_batch_add_and_move(folio, lru_add);  } diff --git
> a/mm/vmscan.c b/mm/vmscan.c index f11491ee9ed5..aa500ae9371a
> 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -851,11 +851,7 @@ static bool lru_gen_set_refs(struct folio *folio, const
> vma_flags_t *vma_flags)
>  		return false;
>  	}
> 
> -	/* Promote on second access */
> -	if (folio_lru_refs(folio) > 1)
> -		set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS,
> BIT(PG_workingset));
> -	else
> -		folio_mark_accessed(folio);
> +	set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_workingset));
>  	return true;
>  }
>  #else
> diff --git a/mm/workingset.c b/mm/workingset.c index
> 7ac2b88c80ae..a79b73ee9762 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -319,13 +319,11 @@ static void lru_gen_refault(struct folio *folio, void
> *shadow)
> 
>  	atomic_long_add(delta, &lrugen->refaulted[hist][type][tier]);
> 
> +	/* see folio_add_lru() where folio_set_active() will be called */
> +	if (lru_gen_in_fault())
> +		mod_lruvec_state(lruvec, WORKINGSET_ACTIVATE_BASE +
> type, delta);
> +
>  	if (workingset) {
> -		/*
> -		 * see folio_add_lru(), where folio_set_active() is
> -		 * called for workingset folios
> -		 */
> -		if (lru_gen_in_fault())
> -			mod_lruvec_state(lruvec,
> WORKINGSET_ACTIVATE_BASE + type, delta);
>  		folio_set_workingset(folio);
>  		mod_lruvec_state(lruvec, WORKINGSET_RESTORE_BASE +
> type, delta);
>  	} else
> --
> 2.34.1