[PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[Hui Su]

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-9Onoh4P/yGk@public.gmane.org>
---
 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

Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[Roman Gushchin]

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!

Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[Michal Hocko]

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-9Onoh4P/yGk@public.gmane.org>
> > ---
> >  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-IBi9RG/b67k@public.gmane.org>

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[Johannes Weiner]

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-9Onoh4P/yGk@public.gmane.org>

Nice.

Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[Shakeel Butt]

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>

Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()

[Andrew Morton]

On Wed, 4 Nov 2020 22:25:16 +0800 Hui Su <sh_def-9Onoh4P/yGk@public.gmane.org> 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-KPsoFbNs7GizrGE5bRqYAgC/G2K4zDHf@public.gmane.org)
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()?