Re: [PATCH v3 2/2] mm: mglru: promote mapped executable folios after first usage

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 7/19/26 3:23 AM, Kairui Song wrote:
> On Fri, Jul 17, 2026 at 6:06 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>> Classical LRU protects mapped executable file folios through commit
>> 8cab4754d24a0 ("vmscan: make mapped executable pages the first class
>> citizen") and commit c909e99364c8 ("vmscan: activate executable pages
>> after first usage"), giving executable code a better chance to stay in
>> memory, avoiding IO thrashing and improving workload performance.
>>
>> However, MGLRU's protection of mapped executable file folios is less
>> reliable. Although shrink_folio_list() checks references, the access flag
>> of mapped executable file folios may have already been checked and
>> cleared by lru_gen_look_around() or walk_mm(). Additionally,
>> folio_update_gen() or lru_gen_set_refs() only sets the 'PG_referenced'
>> flag for mapped executable file folios, which causes shrink_folio_list()
>> to ignore the first usage of these mapped executable file folios and
>> reclaim them easily.
>>
>> Follow the classical LRU's logic, promoting mapped executable file folios
>> after their first usage in folio_update_gen() and lru_gen_set_refs(),
>> giving executable code a better chance to stay in memory.
>>
>> On my 32-core Arm machine, with the memcg limit set to 2G, running
>> 'make -j32' to build kernel showed some improvement in sys time.
>>
>> base                    patched
>> 9248.543s               7861.579s
>>
>> While we are at it, introduce a new helper to check mapped executable
>> file folios.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/vmscan.c | 47 +++++++++++++++++++++++++++++++----------------
>>   1 file changed, 31 insertions(+), 16 deletions(-)
> 
> Hi Baolin, thanks for the update, looks good to me with two nit picks:
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index de62899c108d..1040bf9f96e8 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -268,6 +268,12 @@ static int sc_swappiness(struct scan_control *sc, struct mem_cgroup *memcg)
>>   }
>>   #endif
>>
>> +static inline bool is_exec_file_folio(const struct folio *folio,
>> +               const vma_flags_t *vma_flags)
>> +{
>> +       return vma_flags_test(vma_flags, VMA_EXEC_BIT) && folio_is_file_lru(folio);
>> +}
>> +
>>   static void set_task_reclaim_state(struct task_struct *task,
>>                                     struct reclaim_state *rs)
>>   {
>> @@ -835,11 +841,15 @@ enum folio_references {
>>    * with PG_active set. In contrast, the aging (page table walk) path uses
>>    * folio_update_gen().
>>    */
>> -static bool lru_gen_set_refs(struct folio *folio)
>> +static bool lru_gen_set_refs(struct folio *folio, const vma_flags_t *vma_flags)
>>   {
>>          /* see the comment on LRU_REFS_FLAGS */
>>          if (!folio_test_referenced(folio) && !folio_test_workingset(folio)) {
>>                  set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
>> +               /* Activate file-backed executable folios after first usage. */
>> +               if (is_exec_file_folio(folio, vma_flags))
>> +                       return true;
>> +
> 
> This somehow missed the PG_workingset below?

I was following the original logic of this function, which calls 
folio_mark_accessed() before promoting. But after re-reading the comment 
on LRU_REFS_FLAGS:

"
* For folios accessed multiple times through page tables, folio_update_gen()
  * from a page table walk or lru_gen_set_refs() from a rmap walk sets
  * PG_referenced after the accessed bit is cleared for the first time.
  * Thereafter, those two paths set PG_workingset and promote folios to the
  * youngest generation. Like folio_inc_gen(), folio_update_gen() also 
clears
  * PG_referenced. Note that for this case, LRU_REFS_MASK is not used.
"

I agree that I should set PG_workingset and clear LRU_REFS_FLAGS before 
promoting the executable file folios. And sashiko[1] also pointed this 
out. So I'll change it.

[1] 
https://sashiko.dev/#/patchset/cover.1784197559.git.baolin.wang%40linux.alibaba.com

Additionally, I think Barry's earlier patch[2] also has an issue: when 
lru_gen_set_refs() returns true to promote the folio, we should also 
clear LRU_REFS_FLAGS, rather than calling folio_mark_accessed(). So in 
my opinion, when we call folio_mark_accessed(), we should return false 
and should not promote the folio, so the logic shoule be:

static bool lru_gen_set_refs(struct folio *folio)
{
	/* see the comment on LRU_REFS_FLAGS */
	if (!folio_test_referenced(folio) && !folio_test_workingset(folio)) {
		set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
		return false;
	}

	/* Promote on second access */
	if (folio_lru_refs(folio) > 1) {
		set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_workingset));
		return true;
	}

	folio_mark_accessed(folio);
	return false;
}

What do you think? I'd like to send a fix first to correct the logic here.

[2] 
https://lore.kernel.org/linux-mm/20260526130938.66253-1-baohua@kernel.org/

>>                  return false;
>>          }
>>
>> @@ -851,7 +861,7 @@ static bool lru_gen_set_refs(struct folio *folio)
>>          return true;
>>   }
>>   #else
>> -static bool lru_gen_set_refs(struct folio *folio)
>> +static bool lru_gen_set_refs(struct folio *folio, const vma_flags_t *vma_flags)
>>   {
>>          return false;
>>   }
>> @@ -886,7 +896,7 @@ static enum folio_references folio_check_references(struct folio *folio,
>>                  if (!referenced_ptes)
>>                          return FOLIOREF_RECLAIM;
>>
>> -               return lru_gen_set_refs(folio) ? FOLIOREF_ACTIVATE : FOLIOREF_KEEP;
>> +               return lru_gen_set_refs(folio, &vma_flags) ? FOLIOREF_ACTIVATE : FOLIOREF_KEEP;
>>          }
>>
>>          referenced_folio = folio_test_clear_referenced(folio);
>> @@ -914,7 +924,7 @@ static enum folio_references folio_check_references(struct folio *folio,
>>                  /*
>>                   * Activate file-backed executable folios after first usage.
>>                   */
>> -               if (vma_flags_test(&vma_flags, VMA_EXEC_BIT) && folio_is_file_lru(folio))
>> +               if (is_exec_file_folio(folio, &vma_flags))
>>                          return FOLIOREF_ACTIVATE;
>>
>>                  return FOLIOREF_KEEP;
>> @@ -2119,7 +2129,7 @@ static void shrink_active_list(unsigned long nr_to_scan,
>>                           * IO, plus JVM can create lots of anon VM_EXEC folios,
>>                           * so we ignore them here.
>>                           */
>> -                       if (vma_flags_test(&vma_flags, VMA_EXEC_BIT) && folio_is_file_lru(folio)) {
>> +                       if (is_exec_file_folio(folio, &vma_flags)) {
>>                                  nr_rotated += folio_nr_pages(folio);
>>                                  list_add(&folio->lru, &l_active);
>>                                  continue;
>> @@ -3188,7 +3198,7 @@ static bool positive_ctrl_err(struct ctrl_pos *sp, struct ctrl_pos *pv)
>>    ******************************************************************************/
>>
>>   /* promote pages accessed through page tables */
>> -static int folio_update_gen(struct folio *folio, int gen)
>> +static int folio_update_gen(struct folio *folio, int gen, const vma_flags_t *vma_flags)
>>   {
>>          unsigned long new_flags, old_flags = READ_ONCE(folio->flags.f);
>>
>> @@ -3196,10 +3206,15 @@ static int folio_update_gen(struct folio *folio, int gen)
>>
>>          /* see the comment on LRU_REFS_FLAGS */
>>          if (!folio_test_referenced(folio) && !folio_test_workingset(folio)) {
>> +               /* Activate file-backed executable folios after first usage. */
>> +               if (is_exec_file_folio(folio, vma_flags))
>> +                       goto promote;
>> +
> 
> Will it be cleaner if we just:
> 
> /*
> * See the comment on LRU_REFS_FLAGS, and we protect the executable
> * parts to avoid typical IO thrashing from reclaiming.
> */
> if (!folio_test_referenced(folio) && !folio_test_workingset(folio) &&
>      !is_exec_file_folio(folio, vma_flags)) {
>      set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
>      return -1;
> }

Yes. Much cleaner. Will do. Thanks for reviewing.