| Message ID | 20220111010302.8864-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] mm/memcg: use NUMA_NO_NODE to indicate allocation from unspecified node | expand |
On Tue, Jan 11, 2022 at 9:03 AM Wei Yang <richard.weiyang@gmail.com> wrote: > > Instead of use "-1", let's use NUMA_NO_NODE for consistency. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks.
On Tue 11-01-22 01:02:59, Wei Yang wrote: > Instead of use "-1", let's use NUMA_NO_NODE for consistency. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> I am not really sure this is worth it. After the merge window I plan to post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. With that in place we can drop the check and a node rewrite so the net result will be a less and more straightforward code. If you agree I can add this with your s-o-b into my series: diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 781605e92015..ed19a21ee14e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5044,18 +5044,8 @@ struct mem_cgroup *mem_cgroup_from_id(unsigned short id) static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) { struct mem_cgroup_per_node *pn; - int tmp = node; - /* - * This routine is called against possible nodes. - * But it's BUG to call kmalloc() against offline node. - * - * TODO: this routine can waste much memory for nodes which will - * never be onlined. It's better to use memory hotplug callback - * function. - */ - if (!node_state(node, N_NORMAL_MEMORY)) - tmp = -1; - pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp); + + pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, node); if (!pn) return 1;
On Tue, Jan 11, 2022 at 01:02:59AM +0000, Wei Yang wrote: > Instead of use "-1", let's use NUMA_NO_NODE for consistency. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> If the patch won't be dropped for Michal's work, please, feel free to add Acked-by: Roman Gushchin <guro@fb.com> Thanks!
On Tue, Jan 11, 2022 at 09:40:20AM +0100, Michal Hocko wrote: >On Tue 11-01-22 01:02:59, Wei Yang wrote: >> Instead of use "-1", let's use NUMA_NO_NODE for consistency. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >I am not really sure this is worth it. After the merge window I plan to >post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. Give me some time to understand it :-)
On Wed 12-01-22 00:46:34, Wei Yang wrote: > On Tue, Jan 11, 2022 at 09:40:20AM +0100, Michal Hocko wrote: > >On Tue 11-01-22 01:02:59, Wei Yang wrote: > >> Instead of use "-1", let's use NUMA_NO_NODE for consistency. > >> > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > > > >I am not really sure this is worth it. After the merge window I plan to > >post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. > > Give me some time to understand it :-) Just for the record, here is what I have put on top of that series: --- From b7195eba02fe6308a6927450f4630057c05e808e Mon Sep 17 00:00:00 2001 From: Wei Yang <richard.weiyang@gmail.com> Date: Tue, 11 Jan 2022 09:45:25 +0100 Subject: [PATCH] memcg: do not tweak node in alloc_mem_cgroup_per_node_info alloc_mem_cgroup_per_node_info is allocated for each possible node and this used to be a problem because not !node_online nodes didn't have appropriate data structure allocated. This has changed by "mm: handle uninitialized numa nodes gracefully" so we can drop the special casing here. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Signed-off-by: Michal Hocko <mhocko@suse.com> --- mm/memcontrol.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 781605e92015..ed19a21ee14e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5044,18 +5044,8 @@ struct mem_cgroup *mem_cgroup_from_id(unsigned short id) static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) { struct mem_cgroup_per_node *pn; - int tmp = node; - /* - * This routine is called against possible nodes. - * But it's BUG to call kmalloc() against offline node. - * - * TODO: this routine can waste much memory for nodes which will - * never be onlined. It's better to use memory hotplug callback - * function. - */ - if (!node_state(node, N_NORMAL_MEMORY)) - tmp = -1; - pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp); + + pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, node); if (!pn) return 1;
On Wed, Jan 12, 2022 at 09:56:15AM +0100, Michal Hocko wrote: >On Wed 12-01-22 00:46:34, Wei Yang wrote: >> On Tue, Jan 11, 2022 at 09:40:20AM +0100, Michal Hocko wrote: >> >On Tue 11-01-22 01:02:59, Wei Yang wrote: >> >> Instead of use "-1", let's use NUMA_NO_NODE for consistency. >> >> >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> > >> >I am not really sure this is worth it. After the merge window I plan to >> >post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. >> >> Give me some time to understand it :-) > >Just for the record, here is what I have put on top of that series: Ok, I got what you try to resolve. I am ok with the following change except one point. >--- >>From b7195eba02fe6308a6927450f4630057c05e808e Mon Sep 17 00:00:00 2001 >From: Wei Yang <richard.weiyang@gmail.com> >Date: Tue, 11 Jan 2022 09:45:25 +0100 >Subject: [PATCH] memcg: do not tweak node in alloc_mem_cgroup_per_node_info > >alloc_mem_cgroup_per_node_info is allocated for each possible node and >this used to be a problem because not !node_online nodes didn't have >appropriate data structure allocated. This has changed by "mm: handle >uninitialized numa nodes gracefully" so we can drop the special casing >here. > >Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >Signed-off-by: Michal Hocko <mhocko@suse.com> >--- > mm/memcontrol.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > >diff --git a/mm/memcontrol.c b/mm/memcontrol.c >index 781605e92015..ed19a21ee14e 100644 >--- a/mm/memcontrol.c >+++ b/mm/memcontrol.c >@@ -5044,18 +5044,8 @@ struct mem_cgroup *mem_cgroup_from_id(unsigned short id) > static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > { > struct mem_cgroup_per_node *pn; >- int tmp = node; >- /* >- * This routine is called against possible nodes. >- * But it's BUG to call kmalloc() against offline node. >- * >- * TODO: this routine can waste much memory for nodes which will >- * never be onlined. It's better to use memory hotplug callback >- * function. >- */ Do you think this TODO is not related to this change? >- if (!node_state(node, N_NORMAL_MEMORY)) >- tmp = -1; >- pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp); >+ >+ pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, node); > if (!pn) > return 1; > >-- >2.30.2 > > >-- >Michal Hocko >SUSE Labs
On Fri 14-01-22 00:29:37, Wei Yang wrote: > On Wed, Jan 12, 2022 at 09:56:15AM +0100, Michal Hocko wrote: > >On Wed 12-01-22 00:46:34, Wei Yang wrote: > >> On Tue, Jan 11, 2022 at 09:40:20AM +0100, Michal Hocko wrote: > >> >On Tue 11-01-22 01:02:59, Wei Yang wrote: > >> >> Instead of use "-1", let's use NUMA_NO_NODE for consistency. > >> >> > >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >> > > >> >I am not really sure this is worth it. After the merge window I plan to > >> >post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. > >> > >> Give me some time to understand it :-) > > > >Just for the record, here is what I have put on top of that series: > > Ok, I got what you try to resolve. I am ok with the following change except > one point. > > >--- > >>From b7195eba02fe6308a6927450f4630057c05e808e Mon Sep 17 00:00:00 2001 > >From: Wei Yang <richard.weiyang@gmail.com> > >Date: Tue, 11 Jan 2022 09:45:25 +0100 > >Subject: [PATCH] memcg: do not tweak node in alloc_mem_cgroup_per_node_info > > > >alloc_mem_cgroup_per_node_info is allocated for each possible node and > >this used to be a problem because not !node_online nodes didn't have > >appropriate data structure allocated. This has changed by "mm: handle > >uninitialized numa nodes gracefully" so we can drop the special casing > >here. > > > >Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >Signed-off-by: Michal Hocko <mhocko@suse.com> > >--- > > mm/memcontrol.c | 14 ++------------ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > >diff --git a/mm/memcontrol.c b/mm/memcontrol.c > >index 781605e92015..ed19a21ee14e 100644 > >--- a/mm/memcontrol.c > >+++ b/mm/memcontrol.c > >@@ -5044,18 +5044,8 @@ struct mem_cgroup *mem_cgroup_from_id(unsigned short id) > > static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > > { > > struct mem_cgroup_per_node *pn; > >- int tmp = node; > >- /* > >- * This routine is called against possible nodes. > >- * But it's BUG to call kmalloc() against offline node. > >- * > >- * TODO: this routine can waste much memory for nodes which will > >- * never be onlined. It's better to use memory hotplug callback > >- * function. > >- */ > > Do you think this TODO is not related to this change? It is not really related but I am not sure how useful it is. Essentially any allocation that is per-possible node is in the same situation and if we really need to deal with large and sparse possible nodes masks. If you want me to keep the TODO I will do it though.
On Tue, Jan 11, 2022 at 01:02:59AM +0000, Wei Yang wrote: > Instead of use "-1", let's use NUMA_NO_NODE for consistency. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com> > --- > mm/memcontrol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 2ed5f2a0879d..11715f7323c0 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5054,7 +5054,7 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > * function. > */ > if (!node_state(node, N_NORMAL_MEMORY)) > - tmp = -1; > + tmp = NUMA_NO_NODE; > pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp); > if (!pn) > return 1; > -- > 2.33.1 > >
On Fri, Jan 14, 2022 at 09:51:31AM +0100, Michal Hocko wrote: >On Fri 14-01-22 00:29:37, Wei Yang wrote: >> On Wed, Jan 12, 2022 at 09:56:15AM +0100, Michal Hocko wrote: >> >On Wed 12-01-22 00:46:34, Wei Yang wrote: >> >> On Tue, Jan 11, 2022 at 09:40:20AM +0100, Michal Hocko wrote: >> >> >On Tue 11-01-22 01:02:59, Wei Yang wrote: >> >> >> Instead of use "-1", let's use NUMA_NO_NODE for consistency. >> >> >> >> >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> >> > >> >> >I am not really sure this is worth it. After the merge window I plan to >> >> >post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. >> >> >> >> Give me some time to understand it :-) >> > >> >Just for the record, here is what I have put on top of that series: >> >> Ok, I got what you try to resolve. I am ok with the following change except >> one point. >> >> >--- >> >>From b7195eba02fe6308a6927450f4630057c05e808e Mon Sep 17 00:00:00 2001 >> >From: Wei Yang <richard.weiyang@gmail.com> >> >Date: Tue, 11 Jan 2022 09:45:25 +0100 >> >Subject: [PATCH] memcg: do not tweak node in alloc_mem_cgroup_per_node_info >> > >> >alloc_mem_cgroup_per_node_info is allocated for each possible node and >> >this used to be a problem because not !node_online nodes didn't have >> >appropriate data structure allocated. This has changed by "mm: handle >> >uninitialized numa nodes gracefully" so we can drop the special casing >> >here. >> > >> >Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> >Signed-off-by: Michal Hocko <mhocko@suse.com> >> >--- >> > mm/memcontrol.c | 14 ++------------ >> > 1 file changed, 2 insertions(+), 12 deletions(-) >> > >> >diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> >index 781605e92015..ed19a21ee14e 100644 >> >--- a/mm/memcontrol.c >> >+++ b/mm/memcontrol.c >> >@@ -5044,18 +5044,8 @@ struct mem_cgroup *mem_cgroup_from_id(unsigned short id) >> > static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) >> > { >> > struct mem_cgroup_per_node *pn; >> >- int tmp = node; >> >- /* >> >- * This routine is called against possible nodes. >> >- * But it's BUG to call kmalloc() against offline node. >> >- * >> >- * TODO: this routine can waste much memory for nodes which will >> >- * never be onlined. It's better to use memory hotplug callback >> >- * function. >> >- */ >> >> Do you think this TODO is not related to this change? > >It is not really related but I am not sure how useful it is. Essentially >any allocation that is per-possible node is in the same situation and if >we really need to deal with large and sparse possible nodes masks. > Sounds reasonable :-) >If you want me to keep the TODO I will do it though. > >-- >Michal Hocko >SUSE Labs
On Wed, Jan 12, 2022 at 12:56 AM Michal Hocko <mhocko@suse.com> wrote: > > On Wed 12-01-22 00:46:34, Wei Yang wrote: > > On Tue, Jan 11, 2022 at 09:40:20AM +0100, Michal Hocko wrote: > > >On Tue 11-01-22 01:02:59, Wei Yang wrote: > > >> Instead of use "-1", let's use NUMA_NO_NODE for consistency. > > >> > > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > > > > > >I am not really sure this is worth it. After the merge window I plan to > > >post http://lkml.kernel.org/r/20211214100732.26335-1-mhocko@kernel.org. > > > > Give me some time to understand it :-) > > Just for the record, here is what I have put on top of that series: > --- > From b7195eba02fe6308a6927450f4630057c05e808e Mon Sep 17 00:00:00 2001 > From: Wei Yang <richard.weiyang@gmail.com> > Date: Tue, 11 Jan 2022 09:45:25 +0100 > Subject: [PATCH] memcg: do not tweak node in alloc_mem_cgroup_per_node_info > > alloc_mem_cgroup_per_node_info is allocated for each possible node and > this used to be a problem because not !node_online nodes didn't have > appropriate data structure allocated. This has changed by "mm: handle > uninitialized numa nodes gracefully" so we can drop the special casing > here. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > Signed-off-by: Michal Hocko <mhocko@suse.com> Reviewed-by: Shakeel Butt <shakeelb@google.com>
Hi, Andrew Would you pick up this patch set, or prefer me to send a v2? On Tue, Jan 11, 2022 at 01:02:59AM +0000, Wei Yang wrote: >Instead of use "-1", let's use NUMA_NO_NODE for consistency. > >Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >--- > mm/memcontrol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/mm/memcontrol.c b/mm/memcontrol.c >index 2ed5f2a0879d..11715f7323c0 100644 >--- a/mm/memcontrol.c >+++ b/mm/memcontrol.c >@@ -5054,7 +5054,7 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) > * function. > */ > if (!node_state(node, N_NORMAL_MEMORY)) >- tmp = -1; >+ tmp = NUMA_NO_NODE; > pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp); > if (!pn) > return 1; >-- >2.33.1
On Mon, 31 Jan 2022 01:47:42 +0000 Wei Yang <richard.weiyang@gmail.com> wrote: > Hi, Andrew > > Would you pick up this patch set, or prefer me to send a v2? > It's unclear to me what's happening with [4/4]. At least a new changelog with more justification is expected? So yes, please resend?
On Mon, Jan 31, 2022 at 02:36:20PM -0800, Andrew Morton wrote: >On Mon, 31 Jan 2022 01:47:42 +0000 Wei Yang <richard.weiyang@gmail.com> wrote: > >> Hi, Andrew >> >> Would you pick up this patch set, or prefer me to send a v2? >> > >It's unclear to me what's happening with [4/4]. At least a new >changelog with more justification is expected? > As Michal and Roman suggested, this is not a hot-path. I would drop this one. >So yes, please resend? Sure.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 2ed5f2a0879d..11715f7323c0 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5054,7 +5054,7 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) * function. */ if (!node_state(node, N_NORMAL_MEMORY)) - tmp = -1; + tmp = NUMA_NO_NODE; pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp); if (!pn) return 1;
Instead of use "-1", let's use NUMA_NO_NODE for consistency. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)