On Fri, Jul 7, 2023 at 7:24 AM Aneesh Kumar K V
<aneesh.kumar@xxxxxxxxxxxxx> wrote:
>
> 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?
Yes, you are right.
In fact, there is an existing bug here: even though max_seq can't
change without the patch, min_seq still can, and if it does, the
initial condition for inc_min_seq() to keep looping, i.e., nr_gens at
max, doesn't hold anymore.
for (type = ANON_AND_FILE - 1; type >= 0; type--) {
if (get_nr_gens(lruvec, type) != MAX_NR_GENS)
continue;
This only affects the debugfs interface (force_scan=true), which is
probably why it wasn't never reported.
> 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;
Yes, but the condition should be "<" -- ">" is a bug, which was
asserted in try_to_in_max_seq().
