Re: [External] Re: [PATCH 5/5] mm: memcontrol: Simplify the mem_cgroup_page_lruvec

[Michal Hocko]

On Tue 27-10-20 22:15:16, Muchun Song wrote:
> On Tue, Oct 27, 2020 at 9:36 PM Michal Hocko <mhocko@xxxxxxxx> wrote:
> >
> > On Tue 27-10-20 16:02:56, Muchun Song wrote:
> > > We can reuse the code of mem_cgroup_lruvec() to simplify the code
> > > of the mem_cgroup_page_lruvec().
> >
> > yes, removing the code duplication is reasonable. But ...
> >
> > >
> > > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > > ---
> > >  include/linux/memcontrol.h | 44 +++++++++++++++++++++++++++-----------
> > >  mm/memcontrol.c            | 40 ----------------------------------
> > >  2 files changed, 32 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > index 95807bf6be64..5e8480e54cd8 100644
> > > --- a/include/linux/memcontrol.h
> > > +++ b/include/linux/memcontrol.h
> > > @@ -451,16 +451,9 @@ mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid)
> > >       return memcg->nodeinfo[nid];
> > >  }
> > >
> > > -/**
> > > - * mem_cgroup_lruvec - get the lru list vector for a memcg & node
> > > - * @memcg: memcg of the wanted lruvec
> > > - *
> > > - * Returns the lru list vector holding pages for a given @memcg &
> > > - * @node combination. This can be the node lruvec, if the memory
> > > - * controller is disabled.
> > > - */
> > > -static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
> > > -                                            struct pglist_data *pgdat)
> > > +static inline struct lruvec *mem_cgroup_node_lruvec(struct mem_cgroup *memcg,
> > > +                                                 struct pglist_data *pgdat,
> > > +                                                 int nid)
> >
> > This is just wrong interface. Either take nid or pgdat. You do not want
> > both because that just begs for wrong usage.
> 
> If we want to avoid abuse of mem_cgroup_node_lruvec. We can move
> those functions to the memcontrol.c. And add the "static" attribute to the
> mem_cgroup_node_lruvec. Just export mem_cgroup_lruvec and
> mem_cgroup_page_lruvec. Is this OK?

Sorry, I was probably not clear enough. I am not against the function
per se. I just do not think we want to make it trickier to use than
necessary. That means either use pgdat or nid argument. Not both because
they should always be in sync and you can trivially get from one to the
other and vice versa.
-- 
Michal Hocko
SUSE Labs