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@xxxxxxxxxxxxx> 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;
