Messages in this thread |  | | Subject | Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec | From | Dmitry Osipenko <> | Date | Mon, 30 Nov 2020 22:44:11 +0300 |
| |
29.11.2020 07:43, Alex Shi пишет: > > > 在 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; >
Hi,
This patch causes a hard lock on one of my ARM32 devices using today's linux-next, please fix.
|  |