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@xxxxxxx>
> > ---
> >  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@xxxxxxxx>

Thanks!

-- 
Michal Hocko
SUSE Labs