Re: [PATCH] mm: memcontrol: only manage socket pressure for CONFIG_INET
Hi,
I hit this while I was testing 4.5-rc1 with randconfig during merger period.
And now I noticed that it was fixed after Linus merged akpm branch.
commit eae21770b4fed5597623aad0d618190fa60426ff
Merge: e9f57eb 9f273c2
Author: Linus Torvalds
Date: Thu Jan 21 12:32:08 2016 -0800
Merge branch 'akpm' (patches from Andrew)
Try one commit before this (commit e9f57ebcba563e0cd532926cab83c92bb4d79360 )
DOES have an issue.
So I believe it was fixed for now.
Thanks
Masanari
On Thu, Dec 10, 2015 at 8:13 AM, Andrew Morton
wrote:
> On Wed, 9 Dec 2015 18:05:05 -0500 Johannes Weiner wrote:
>
>> On Wed, Dec 09, 2015 at 02:28:36PM -0800, Andrew Morton wrote:
>> > On Wed, 9 Dec 2015 13:58:58 -0500 Johannes Weiner
>> > wrote:
>> > > The calls to tcp_init_cgroup() appear earlier in the series than "mm:
>> > > memcontrol: hook up vmpressure to socket pressure". However, they get
>> > > moved around a few times so fixing it earlier means respinning the
>> > > series. Andrew, it's up to you whether we take the bisectability hit
>> > > for !CONFIG_INET && CONFIG_MEMCG (how common is this?) or whether you
>> > > want me to resend the series.
>> >
>> > hm, drat, I was suspecting dependency issues here, but a test build
>> > said it was OK.
>> >
>> > Actually, I was expecting this patch series to depend on the linux-next
>> > cgroup2 changes, but that doesn't appear to be the case. *should* this
>> > series be staged after the cgroup2 code?
>>
>> Code-wise they are independent. My stuff is finishing up the new memcg
>> control knobs, the cgroup2 stuff is changing how and when those knobs
>> are exposed from within the cgroup core. I'm not relying on any recent
>> changes in the cgroup core AFAICS, so the order shouldn't matter here.
>
> OK, thanks.
>
>> > Regarding this particular series: yes, I think we can live with a
>> > bisection hole for !CONFIG_INET && CONFIG_MEMCG users. But I'm not
>> > sure why we're discussing bisection issues, because Arnd's build
>> > failure occurs with everything applied?
>>
>> Arnd's patches apply to the top of the stack, but they address issues
>> introduced early in the series and the problematic code gets touched a
>> lot in subsequent patches. E.g. the first build breakage is in ("net:
>> tcp_memcontrol: simplify linkage between socket and page counter")
>> when the tcp_init_cgroup() and tcp_destroy_cgroup() function calls get
>> moved around and lose the CONFIG_INET protection.
>
> Yeah, this is a pain. I think I'll fold Arnd's fix into
> mm-memcontrol-introduce-config_memcg_legacy_kmem.patch (which is staged
> after all the other MM patches and after linux-next) and will pretend I
> didn't know about the issue ;)
>
>> Anyway, if we can live with the bisection caveat then Arnd's fixes on
>> top of the kmem series look good to me. Depending on what Vladimir
>> thinks we might want to replace the CONFIG_SLOB fix with something
>> else later on, but that shouldn't be a problem, either.
>
> I don't have a fix for the CONFIG_SLOB&&CONFIG_MEMCG issue yet. I
> agree that it would be best to make the combination work correctly
> rather than banning it, but that does require a bit of runtime testing.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: memcontrol: only manage socket pressure for CONFIG_INET
On Wed, 9 Dec 2015 18:05:05 -0500 Johannes Weiner wrote:
> On Wed, Dec 09, 2015 at 02:28:36PM -0800, Andrew Morton wrote:
> > On Wed, 9 Dec 2015 13:58:58 -0500 Johannes Weiner
> > wrote:
> > > The calls to tcp_init_cgroup() appear earlier in the series than "mm:
> > > memcontrol: hook up vmpressure to socket pressure". However, they get
> > > moved around a few times so fixing it earlier means respinning the
> > > series. Andrew, it's up to you whether we take the bisectability hit
> > > for !CONFIG_INET && CONFIG_MEMCG (how common is this?) or whether you
> > > want me to resend the series.
> >
> > hm, drat, I was suspecting dependency issues here, but a test build
> > said it was OK.
> >
> > Actually, I was expecting this patch series to depend on the linux-next
> > cgroup2 changes, but that doesn't appear to be the case. *should* this
> > series be staged after the cgroup2 code?
>
> Code-wise they are independent. My stuff is finishing up the new memcg
> control knobs, the cgroup2 stuff is changing how and when those knobs
> are exposed from within the cgroup core. I'm not relying on any recent
> changes in the cgroup core AFAICS, so the order shouldn't matter here.
OK, thanks.
> > Regarding this particular series: yes, I think we can live with a
> > bisection hole for !CONFIG_INET && CONFIG_MEMCG users. But I'm not
> > sure why we're discussing bisection issues, because Arnd's build
> > failure occurs with everything applied?
>
> Arnd's patches apply to the top of the stack, but they address issues
> introduced early in the series and the problematic code gets touched a
> lot in subsequent patches. E.g. the first build breakage is in ("net:
> tcp_memcontrol: simplify linkage between socket and page counter")
> when the tcp_init_cgroup() and tcp_destroy_cgroup() function calls get
> moved around and lose the CONFIG_INET protection.
Yeah, this is a pain. I think I'll fold Arnd's fix into
mm-memcontrol-introduce-config_memcg_legacy_kmem.patch (which is staged
after all the other MM patches and after linux-next) and will pretend I
didn't know about the issue ;)
> Anyway, if we can live with the bisection caveat then Arnd's fixes on
> top of the kmem series look good to me. Depending on what Vladimir
> thinks we might want to replace the CONFIG_SLOB fix with something
> else later on, but that shouldn't be a problem, either.
I don't have a fix for the CONFIG_SLOB&&CONFIG_MEMCG issue yet. I
agree that it would be best to make the combination work correctly
rather than banning it, but that does require a bit of runtime testing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: memcontrol: only manage socket pressure for CONFIG_INET
On Wed, Dec 09, 2015 at 02:28:36PM -0800, Andrew Morton wrote:
> On Wed, 9 Dec 2015 13:58:58 -0500 Johannes Weiner wrote:
>
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 6faea81e66d7..73cd572167bb 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -4220,13 +4220,13 @@ mem_cgroup_css_online(struct cgroup_subsys_state
> > > *css)
> > > if (ret)
> > > return ret;
> > >
> > > +#ifdef CONFIG_INET
> > > #ifdef CONFIG_MEMCG_LEGACY_KMEM
> > > ret = tcp_init_cgroup(memcg);
> > > if (ret)
> > > return ret;
> > > #endif
> >
> > The calls to tcp_init_cgroup() appear earlier in the series than "mm:
> > memcontrol: hook up vmpressure to socket pressure". However, they get
> > moved around a few times so fixing it earlier means respinning the
> > series. Andrew, it's up to you whether we take the bisectability hit
> > for !CONFIG_INET && CONFIG_MEMCG (how common is this?) or whether you
> > want me to resend the series.
>
> hm, drat, I was suspecting dependency issues here, but a test build
> said it was OK.
>
> Actually, I was expecting this patch series to depend on the linux-next
> cgroup2 changes, but that doesn't appear to be the case. *should* this
> series be staged after the cgroup2 code?
Code-wise they are independent. My stuff is finishing up the new memcg
control knobs, the cgroup2 stuff is changing how and when those knobs
are exposed from within the cgroup core. I'm not relying on any recent
changes in the cgroup core AFAICS, so the order shouldn't matter here.
> Regarding this particular series: yes, I think we can live with a
> bisection hole for !CONFIG_INET && CONFIG_MEMCG users. But I'm not
> sure why we're discussing bisection issues, because Arnd's build
> failure occurs with everything applied?
Arnd's patches apply to the top of the stack, but they address issues
introduced early in the series and the problematic code gets touched a
lot in subsequent patches. E.g. the first build breakage is in ("net:
tcp_memcontrol: simplify linkage between socket and page counter")
when the tcp_init_cgroup() and tcp_destroy_cgroup() function calls get
moved around and lose the CONFIG_INET protection.
This will leave states in between broken for this configuration, which
is why I brought up bisection. Or did you mean something else?
> > Sorry about the trouble. I don't have a git tree on kernel.org because
> > we don't really use git in -mm, but the downside is that we don't get
> > the benefits of the automatic build testing for all kinds of configs.
> > I'll try to set up a git tree to expose series to full build coverage
> > before they hit -mm and -next.
>
> This sort of thing happens quite rarely.
True, this is a particularly tedious one. The only reason I brought it
up is because I use git to prepare patches anyway, and pushing patches
into a branch reachable by Fengguang's rig before I send emails might
have caught this stuff without spamming so many inboxes ;)
Anyway, if we can live with the bisection caveat then Arnd's fixes on
top of the kmem series look good to me. Depending on what Vladimir
thinks we might want to replace the CONFIG_SLOB fix with something
else later on, but that shouldn't be a problem, either.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: memcontrol: only manage socket pressure for CONFIG_INET
On Wed, 9 Dec 2015 13:58:58 -0500 Johannes Weiner wrote: > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 6faea81e66d7..73cd572167bb 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -4220,13 +4220,13 @@ mem_cgroup_css_online(struct cgroup_subsys_state > > *css) > > if (ret) > > return ret; > > > > +#ifdef CONFIG_INET > > #ifdef CONFIG_MEMCG_LEGACY_KMEM > > ret = tcp_init_cgroup(memcg); > > if (ret) > > return ret; > > #endif > > The calls to tcp_init_cgroup() appear earlier in the series than "mm: > memcontrol: hook up vmpressure to socket pressure". However, they get > moved around a few times so fixing it earlier means respinning the > series. Andrew, it's up to you whether we take the bisectability hit > for !CONFIG_INET && CONFIG_MEMCG (how common is this?) or whether you > want me to resend the series. hm, drat, I was suspecting dependency issues here, but a test build said it was OK. Actually, I was expecting this patch series to depend on the linux-next cgroup2 changes, but that doesn't appear to be the case. *should* this series be staged after the cgroup2 code? Regarding this particular series: yes, I think we can live with a bisection hole for !CONFIG_INET && CONFIG_MEMCG users. But I'm not sure why we're discussing bisection issues, because Arnd's build failure occurs with everything applied? > Sorry about the trouble. I don't have a git tree on kernel.org because > we don't really use git in -mm, but the downside is that we don't get > the benefits of the automatic build testing for all kinds of configs. > I'll try to set up a git tree to expose series to full build coverage > before they hit -mm and -next. This sort of thing happens quite rarely. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: memcontrol: only manage socket pressure for CONFIG_INET
On Wed, Dec 09, 2015 at 05:32:16PM +0100, Arnd Bergmann wrote:
> When IPV4 support is disabled, the memcg->socket_pressure field is
> not defined and we get a build error from the vmpressure code:
>
> mm/vmpressure.c: In function 'vmpressure':
> mm/vmpressure.c:287:9: error: 'struct mem_cgroup' has no member named
> 'socket_pressure'
> memcg->socket_pressure = jiffies + HZ;
> mm/built-in.o: In function `mem_cgroup_css_free':
> :(.text+0x1c03a): undefined reference to `tcp_destroy_cgroup'
> mm/built-in.o: In function `mem_cgroup_css_online':
> :(.text+0x1c20e): undefined reference to `tcp_init_cgroup'
>
> This puts the code causing this in the same #ifdef that guards the
> struct member and the TCP implementation.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 20cc40e66c42 ("mm: memcontrol: hook up vmpressure to socket pressure")
Acked-by: Johannes Weiner
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6faea81e66d7..73cd572167bb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4220,13 +4220,13 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
> if (ret)
> return ret;
>
> +#ifdef CONFIG_INET
> #ifdef CONFIG_MEMCG_LEGACY_KMEM
> ret = tcp_init_cgroup(memcg);
> if (ret)
> return ret;
> #endif
The calls to tcp_init_cgroup() appear earlier in the series than "mm:
memcontrol: hook up vmpressure to socket pressure". However, they get
moved around a few times so fixing it earlier means respinning the
series. Andrew, it's up to you whether we take the bisectability hit
for !CONFIG_INET && CONFIG_MEMCG (how common is this?) or whether you
want me to resend the series.
Sorry about the trouble. I don't have a git tree on kernel.org because
we don't really use git in -mm, but the downside is that we don't get
the benefits of the automatic build testing for all kinds of configs.
I'll try to set up a git tree to expose series to full build coverage
before they hit -mm and -next.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mm: memcontrol: only manage socket pressure for CONFIG_INET
When IPV4 support is disabled, the memcg->socket_pressure field is
not defined and we get a build error from the vmpressure code:
mm/vmpressure.c: In function 'vmpressure':
mm/vmpressure.c:287:9: error: 'struct mem_cgroup' has no member named
'socket_pressure'
memcg->socket_pressure = jiffies + HZ;
mm/built-in.o: In function `mem_cgroup_css_free':
:(.text+0x1c03a): undefined reference to `tcp_destroy_cgroup'
mm/built-in.o: In function `mem_cgroup_css_online':
:(.text+0x1c20e): undefined reference to `tcp_init_cgroup'
This puts the code causing this in the same #ifdef that guards the
struct member and the TCP implementation.
Signed-off-by: Arnd Bergmann
Fixes: 20cc40e66c42 ("mm: memcontrol: hook up vmpressure to socket pressure")
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6faea81e66d7..73cd572167bb 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4220,13 +4220,13 @@ mem_cgroup_css_online(struct cgroup_subsys_state *css)
if (ret)
return ret;
+#ifdef CONFIG_INET
#ifdef CONFIG_MEMCG_LEGACY_KMEM
ret = tcp_init_cgroup(memcg);
if (ret)
return ret;
#endif
-#ifdef CONFIG_INET
if (cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_nosocket)
static_branch_inc(&memcg_sockets_enabled_key);
#endif
@@ -4276,7 +4276,7 @@ static void mem_cgroup_css_free(struct
cgroup_subsys_state *css)
memcg_free_kmem(memcg);
-#ifdef CONFIG_MEMCG_LEGACY_KMEM
+#if defined(CONFIG_MEMCG_LEGACY_KMEM) && defined(CONFIG_INET)
tcp_destroy_cgroup(memcg);
#endif
diff --git a/mm/vmpressure.c b/mm/vmpressure.c
index 506f03e4be47..8cdeebe48848 100644
--- a/mm/vmpressure.c
+++ b/mm/vmpressure.c
@@ -275,6 +275,7 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool
tree,
level = vmpressure_calc_level(scanned, reclaimed);
+#ifdef CONFIG_INET
if (level > VMPRESSURE_LOW) {
/*
* Let the socket buffer allocator know that
@@ -286,6 +287,7 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool
tree,
*/
memcg->socket_pressure = jiffies + HZ;
}
+#endif
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
