Re: [PATCH] mm: mglru: Fix soft lockup attributed to scanning folios

[Yafang Shao <laoar.shao@gmail.com>]

On Wed, Mar 13, 2024 at 6:12 AM Yu Zhao <yuzhao@google.com> wrote:
>
> On Tue, Mar 12, 2024 at 02:29:48PM -0600, Yu Zhao wrote:
> > On Fri, Mar 08, 2024 at 04:57:08PM +0800, Yafang Shao wrote:
> > > On Fri, Mar 8, 2024 at 1:06 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >
> > > > On Thu,  7 Mar 2024 11:19:52 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >
> > > > > After we enabled mglru on our 384C1536GB production servers, we
> > > > > encountered frequent soft lockups attributed to scanning folios.
> > > > >
> > > > > The soft lockup as follows,
> > > > >
> > > > > ...
> > > > >
> > > > > There were a total of 22 tasks waiting for this spinlock
> > > > > (RDI: ffff99d2b6ff9050):
> > > > >
> > > > >  crash> foreach RU bt | grep -B 8  queued_spin_lock_slowpath |  grep "RDI: ffff99d2b6ff9050" | wc -l
> > > > >  22
> > > >
> > > > If we're holding the lock for this long then there's a possibility of
> > > > getting hit by the NMI watchdog also.
> > >
> > > The NMI watchdog is disabled as these servers are KVM guest.
> > >
> > >     kernel.nmi_watchdog = 0
> > >     kernel.soft_watchdog = 1
> > >
> > > >
> > > > > Additionally, two other threads were also engaged in scanning folios, one
> > > > > with 19 waiters and the other with 15 waiters.
> > > > >
> > > > > To address this issue under heavy reclaim conditions, we introduced a
> > > > > hotfix version of the fix, incorporating cond_resched() in scan_folios().
> > > > > Following the application of this hotfix to our servers, the soft lockup
> > > > > issue ceased.
> > > > >
> > > > > ...
> > > > >
> > > > > --- a/mm/vmscan.c
> > > > > +++ b/mm/vmscan.c
> > > > > @@ -4367,6 +4367,10 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc,
> > > > >
> > > > >                       if (!--remaining || max(isolated, skipped_zone) >= MIN_LRU_BATCH)
> > > > >                               break;
> > > > > +
> > > > > +                     spin_unlock_irq(&lruvec->lru_lock);
> > > > > +                     cond_resched();
> > > > > +                     spin_lock_irq(&lruvec->lru_lock);
> > > > >               }
> > > >
> > > > Presumably wrapping this with `if (need_resched())' will save some work.
> > >
> > > good suggestion.
> > >
> > > >
> > > > This lock is held for a reason.  I'd like to see an analysis of why
> > > > this change is safe.
> > >
> > > I believe the key point here is whether we can reduce the scope of
> > > this lock from:
> > >
> > >   evict_folios
> > >       spin_lock_irq(&lruvec->lru_lock);
> > >       scanned = isolate_folios(lruvec, sc, swappiness, &type, &list);
> > >       scanned += try_to_inc_min_seq(lruvec, swappiness);
> > >       if (get_nr_gens(lruvec, !swappiness) == MIN_NR_GENS)
> > >           scanned = 0;
> > >       spin_unlock_irq(&lruvec->lru_lock);
> > >
> > > to:
> > >
> > >   evict_folios
> > >       spin_lock_irq(&lruvec->lru_lock);
> > >       scanned = isolate_folios(lruvec, sc, swappiness, &type, &list);
> > >       spin_unlock_irq(&lruvec->lru_lock);
> > >
> > >       spin_lock_irq(&lruvec->lru_lock);
> > >       scanned += try_to_inc_min_seq(lruvec, swappiness);
> > >       if (get_nr_gens(lruvec, !swappiness) == MIN_NR_GENS)
> > >           scanned = 0;
> > >       spin_unlock_irq(&lruvec->lru_lock);
> > >
> > > In isolate_folios(), it merely utilizes the min_seq to retrieve the
> > > generation without modifying it. If multiple tasks are running
> > > evict_folios() concurrently, it seems inconsequential whether min_seq
> > > is incremented by one task or another. I'd appreciate Yu's
> > > confirmation on this matter.
> >
> > Hi Yafang,
> >
> > Thanks for the patch!
> >
> > Yes, your second analysis is correct -- we can't just drop the lock
> > as the original patch does because min_seq can be updated in the mean
> > time. If this happens, the gen value becomes invalid, since it's based
> > on the expired min_seq:
> >
> >   sort_folio()
> >   {
> >     ..
> >     gen = lru_gen_from_seq(lrugen->min_seq[type]);
> >     ..
> >   }
> >
> > The following might be a better approach (untested):
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 4255619a1a31..6fe53cfa8ef8 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -4365,7 +4365,8 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc,
> >                               skipped_zone += delta;
> >                       }
> >
> > -                     if (!--remaining || max(isolated, skipped_zone) >= MIN_LRU_BATCH)
> > +                     if (!--remaining || max(isolated, skipped_zone) >= MIN_LRU_BATCH ||
> > +                         spin_is_contended(&lruvec->lru_lock))
> >                               break;
> >               }
> >
> > @@ -4375,7 +4376,8 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc,
> >                       skipped += skipped_zone;
> >               }
> >
> > -             if (!remaining || isolated >= MIN_LRU_BATCH)
> > +             if (!remaining || isolated >= MIN_LRU_BATCH ||
> > +                 (scanned && spin_is_contended(&lruvec->lru_lock)))
> >                       break;
> >       }
>
> A better way might be:
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4255619a1a31..ac59f064c4e1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4367,6 +4367,11 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc,
>
>                         if (!--remaining || max(isolated, skipped_zone) >= MIN_LRU_BATCH)
>                                 break;
> +
> +                       if (need_resched() || spin_is_contended(&lruvec->lru_lock)) {
> +                               remaining = 0;
> +                               break;
> +                       }
>                 }
>
>                 if (skipped_zone) {

It is better. Thanks for your suggestion.
I will verify it on our production servers, which may take several days.

-- 
Regards
Yafang