Messages in this thread Patch in this message |  | | Subject | Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec | From | Alex Shi <> | Date | Sun, 29 Nov 2020 12:43:52 +0800 |
| |
在 2020/11/28 下午12:02, Andrew Morton 写道: > On Fri, 27 Nov 2020 11:08:35 +0800 Alex Shi <alex.shi@linux.alibaba.com> wrote: > >> Sometime, we use NULL memcg in mem_cgroup_lruvec(memcg, pgdat) >> so we could get out early in the situation to avoid useless checking. >> >> Also warning if both parameter are NULL. > > Why do you think a warning is needed here?
Uh, Consider there are no problem for long time, it could be saved.
> >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -613,14 +613,13 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, >> struct mem_cgroup_per_node *mz; >> struct lruvec *lruvec; >> >> - if (mem_cgroup_disabled()) { >> + VM_WARN_ON_ONCE(!memcg && !pgdat); >> + >> + if (mem_cgroup_disabled() || !memcg) { >> lruvec = &pgdat->__lruvec; >> goto out; >> } >> >> - if (!memcg) >> - memcg = root_mem_cgroup; >> - > > This change isn't obviously equivalent, is it?
If !memcg, the root_mem_cgroup will still lead the lruvec to a pgdat same as parameter.
> >> mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); >> lruvec = &mz->lruvec; >> out: > > And the resulting code is awkward: > > if (mem_cgroup_disabled() || !memcg) { > lruvec = &pgdat->__lruvec; > goto out; > } > > mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); > lruvec = &mz->lruvec; > out: > > > could be > > if (mem_cgroup_disabled() || !memcg) { > lruvec = &pgdat->__lruvec; > } else { > mem_cgroup_per_node mz; > > mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); > lruvec = &mz->lruvec; > } >
Right. remove 'goto' is better for understander.
So, is the following patch ok?
From 225f29e03b40a7cbaeb4e3bb76f8efbcd7d648a2 Mon Sep 17 00:00:00 2001 From: Alex Shi <alex.shi@linux.alibaba.com> Date: Wed, 25 Nov 2020 14:06:33 +0800 Subject: [PATCH v2] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec
Sometime, we use NULL memcg in mem_cgroup_lruvec(memcg, pgdat) so we could get out early in the situation to avoid useless checking.
Polished as Andrew Morton's suggestion.
Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Shakeel Butt <shakeelb@google.com> Cc: Roman Gushchin <guro@fb.com> Cc: Lorenzo Stoakes <lstoakes@gmail.com> Cc: Stephen Rothwell <sfr@canb.auug.org.au> Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com> Cc: Yafang Shao <laoar.shao@gmail.com> Cc: Wei Yang <richard.weiyang@gmail.com> Cc: linux-kernel@vger.kernel.org --- include/linux/memcontrol.h | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 3e6a1df3bdb9..4ff2ffe2b73d 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -610,20 +610,17 @@ mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid) static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg, struct pglist_data *pgdat) { - struct mem_cgroup_per_node *mz; struct lruvec *lruvec; - if (mem_cgroup_disabled()) { + if (mem_cgroup_disabled() || !memcg) { lruvec = &pgdat->__lruvec; - goto out; - } + } else { + struct mem_cgroup_per_node *mz; - if (!memcg) - memcg = root_mem_cgroup; + mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); + lruvec = &mz->lruvec; + } - mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); - lruvec = &mz->lruvec; -out: /* * Since a node can be onlined after the mem_cgroup was created, * we have to be prepared to initialize lruvec->pgdat here; -- 2.29.GIT
|  |