On Wed, Mar 13, 2024 at 6:12 AM Yu Zhao <yuzhao@xxxxxxxxxx> 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@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; > > } > > 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