| Message ID | 20220524060551.80037-3-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use obj_cgroup APIs to charge the LRU pages | expand |
On Tue, May 24, 2022 at 02:05:42PM +0800, Muchun Song wrote: > If we reuse the objcg APIs to charge LRU pages, the folio_memcg() > can be changed when the LRU pages reparented. In this case, we need > to acquire the new lruvec lock. > > lruvec = folio_lruvec(folio); > > // The page is reparented. > > compact_lock_irqsave(&lruvec->lru_lock, &flags, cc); > > // Acquired the wrong lruvec lock and need to retry. > > But compact_lock_irqsave() only take lruvec lock as the parameter, > we cannot aware this change. If it can take the page as parameter > to acquire the lruvec lock. When the page memcg is changed, we can > use the folio_memcg() detect whether we need to reacquire the new > lruvec lock. So compact_lock_irqsave() is not suitable for us. > Similar to folio_lruvec_lock_irqsave(), introduce > compact_folio_lruvec_lock_irqsave() to acquire the lruvec lock in > the compaction routine. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> This looks generally good to me. It did raise the question how deferencing lruvec is safe before the lock is acquired when reparenting can race. The answer is in the next patch when you add the rcu_read_lock(). Since the patches aren't big, it would probably be better to merge them. > @@ -509,6 +509,29 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags, > return true; > } > > +static struct lruvec * > +compact_folio_lruvec_lock_irqsave(struct folio *folio, unsigned long *flags, > + struct compact_control *cc) > +{ > + struct lruvec *lruvec; > + > + lruvec = folio_lruvec(folio); > + > + /* Track if the lock is contended in async mode */ > + if (cc->mode == MIGRATE_ASYNC && !cc->contended) { > + if (spin_trylock_irqsave(&lruvec->lru_lock, *flags)) > + goto out; > + > + cc->contended = true; > + } > + > + spin_lock_irqsave(&lruvec->lru_lock, *flags); Can you implement this on top of the existing one? lruvec = folio_lruvec(folio); compact_lock_irqsave(&lruvec->lru_lock, flags); lruvec_memcg_debug(lruvec, folio); return lruvec;
On Tue, May 24, 2022 at 03:22:55PM -0400, Johannes Weiner wrote: > On Tue, May 24, 2022 at 02:05:42PM +0800, Muchun Song wrote: > > If we reuse the objcg APIs to charge LRU pages, the folio_memcg() > > can be changed when the LRU pages reparented. In this case, we need > > to acquire the new lruvec lock. > > > > lruvec = folio_lruvec(folio); > > > > // The page is reparented. > > > > compact_lock_irqsave(&lruvec->lru_lock, &flags, cc); > > > > // Acquired the wrong lruvec lock and need to retry. > > > > But compact_lock_irqsave() only take lruvec lock as the parameter, > > we cannot aware this change. If it can take the page as parameter > > to acquire the lruvec lock. When the page memcg is changed, we can > > use the folio_memcg() detect whether we need to reacquire the new > > lruvec lock. So compact_lock_irqsave() is not suitable for us. > > Similar to folio_lruvec_lock_irqsave(), introduce > > compact_folio_lruvec_lock_irqsave() to acquire the lruvec lock in > > the compaction routine. > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > This looks generally good to me. > > It did raise the question how deferencing lruvec is safe before the > lock is acquired when reparenting can race. The answer is in the next > patch when you add the rcu_read_lock(). Since the patches aren't big, > it would probably be better to merge them. > Will do in v5. > > @@ -509,6 +509,29 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags, > > return true; > > } > > > > +static struct lruvec * > > +compact_folio_lruvec_lock_irqsave(struct folio *folio, unsigned long *flags, > > + struct compact_control *cc) > > +{ > > + struct lruvec *lruvec; > > + > > + lruvec = folio_lruvec(folio); > > + > > + /* Track if the lock is contended in async mode */ > > + if (cc->mode == MIGRATE_ASYNC && !cc->contended) { > > + if (spin_trylock_irqsave(&lruvec->lru_lock, *flags)) > > + goto out; > > + > > + cc->contended = true; > > + } > > + > > + spin_lock_irqsave(&lruvec->lru_lock, *flags); > > Can you implement this on top of the existing one? > > lruvec = folio_lruvec(folio); > compact_lock_irqsave(&lruvec->lru_lock, flags); > lruvec_memcg_debug(lruvec, folio); > return lruvec; > I'll do a try. Thanks for your suggestions.
diff --git a/mm/compaction.c b/mm/compaction.c index fe915db6149b..817098817302 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -509,6 +509,29 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags, return true; } +static struct lruvec * +compact_folio_lruvec_lock_irqsave(struct folio *folio, unsigned long *flags, + struct compact_control *cc) +{ + struct lruvec *lruvec; + + lruvec = folio_lruvec(folio); + + /* Track if the lock is contended in async mode */ + if (cc->mode == MIGRATE_ASYNC && !cc->contended) { + if (spin_trylock_irqsave(&lruvec->lru_lock, *flags)) + goto out; + + cc->contended = true; + } + + spin_lock_irqsave(&lruvec->lru_lock, *flags); +out: + lruvec_memcg_debug(lruvec, folio); + + return lruvec; +} + /* * Compaction requires the taking of some coarse locks that are potentially * very heavily contended. The lock should be periodically unlocked to avoid @@ -844,6 +867,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, /* Time to isolate some pages for migration */ for (; low_pfn < end_pfn; low_pfn++) { + struct folio *folio; if (skip_on_failure && low_pfn >= next_skip_pfn) { /* @@ -1065,18 +1089,17 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, if (!TestClearPageLRU(page)) goto isolate_fail_put; - lruvec = folio_lruvec(page_folio(page)); + folio = page_folio(page); + lruvec = folio_lruvec(folio); /* If we already hold the lock, we can skip some rechecking */ if (lruvec != locked) { if (locked) unlock_page_lruvec_irqrestore(locked, flags); - compact_lock_irqsave(&lruvec->lru_lock, &flags, cc); + lruvec = compact_folio_lruvec_lock_irqsave(folio, &flags, cc); locked = lruvec; - lruvec_memcg_debug(lruvec, page_folio(page)); - /* Try get exclusive access under lock */ if (!skip_updated) { skip_updated = true;
If we reuse the objcg APIs to charge LRU pages, the folio_memcg() can be changed when the LRU pages reparented. In this case, we need to acquire the new lruvec lock. lruvec = folio_lruvec(folio); // The page is reparented. compact_lock_irqsave(&lruvec->lru_lock, &flags, cc); // Acquired the wrong lruvec lock and need to retry. But compact_lock_irqsave() only take lruvec lock as the parameter, we cannot aware this change. If it can take the page as parameter to acquire the lruvec lock. When the page memcg is changed, we can use the folio_memcg() detect whether we need to reacquire the new lruvec lock. So compact_lock_irqsave() is not suitable for us. Similar to folio_lruvec_lock_irqsave(), introduce compact_folio_lruvec_lock_irqsave() to acquire the lruvec lock in the compaction routine. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/compaction.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)