* [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec @ 2020-11-27 3:08 Alex Shi 2020-11-28 4:02 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Alex Shi @ 2020-11-27 3:08 UTC (permalink / raw) Cc: Andrew Morton, Johannes Weiner, Shakeel Butt, Roman Gushchin, Lorenzo Stoakes, Stephen Rothwell, Alexander Duyck, Yafang Shao, Wei Yang, linux-kernel 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. 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 | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 3e6a1df3bdb9..4cdb110f84e0 100644 --- 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; - mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id); lruvec = &mz->lruvec; out: -- 2.29.GIT ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec 2020-11-27 3:08 [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec Alex Shi @ 2020-11-28 4:02 ` Andrew Morton 2020-11-29 4:43 ` Alex Shi 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2020-11-28 4:02 UTC (permalink / raw) To: Alex Shi Cc: Johannes Weiner, Shakeel Butt, Roman Gushchin, Lorenzo Stoakes, Stephen Rothwell, Alexander Duyck, Yafang Shao, Wei Yang, linux-kernel 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? > --- 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? > 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec 2020-11-28 4:02 ` Andrew Morton @ 2020-11-29 4:43 ` Alex Shi 2020-11-30 19:44 ` Dmitry Osipenko 0 siblings, 1 reply; 5+ messages in thread From: Alex Shi @ 2020-11-29 4:43 UTC (permalink / raw) To: Andrew Morton Cc: Johannes Weiner, Shakeel Butt, Roman Gushchin, Lorenzo Stoakes, Stephen Rothwell, Alexander Duyck, Yafang Shao, Wei Yang, linux-kernel 在 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec 2020-11-29 4:43 ` Alex Shi @ 2020-11-30 19:44 ` Dmitry Osipenko 2020-11-30 20:19 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Osipenko @ 2020-11-30 19:44 UTC (permalink / raw) To: Alex Shi, Andrew Morton Cc: Johannes Weiner, Shakeel Butt, Roman Gushchin, Lorenzo Stoakes, Stephen Rothwell, Alexander Duyck, Yafang Shao, Wei Yang, linux-kernel, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec 2020-11-30 19:44 ` Dmitry Osipenko @ 2020-11-30 20:19 ` Andrew Morton 0 siblings, 0 replies; 5+ messages in thread From: Andrew Morton @ 2020-11-30 20:19 UTC (permalink / raw) To: Dmitry Osipenko Cc: Alex Shi, Johannes Weiner, Shakeel Butt, Roman Gushchin, Lorenzo Stoakes, Stephen Rothwell, Alexander Duyck, Yafang Shao, Wei Yang, linux-kernel, linux-arm-kernel On Mon, 30 Nov 2020 22:44:11 +0300 Dmitry Osipenko <digetx@gmail.com> wrote: > > 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. > > > > --- 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. Thanks. This is unexpected. I assume you've confirmed that reverting this change from linux-next fixes things? ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-30 20:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-27 3:08 [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec Alex Shi 2020-11-28 4:02 ` Andrew Morton 2020-11-29 4:43 ` Alex Shi 2020-11-30 19:44 ` Dmitry Osipenko 2020-11-30 20:19 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).