Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
On Wed, 4 Nov 2020 22:25:16 +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().
Alex Shi's "mm/memcg: warning on !memcg after readahead page charged"
(https://lkml.kernel.org/r/1604283436-18880-3-git-send-email-alex@linux.alibaba.com)
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()?
Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
On Wed, Nov 4, 2020 at 6:26 AM 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 Reviewed-by: Shakeel Butt
Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
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 Nice. Acked-by: Johannes Weiner
Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
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 > > --- > > 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 Thanks! -- Michal Hocko SUSE Labs
Re: [PATCH] mm/memcontrol:rewrite mem_cgroup_page_lruvec()
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
> ---
> 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!
[PATCH] 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().
Signed-off-by: Hui Su
---
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 = >__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 = >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
