| Message ID | 20210416051407.54878-6-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memcontrol code cleanup and simplification | expand |
On Fri 16-04-21 13:14:04, Muchun Song wrote: > lruvec_holds_page_lru_lock() doesn't check anything about locking and is > used to check whether the page belongs to the lruvec. So rename it to > page_matches_lruvec(). > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Acked-by: Michal Hocko <mhocko@suse.com> Thanks > --- > include/linux/memcontrol.h | 7 +++---- > mm/vmscan.c | 2 +- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 2fc728492c9b..40b0c31ea4ba 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -1492,8 +1492,7 @@ static inline void unlock_page_lruvec_irqrestore(struct lruvec *lruvec, > spin_unlock_irqrestore(&lruvec->lru_lock, flags); > } > > -static inline bool lruvec_holds_page_lru_lock(struct page *page, > - struct lruvec *lruvec) > +static inline bool page_matches_lruvec(struct page *page, struct lruvec *lruvec) > { > return lruvec_pgdat(lruvec) == page_pgdat(page) && > lruvec_memcg(lruvec) == page_memcg(page); > @@ -1504,7 +1503,7 @@ static inline struct lruvec *relock_page_lruvec_irq(struct page *page, > struct lruvec *locked_lruvec) > { > if (locked_lruvec) { > - if (lruvec_holds_page_lru_lock(page, locked_lruvec)) > + if (page_matches_lruvec(page, locked_lruvec)) > return locked_lruvec; > > unlock_page_lruvec_irq(locked_lruvec); > @@ -1518,7 +1517,7 @@ static inline struct lruvec *relock_page_lruvec_irqsave(struct page *page, > struct lruvec *locked_lruvec, unsigned long *flags) > { > if (locked_lruvec) { > - if (lruvec_holds_page_lru_lock(page, locked_lruvec)) > + if (page_matches_lruvec(page, locked_lruvec)) > return locked_lruvec; > > unlock_page_lruvec_irqrestore(locked_lruvec, *flags); > diff --git a/mm/vmscan.c b/mm/vmscan.c > index bb8321026c0c..2bc5cf409958 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -2062,7 +2062,7 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, > * All pages were isolated from the same lruvec (and isolation > * inhibits memcg migration). > */ > - VM_BUG_ON_PAGE(!lruvec_holds_page_lru_lock(page, lruvec), page); > + VM_BUG_ON_PAGE(!page_matches_lruvec(page, lruvec), page); > add_page_to_lru_list(page, lruvec); > nr_pages = thp_nr_pages(page); > nr_moved += nr_pages; > -- > 2.11.0
On Fri, Apr 16, 2021 at 01:14:04PM +0800, Muchun Song wrote: > lruvec_holds_page_lru_lock() doesn't check anything about locking and is > used to check whether the page belongs to the lruvec. So rename it to > page_matches_lruvec(). > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> The rename makes sense, since the previous name was defined by a specific use case rather than what it does. That said, it did imply a lock context that makes the test result stable. Without that the function could use a short comment, IMO. How about: /* Test requires a stable page->memcg binding, see page_memcg() */ With that, Acked-by: Johannes Weiner <hannes@cmpxchg.org>
On Thu, Apr 15, 2021 at 10:16 PM Muchun Song <songmuchun@bytedance.com> wrote: > > lruvec_holds_page_lru_lock() doesn't check anything about locking and is > used to check whether the page belongs to the lruvec. So rename it to > page_matches_lruvec(). > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Reviewed-by: Shakeel Butt <shakeelb@google.com>
On Fri, Apr 16, 2021 at 11:20 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Fri, Apr 16, 2021 at 01:14:04PM +0800, Muchun Song wrote: > > lruvec_holds_page_lru_lock() doesn't check anything about locking and is > > used to check whether the page belongs to the lruvec. So rename it to > > page_matches_lruvec(). > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > The rename makes sense, since the previous name was defined by a > specific use case rather than what it does. That said, it did imply a > lock context that makes the test result stable. Without that the > function could use a short comment, IMO. How about: > > /* Test requires a stable page->memcg binding, see page_memcg() */ Make sense. I will add this comment. > > With that, > Acked-by: Johannes Weiner <hannes@cmpxchg.org> Thanks.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 2fc728492c9b..40b0c31ea4ba 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -1492,8 +1492,7 @@ static inline void unlock_page_lruvec_irqrestore(struct lruvec *lruvec, spin_unlock_irqrestore(&lruvec->lru_lock, flags); } -static inline bool lruvec_holds_page_lru_lock(struct page *page, - struct lruvec *lruvec) +static inline bool page_matches_lruvec(struct page *page, struct lruvec *lruvec) { return lruvec_pgdat(lruvec) == page_pgdat(page) && lruvec_memcg(lruvec) == page_memcg(page); @@ -1504,7 +1503,7 @@ static inline struct lruvec *relock_page_lruvec_irq(struct page *page, struct lruvec *locked_lruvec) { if (locked_lruvec) { - if (lruvec_holds_page_lru_lock(page, locked_lruvec)) + if (page_matches_lruvec(page, locked_lruvec)) return locked_lruvec; unlock_page_lruvec_irq(locked_lruvec); @@ -1518,7 +1517,7 @@ static inline struct lruvec *relock_page_lruvec_irqsave(struct page *page, struct lruvec *locked_lruvec, unsigned long *flags) { if (locked_lruvec) { - if (lruvec_holds_page_lru_lock(page, locked_lruvec)) + if (page_matches_lruvec(page, locked_lruvec)) return locked_lruvec; unlock_page_lruvec_irqrestore(locked_lruvec, *flags); diff --git a/mm/vmscan.c b/mm/vmscan.c index bb8321026c0c..2bc5cf409958 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2062,7 +2062,7 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, * All pages were isolated from the same lruvec (and isolation * inhibits memcg migration). */ - VM_BUG_ON_PAGE(!lruvec_holds_page_lru_lock(page, lruvec), page); + VM_BUG_ON_PAGE(!page_matches_lruvec(page, lruvec), page); add_page_to_lru_list(page, lruvec); nr_pages = thp_nr_pages(page); nr_moved += nr_pages;
lruvec_holds_page_lru_lock() doesn't check anything about locking and is used to check whether the page belongs to the lruvec. So rename it to page_matches_lruvec(). Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/memcontrol.h | 7 +++---- mm/vmscan.c | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-)