mm/memcontrol:rewrite mem_cgroup_page_lruvec()
Subsystems (3)
PATCH 1/1 mm/memcontrol:rewrite mem_cgroup_page_lruvec()
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().
--- include/linux/memcontrol.h | 18 +++++++++++++++-- mm/memcontrol.c | 40 -------------------------------------- 2 files changed, 16 insertions(+), 42 deletions(-)
include/linux/memcontrol.h +16 -2
@@ -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);
mm/memcontrol.c +0 -40
@@ -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
Signature · 1 line
2.29.0
Quoted 30 lines · Hui Su
mem_cgroup_page_lruvec()inmemcontrol.candmem_cgroup_lruvec()inmemcontrol.his very similar except for the param(page and memcg) which also can be convert to each other.So rewrite
mem_cgroup_page_lruvec()withmem_cgroup_lruvec().Signed-off-by Hui Su <[email protected]>--- include/linux/memcontrol.h | 18 +++++++++++++++-- mm/memcontrol.c | 40 -------------------------------------- 2 files changed, 16 insertions(+), 42 deletions(-)include/linux/memcontrol.h +2
-1@@ -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,
include/linux/memcontrol.h static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, · line 491
Quoted 2 lines
include/linux/memcontrol.h +0
-0@@ -489,7 +490,20 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,return lruvec; }
Hi Hui,
include/linux/memcontrol.h static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, · line 501
Quoted 11 lines
include/linux/memcontrol.h +9
-1-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?
include/linux/memcontrol.h static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, · line 503
Quoted 2 lines
include/linux/memcontrol.h +2
-0+{ +struct mem_cgroup *memcg = page->mem_cgroup;
It seems like you need to rebase the patch against the latest mm snapshot.
include/linux/memcontrol.h static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, · line 505
Quoted 2 lines
include/linux/memcontrol.h +2
-0+ +return mem_cgroup_lruvec (memcg, pgdat);
Thanks!
Michal Hocko
Acked-byQuoted 37 lines · Roman Gushchin
Quoted 30 lines · Hui Su
mem_cgroup_page_lruvec()inmemcontrol.candmem_cgroup_lruvec()inmemcontrol.his very similar except for the param(page and memcg) which also can be convert to each other.So rewrite
mem_cgroup_page_lruvec()withmem_cgroup_lruvec().Signed-off-by Hui Su <sh_def-9Onoh4P/[email protected]>--- include/linux/memcontrol.h | 18 +++++++++++++++-- mm/memcontrol.c | 40 -------------------------------------- 2 files changed, 16 insertions(+), 42 deletions(-)include/linux/memcontrol.h +2
-1@@ -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,include/linux/memcontrol.h static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, · line 491
Quoted 2 lines
include/linux/memcontrol.h +0
-0@@ -489,7 +490,20 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,return lruvec; }Hi Hui,
Quoted 1 line
Quoted 12 lines
include/linux/memcontrol.h static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, · line 501
Quoted 10 lines
include/linux/memcontrol.h +9
-1-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.
Thanks!
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 8 lines · Hui Su
mem_cgroup_page_lruvec()inmemcontrol.candmem_cgroup_lruvec()inmemcontrol.his very similar except for the param(page and memcg) which also can be convert to each other.So rewrite
mem_cgroup_page_lruvec()withmem_cgroup_lruvec().Signed-off-by Hui Su <sh_def-9Onoh4P/[email protected]>
Nice.
Quoted 9 lines · Hui Su
mem_cgroup_page_lruvec()inmemcontrol.candmem_cgroup_lruvec()inmemcontrol.his very similar except for the param(page and memcg) which also can be convert to each other.So rewrite
mem_cgroup_page_lruvec()withmem_cgroup_lruvec().Signed-off-by Hui Su <[email protected]>
Andrew Morton
maintainerQuoted 6 lines · On Wed, 4 Nov 2020 22:25:16 +0800 Hui Su <sh_def-9Onoh4P/[email protected]>…
mem_cgroup_page_lruvec()inmemcontrol.candmem_cgroup_lruvec()inmemcontrol.his very similar except for the param(page and memcg) which also can be convert to each other.So rewrite
mem_cgroup_page_lruvec()withmem_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/[email protected])
changes mem_cgroup_page_lruvec() thusly:
mm/memcontrol.c renamed from mm/memcontrol.c~mm-memcg-warning-on-memcg-after-readahead-page-charged +1 -4
@@ -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:
Quoted 5 lines
(quoted diff) +4
-1-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
Review
Quoted 10 lines
(quoted diff) +10
-0+ * + * 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()?