* [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).