| Message ID | 20201027080256.76497-6-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some bugs in memcg/slab | expand |
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@bytedance.com> > --- > 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.
On Tue, Oct 27, 2020 at 9:36 PM Michal Hocko <mhocko@suse.com> 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@bytedance.com> > > --- > > 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? Thanks. > -- > Michal Hocko > SUSE Labs
On Tue 27-10-20 22:15:16, Muchun Song wrote: > On Tue, Oct 27, 2020 at 9:36 PM Michal Hocko <mhocko@suse.com> 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@bytedance.com> > > > --- > > > 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.
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) { struct mem_cgroup_per_node *mz; struct lruvec *lruvec; @@ -473,7 +466,7 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, if (!memcg) memcg = root_mem_cgroup; - mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); + mz = mem_cgroup_nodeinfo(memcg, nid); lruvec = &mz->lruvec; out: /* @@ -486,7 +479,34 @@ 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_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) +{ + return mem_cgroup_node_lruvec(memcg, pgdat, pgdat->node_id); +} + +/** + * 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(). + */ +static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page, + struct pglist_data *pgdat) +{ + return mem_cgroup_node_lruvec(page->mem_cgroup, pgdat, + page_to_nid(page)); +} struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p); diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 70345b15b150..7097f3fc4dee 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1332,46 +1332,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
We can reuse the code of mem_cgroup_lruvec() to simplify the code of the mem_cgroup_page_lruvec(). Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/memcontrol.h | 44 +++++++++++++++++++++++++++----------- mm/memcontrol.c | 40 ---------------------------------- 2 files changed, 32 insertions(+), 52 deletions(-)