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@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 7 Mar 2024 11:19:52 +0800 Yafang Shao <laoar.shao@xxxxxxxxx> 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;
}
