Re: [PATCH v2 0/5] Avoid building lrugen page table walk code

[Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>]

On 7/7/23 1:27 PM, Yu Zhao wrote:
> On Thu, Jul 6, 2023 at 12:21 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> This patchset avoids building changes added by commit bd74fdaea146 ("mm:
>> multi-gen LRU: support page table walks") on platforms that don't support
>> hardware atomic updates of access bits.
>>
>> Aneesh Kumar K.V (5):
>>   mm/mglru: Create a new helper iterate_mm_list_walk
>>   mm/mglru: Move Bloom filter code around
>>   mm/mglru: Move code around to make future patch easy
>>   mm/mglru: move iterate_mm_list_walk Helper
>>   mm/mglru: Don't build multi-gen LRU page table walk code on
>>     architecture not supported
>>
>>  arch/Kconfig               |   3 +
>>  arch/arm64/Kconfig         |   1 +
>>  arch/x86/Kconfig           |   1 +
>>  include/linux/memcontrol.h |   2 +-
>>  include/linux/mm_types.h   |  10 +-
>>  include/linux/mmzone.h     |  12 +-
>>  kernel/fork.c              |   2 +-
>>  mm/memcontrol.c            |   2 +-
>>  mm/vmscan.c                | 955 +++++++++++++++++++------------------
>>  9 files changed, 528 insertions(+), 460 deletions(-)
> 
> 1. There is no need for a new Kconfig -- the condition is simply
>    defined(CONFIG_LRU_GEN) && !defined(arch_has_hw_pte_young)
> 
> 2. The best practice to disable static functions is not by macros but:
> 
>     static int const_cond(void)
>     {
>         return 1;
>     }
> 
>     int main(void)
>     {
>         int a = const_cond();
> 
>         if (a)
>                 return 0;
> 
>         /* the compiler doesn't generate code for static funcs below */
>         static_func_1();
>         ...
>         static_func_N();
> 
> LTO also optimizes external functions. But not everyone uses it. So we
> still need macros for them, and of course data structures.
> 
> 3. In 4/5, you have:
> 
>     @@ -461,6 +461,7 @@ enum {
>      struct lru_gen_mm_state {
>         /* set to max_seq after each iteration */
>         unsigned long seq;
>     +#ifdef CONFIG_LRU_TASK_PAGE_AGING
>         /* where the current iteration continues after */
>         struct list_head *head;
>         /* where the last iteration ended before */
>     @@ -469,6 +470,11 @@ struct lru_gen_mm_state {
>         unsigned long *filters[NR_BLOOM_FILTERS];
>         /* the mm stats for debugging */
>         unsigned long stats[NR_HIST_GENS][NR_MM_STATS];
>     +#else
>     +   /* protect the seq update above */
>     +   /* May be we can use lruvec->lock?  */
>     +   spinlock_t lock;
>     +#endif
>      };
> 
> The answer is yes, and not only that, we don't need lru_gen_mm_state at all.
> 
> I'm attaching a patch that fixes all above. If you want to post it,
> please feel free -- fully test it please, since I didn't. Otherwise I
> can ask TJ to help make this work for you.
> 
> $ git diff --stat
>  include/linux/memcontrol.h |   2 +-
>  include/linux/mm_types.h   |  12 +-
>  include/linux/mmzone.h     |   2 +
>  kernel/bounds.c            |   6 +-
>  kernel/fork.c              |   2 +-
>  mm/vmscan.c                | 169 +++++++++++++++++++--------
>  6 files changed, 137 insertions(+), 56 deletions(-)
> 
> On x86:
> 
> $ ./scripts/bloat-o-meter mm/vmscan.o.old mm/vmscan.o
> add/remove: 24/34 grow/shrink: 2/7 up/down: 966/-8716 (-7750)
> Function                                     old     new   delta
> ...
> should_skip_vma                              206       -    -206
> get_pte_pfn                                  261       -    -261
> lru_gen_add_mm                               323       -    -323
> lru_gen_seq_show                            1710    1370    -340
> lru_gen_del_mm                               432       -    -432
> reset_batch_size                             572       -    -572
> try_to_inc_max_seq                          2947    1635   -1312
> walk_pmd_range_locked                       1508       -   -1508
> walk_pud_range                              3238       -   -3238
> Total: Before=99449, After=91699, chg -7.79%
> 
> $ objdump -S mm/vmscan.o | grep -A 20 "<try_to_inc_max_seq>:"
> 000000000000a350 <try_to_inc_max_seq>:
> {
>     a350: e8 00 00 00 00        call   a355 <try_to_inc_max_seq+0x5>
>     a355: 55                    push   %rbp
>     a356: 48 89 e5              mov    %rsp,%rbp
>     a359: 41 57                push   %r15
>     a35b: 41 56                push   %r14
>     a35d: 41 55                push   %r13
>     a35f: 41 54                push   %r12
>     a361: 53                    push   %rbx
>     a362: 48 83 ec 70          sub    $0x70,%rsp
>     a366: 41 89 d4              mov    %edx,%r12d
>     a369: 49 89 f6              mov    %rsi,%r14
>     a36c: 49 89 ff              mov    %rdi,%r15
> spin_lock_irq(&lruvec->lru_lock);
>     a36f: 48 8d 5f 50          lea    0x50(%rdi),%rbx
>     a373: 48 89 df              mov    %rbx,%rdi
>     a376: e8 00 00 00 00        call   a37b <try_to_inc_max_seq+0x2b>
> success = max_seq == lrugen->max_seq;
>     a37b: 49 8b 87 88 00 00 00 mov    0x88(%r15),%rax
>     a382: 4c 39 f0              cmp    %r14,%rax

For the below diff:

@@ -4497,14 +4547,16 @@ static bool try_to_inc_max_seq(struct lruvec *lruvec, unsigned long max_seq,
 	struct lru_gen_mm_walk *walk;
 	struct mm_struct *mm = NULL;
 	struct lru_gen_folio *lrugen = &lruvec->lrugen;
+	struct lru_gen_mm_state *mm_state = get_mm_state(lruvec);
 
 	VM_WARN_ON_ONCE(max_seq > READ_ONCE(lrugen->max_seq));
 
+	if (!mm_state)
+		return inc_max_seq(lruvec, max_seq, can_swap, force_scan);
+
 	/* see the comment in iterate_mm_list() */
-	if (max_seq <= READ_ONCE(lruvec->mm_state.seq)) {
-		success = false;
-		goto done;
-	}
+	if (max_seq <= READ_ONCE(mm_state->seq))
+		return false;
 
 	/*
 	 * If the hardware doesn't automatically set the accessed bit, fallback
@@ -4534,8 +4586,10 @@ static bool try_to_inc_max_seq(struct lruvec *lruvec, unsigned long max_seq,
 			walk_mm(lruvec, mm, walk);
 	} while (mm);
 done:
-	if (success)
-		inc_max_seq(lruvec, can_swap, force_scan);
+	if (success) {
+		success = inc_max_seq(lruvec, max_seq, can_swap, force_scan);
+		WARN_ON_ONCE(!success);
+	}
 
 	return success;
 }
@

We did discuss a possible race that can happen if we allow multiple callers hit inc_max_seq at the same time. 
inc_max_seq drop the lru_lock and restart the loop at the previous value of type. ie. if we want to do the above
we might also need the below? 


modified   mm/vmscan.c
@@ -4368,6 +4368,7 @@ void inc_max_seq(struct lruvec *lruvec, bool can_swap, bool force_scan)
 	int type, zone;
 	struct lru_gen_struct *lrugen = &lruvec->lrugen;
 
+retry:
 	spin_lock_irq(&lruvec->lru_lock);
 
 	VM_WARN_ON_ONCE(!seq_is_valid(lruvec));
@@ -4381,7 +4382,7 @@ void inc_max_seq(struct lruvec *lruvec, bool can_swap, bool force_scan)
 		while (!inc_min_seq(lruvec, type, can_swap)) {
 			spin_unlock_irq(&lruvec->lru_lock);
 			cond_resched();
-			spin_lock_irq(&lruvec->lru_lock);
+			goto retry;
 		}
 	}
 
I also found that allowing only one cpu to increment max seq value and making other request
with the same max_seq return false is also useful in performance runs. ie, we need an equivalent of this? 


+	if (max_seq <= READ_ONCE(mm_state->seq))
+		return false;