linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
@ 2020-11-04 14:25 Hui Su
  2020-11-04 22:38 ` Roman Gushchin
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Hui Su @ 2020-11-04 14:25 UTC (permalink / raw)
  To: hannes, mhocko, vdavydov.dev, akpm, shakeelb, guro, laoar.shao,
	chris, linux-kernel, cgroups, linux-mm
  Cc: sh_def

mem_cgroup_page_lruvec() in memcontrol.c and
mem_cgroup_lruvec() in memcontrol.h is very similar
except for the param(page and memcg) which also can be
convert to each other.

So rewrite mem_cgroup_page_lruvec() with mem_cgroup_lruvec().

Signed-off-by: Hui Su <sh_def@163.com>
---
 include/linux/memcontrol.h | 18 +++++++++++++++--
 mm/memcontrol.c            | 40 --------------------------------------
 2 files changed, 16 insertions(+), 42 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index e391e3c56de5..a586363fb766 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -457,9 +457,10 @@ mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
 /**
  * mem_cgroup_lruvec - get the lru list vector for a memcg & node
  * @memcg: memcg of the wanted lruvec
+ * @pgdat: pglist_data
  *
  * Returns the lru list vector holding pages for a given @memcg &
- * @node combination. This can be the node lruvec, if the memory
+ * @pgdat combination. This can be the node lruvec, if the memory
  * controller is disabled.
  */
 static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
@@ -489,7 +490,20 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
 	return lruvec;
 }
 
-struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
+/**
+ * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
+ * @page: the page
+ * @pgdat: pgdat of the page
+ *
+ * This function relies on page->mem_cgroup being stable.
+ */
+static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
+						struct pglist_data *pgdat)
+{
+	struct mem_cgroup *memcg = page->mem_cgroup;
+
+	return mem_cgroup_lruvec(memcg, pgdat);
+}
 
 struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3dcbf24d2227..9d4e1150b194 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1330,46 +1330,6 @@ int mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
 	return ret;
 }
 
-/**
- * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
- * @page: the page
- * @pgdat: pgdat of the page
- *
- * This function relies on page->mem_cgroup being stable - see the
- * access rules in commit_charge().
- */
-struct lruvec *mem_cgroup_page_lruvec(struct page *page, struct pglist_data *pgdat)
-{
-	struct mem_cgroup_per_node *mz;
-	struct mem_cgroup *memcg;
-	struct lruvec *lruvec;
-
-	if (mem_cgroup_disabled()) {
-		lruvec = &pgdat->__lruvec;
-		goto out;
-	}
-
-	memcg = page->mem_cgroup;
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
-	if (!memcg)
-		memcg = root_mem_cgroup;
-
-	mz = mem_cgroup_page_nodeinfo(memcg, page);
-	lruvec = &mz->lruvec;
-out:
-	/*
-	 * Since a node can be onlined after the mem_cgroup was created,
-	 * we have to be prepared to initialize lruvec->zone here;
-	 * and if offlined then reonlined, we need to reinitialize it.
-	 */
-	if (unlikely(lruvec->pgdat != pgdat))
-		lruvec->pgdat = pgdat;
-	return lruvec;
-}
-
 /**
  * mem_cgroup_update_lru_size - account for adding or removing an lru page
  * @lruvec: mem_cgroup per zone lru vector
-- 
2.29.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
  2020-11-04 14:25 [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec() Hui Su
@ 2020-11-04 22:38 ` Roman Gushchin
  2020-11-05  7:26   ` Michal Hocko
  2020-11-05 15:53 ` Johannes Weiner
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Roman Gushchin @ 2020-11-04 22:38 UTC (permalink / raw)
  To: Hui Su
  Cc: hannes, mhocko, vdavydov.dev, akpm, shakeelb, laoar.shao, chris,
	linux-kernel, cgroups, linux-mm

On Wed, Nov 04, 2020 at 10:25:16PM +0800, Hui Su wrote:
> mem_cgroup_page_lruvec() in memcontrol.c and
> mem_cgroup_lruvec() in memcontrol.h is very similar
> except for the param(page and memcg) which also can be
> convert to each other.
> 
> So rewrite mem_cgroup_page_lruvec() with mem_cgroup_lruvec().
> 
> Signed-off-by: Hui Su <sh_def@163.com>
> ---
>  include/linux/memcontrol.h | 18 +++++++++++++++--
>  mm/memcontrol.c            | 40 --------------------------------------
>  2 files changed, 16 insertions(+), 42 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index e391e3c56de5..a586363fb766 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -457,9 +457,10 @@ mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
>  /**
>   * mem_cgroup_lruvec - get the lru list vector for a memcg & node
>   * @memcg: memcg of the wanted lruvec
> + * @pgdat: pglist_data
>   *
>   * Returns the lru list vector holding pages for a given @memcg &
> - * @node combination. This can be the node lruvec, if the memory
> + * @pgdat combination. This can be the node lruvec, if the memory
>   * controller is disabled.
>   */
>  static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
> @@ -489,7 +490,20 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  	return lruvec;
>  }

Hi Hui,

>  
> -struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
> +/**
> + * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
> + * @page: the page
> + * @pgdat: pgdat of the page
> + *
> + * This function relies on page->mem_cgroup being stable.
> + */
> +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> +						struct pglist_data *pgdat)

Hm, do we need to pass page and pgdat?

> +{
> +	struct mem_cgroup *memcg = page->mem_cgroup;

It seems like you need to rebase the patch against the latest mm snapshot.

> +
> +	return mem_cgroup_lruvec(memcg, pgdat);

Thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
  2020-11-04 22:38 ` Roman Gushchin
@ 2020-11-05  7:26   ` Michal Hocko
  0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2020-11-05  7:26 UTC (permalink / raw)
  To: Roman Gushchin
  Cc: Hui Su, hannes, vdavydov.dev, akpm, shakeelb, laoar.shao, chris,
	linux-kernel, cgroups, linux-mm

On Wed 04-11-20 14:38:00, Roman Gushchin wrote:
> On Wed, Nov 04, 2020 at 10:25:16PM +0800, Hui Su wrote:
> > mem_cgroup_page_lruvec() in memcontrol.c and
> > mem_cgroup_lruvec() in memcontrol.h is very similar
> > except for the param(page and memcg) which also can be
> > convert to each other.
> > 
> > So rewrite mem_cgroup_page_lruvec() with mem_cgroup_lruvec().
> > 
> > Signed-off-by: Hui Su <sh_def@163.com>
> > ---
> >  include/linux/memcontrol.h | 18 +++++++++++++++--
> >  mm/memcontrol.c            | 40 --------------------------------------
> >  2 files changed, 16 insertions(+), 42 deletions(-)
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index e391e3c56de5..a586363fb766 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -457,9 +457,10 @@ mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
> >  /**
> >   * mem_cgroup_lruvec - get the lru list vector for a memcg & node
> >   * @memcg: memcg of the wanted lruvec
> > + * @pgdat: pglist_data
> >   *
> >   * Returns the lru list vector holding pages for a given @memcg &
> > - * @node combination. This can be the node lruvec, if the memory
> > + * @pgdat combination. This can be the node lruvec, if the memory
> >   * controller is disabled.
> >   */
> >  static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
> > @@ -489,7 +490,20 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
> >  	return lruvec;
> >  }
> 
> Hi Hui,
> 
> >  
> > -struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
> > +/**
> > + * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
> > + * @page: the page
> > + * @pgdat: pgdat of the page
> > + *
> > + * This function relies on page->mem_cgroup being stable.
> > + */
> > +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> > +						struct pglist_data *pgdat)
> 
> Hm, do we need to pass page and pgdat?

Not really because page already knows its node so pgdat can be easily
taken from there. I suspect the only reason for having pgdat here is
that many callers already know it and we optimize for memcg disable
case. Hard to tell whether this actually matters because most of those
paths are not really hot but something that would require a deeper
investigation. Hint hint...

Anyway, this looks like a nice simplification already. There were some
attempts to do similar thing recently but they were adding nodeid as an
additional argument and I really disliked those.

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
  2020-11-04 14:25 [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec() Hui Su
  2020-11-04 22:38 ` Roman Gushchin
@ 2020-11-05 15:53 ` Johannes Weiner
  2020-11-06 16:44 ` Shakeel Butt
  2020-11-07  1:30 ` Andrew Morton
  3 siblings, 0 replies; 6+ messages in thread
From: Johannes Weiner @ 2020-11-05 15:53 UTC (permalink / raw)
  To: Hui Su
  Cc: mhocko, vdavydov.dev, akpm, shakeelb, guro, laoar.shao, chris,
	linux-kernel, cgroups, linux-mm

On Wed, Nov 04, 2020 at 10:25:16PM +0800, Hui Su wrote:
> mem_cgroup_page_lruvec() in memcontrol.c and
> mem_cgroup_lruvec() in memcontrol.h is very similar
> except for the param(page and memcg) which also can be
> convert to each other.
> 
> So rewrite mem_cgroup_page_lruvec() with mem_cgroup_lruvec().
> 
> Signed-off-by: Hui Su <sh_def@163.com>

Nice.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
  2020-11-04 14:25 [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec() Hui Su
  2020-11-04 22:38 ` Roman Gushchin
  2020-11-05 15:53 ` Johannes Weiner
@ 2020-11-06 16:44 ` Shakeel Butt
  2020-11-07  1:30 ` Andrew Morton
  3 siblings, 0 replies; 6+ messages in thread
From: Shakeel Butt @ 2020-11-06 16:44 UTC (permalink / raw)
  To: Hui Su
  Cc: Johannes Weiner, Michal Hocko, Vladimir Davydov, Andrew Morton,
	Roman Gushchin, Yafang Shao, Chris Down, LKML, Cgroups, Linux MM

On Wed, Nov 4, 2020 at 6:26 AM Hui Su <sh_def@163.com> wrote:
>
> mem_cgroup_page_lruvec() in memcontrol.c and
> mem_cgroup_lruvec() in memcontrol.h is very similar
> except for the param(page and memcg) which also can be
> convert to each other.
>
> So rewrite mem_cgroup_page_lruvec() with mem_cgroup_lruvec().
>
> Signed-off-by: Hui Su <sh_def@163.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
  2020-11-04 14:25 [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec() Hui Su
                   ` (2 preceding siblings ...)
  2020-11-06 16:44 ` Shakeel Butt
@ 2020-11-07  1:30 ` Andrew Morton
  3 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2020-11-07  1:30 UTC (permalink / raw)
  To: Hui Su
  Cc: hannes, mhocko, vdavydov.dev, shakeelb, guro, laoar.shao, chris,
	linux-kernel, cgroups, linux-mm

On Wed, 4 Nov 2020 22:25:16 +0800 Hui Su <sh_def@163.com> wrote:

> mem_cgroup_page_lruvec() in memcontrol.c and
> mem_cgroup_lruvec() in memcontrol.h is very similar
> except for the param(page and memcg) which also can be
> convert to each other.
> 
> So rewrite mem_cgroup_page_lruvec() with mem_cgroup_lruvec().

Alex Shi's "mm/memcg: warning on !memcg after readahead page charged"
(https://lkml.kernel.org/r/1604283436-18880-3-git-send-email-alex.shi@linux.alibaba.com)
changes mem_cgroup_page_lruvec() thusly:

--- a/mm/memcontrol.c~mm-memcg-warning-on-memcg-after-readahead-page-charged
+++ a/mm/memcontrol.c
@@ -1325,10 +1325,7 @@ struct lruvec *mem_cgroup_page_lruvec(st
 	}
 
 	memcg = page_memcg(page);
-	/*
-	 * Swapcache readahead pages are added to the LRU - and
-	 * possibly migrated - before they are charged.
-	 */
+	VM_WARN_ON_ONCE_PAGE(!memcg, page);
 	if (!memcg)
 		memcg = root_mem_cgroup;
 
So the patch didn't apply.

That's easily fixed, but it does make one wonder whether this:

> -struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
> +/**
> + * mem_cgroup_page_lruvec - return lruvec for isolating/putting an LRU page
> + * @page: the page
> + * @pgdat: pgdat of the page
> + *
> + * This function relies on page->mem_cgroup being stable.
> + */
> +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
> +						struct pglist_data *pgdat)
> +{
> +	struct mem_cgroup *memcg = page->mem_cgroup;
> +
> +	return mem_cgroup_lruvec(memcg, pgdat);
> +}

Should be using page_memcg()?


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-07  1:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-04 14:25 [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec() Hui Su
2020-11-04 22:38 ` Roman Gushchin
2020-11-05  7:26   ` Michal Hocko
2020-11-05 15:53 ` Johannes Weiner
2020-11-06 16:44 ` Shakeel Butt
2020-11-07  1:30 ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).