Message ID | 51D41E34.5010802@huawei.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, Jul 3, 2013 at 2:51 PM, Li Zefan <lizefan@huawei.com> wrote: > On 2013/7/3 20:19, Sedat Dilek wrote: >> When "CONFIG_MEMCG_KMEM=n" I see this in my build-log: >> >> LD init/built-in.o >> mm/built-in.o: In function `mem_cgroup_css_free': >> memcontrol.c:(.text+0x5caa6): undefined reference to `mem_cgroup_sockets_destroy' >> make[2]: *** [vmlinux] Error 1 >> >> Inspired by the ifdef for mem_cgroup_sockets_{init,destroy} here... >> >> [ net/core/sock.c ] >> >> #ifdef CONFIG_MEMCG_KMEM >> int mem_cgroup_sockets_init() >> ... >> void mem_cgroup_sockets_destroy() >> ... >> #endif >> >> ...I did the the same for both in "include/net/sock.h". >> >> This fixes the issue for me in next-20130703. >> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > > Maybe it's better to add memcg_destroy_kmem(), to pair with > memcg_init_kmem(). > > This patch can be folded into "memcg: use css_get/put when charging/uncharging kmem" > Hi Li Zefan, Can you or a guru from netdev explain me why there exists mem_cgroup_sockets_init() and mem_cgroup_sockets_destroy() in 1. net/core/sock.c <--- AFAICS this includes below sock.h, too. 2. net/core/sock.h <--- mm/memcontrol.c is including this one. Make me less confused. And can you explain why your approach is "better" to do the change in memcontrol.c than in sock.h (see sock.c). Thanks in advance. - Sedat - > ======================= > > [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n > > Fix this build error: > > mm/built-in.o: In function `mem_cgroup_css_free': > memcontrol.c:(.text+0x5caa6): undefined reference to > 'mem_cgroup_sockets_destroy' > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Li Zefan <lizefan@huawei.com> > --- > mm/memcontrol.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 234f311..59ea6f9 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5876,6 +5876,11 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > return mem_cgroup_sockets_init(memcg, ss); > } > > +static void memcg_destroy_kmem(struct mem_cgroup *memcg) > +{ > + mem_cgroup_sockets_destroy(memcg); > +} > + > static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > { > if (!memcg_kmem_is_active(memcg)) > @@ -5915,6 +5920,10 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > return 0; > } > > +static void memcg_destroy_kmem(struct mem_cgroup *memcg) > +{ > +} > + > static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > { > } > @@ -6312,8 +6321,7 @@ static void mem_cgroup_css_free(struct cgroup *cont) > { > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); > > - mem_cgroup_sockets_destroy(memcg); > - > + memcg_destroy_kmem(memcg); > __mem_cgroup_free(memcg); > } > > -- > 1.8.0.2 > > >> --- >> [ v2: git dislikes lines beginning with hash ('#'). ] >> >> include/net/sock.h | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/net/sock.h b/include/net/sock.h >> index ea6206c..ad4bf7f 100644 >> --- a/include/net/sock.h >> +++ b/include/net/sock.h >> @@ -71,6 +71,7 @@ >> struct cgroup; >> struct cgroup_subsys; >> #ifdef CONFIG_NET >> +#ifdef CONFIG_MEMCG_KMEM > > #if defined(CONFIG_NET) && defined(CONFIG_MEMCG_KMEM) > >> int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss); >> void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg); >> #else >> @@ -83,7 +84,8 @@ static inline >> void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg) >> { >> } >> -#endif >> +#endif /* CONFIG_NET */ >> +#endif /* CONFIG_MEMCG_KMEM */ >> /* >> * This structure really needs to be cleaned up. >> * Most of it is for TCP, and not used by any of >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 3, 2013 at 3:09 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote: > On Wed, Jul 3, 2013 at 2:51 PM, Li Zefan <lizefan@huawei.com> wrote: >> On 2013/7/3 20:19, Sedat Dilek wrote: >>> When "CONFIG_MEMCG_KMEM=n" I see this in my build-log: >>> >>> LD init/built-in.o >>> mm/built-in.o: In function `mem_cgroup_css_free': >>> memcontrol.c:(.text+0x5caa6): undefined reference to `mem_cgroup_sockets_destroy' >>> make[2]: *** [vmlinux] Error 1 >>> >>> Inspired by the ifdef for mem_cgroup_sockets_{init,destroy} here... >>> >>> [ net/core/sock.c ] >>> >>> #ifdef CONFIG_MEMCG_KMEM >>> int mem_cgroup_sockets_init() >>> ... >>> void mem_cgroup_sockets_destroy() >>> ... >>> #endif >>> >>> ...I did the the same for both in "include/net/sock.h". >>> >>> This fixes the issue for me in next-20130703. >>> >>> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> >> >> Maybe it's better to add memcg_destroy_kmem(), to pair with >> memcg_init_kmem(). >> >> This patch can be folded into "memcg: use css_get/put when charging/uncharging kmem" >> > > Hi Li Zefan, > > Can you or a guru from netdev explain me why there exists > mem_cgroup_sockets_init() and mem_cgroup_sockets_destroy() in > > 1. net/core/sock.c <--- AFAICS this includes below sock.h, too. > 2. net/core/sock.h <--- mm/memcontrol.c is including this one. > *** include/net/sock.h *** - Sedat - > Make me less confused. > > And can you explain why your approach is "better" to do the change in > memcontrol.c than in sock.h (see sock.c). > > Thanks in advance. > > - Sedat - > >> ======================= >> >> [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n >> >> Fix this build error: >> >> mm/built-in.o: In function `mem_cgroup_css_free': >> memcontrol.c:(.text+0x5caa6): undefined reference to >> 'mem_cgroup_sockets_destroy' >> >> Reported-by: Fengguang Wu <fengguang.wu@intel.com> >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> >> Signed-off-by: Li Zefan <lizefan@huawei.com> >> --- >> mm/memcontrol.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 234f311..59ea6f9 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -5876,6 +5876,11 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) >> return mem_cgroup_sockets_init(memcg, ss); >> } >> >> +static void memcg_destroy_kmem(struct mem_cgroup *memcg) >> +{ >> + mem_cgroup_sockets_destroy(memcg); >> +} >> + >> static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) >> { >> if (!memcg_kmem_is_active(memcg)) >> @@ -5915,6 +5920,10 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) >> return 0; >> } >> >> +static void memcg_destroy_kmem(struct mem_cgroup *memcg) >> +{ >> +} >> + >> static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) >> { >> } >> @@ -6312,8 +6321,7 @@ static void mem_cgroup_css_free(struct cgroup *cont) >> { >> struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); >> >> - mem_cgroup_sockets_destroy(memcg); >> - >> + memcg_destroy_kmem(memcg); >> __mem_cgroup_free(memcg); >> } >> >> -- >> 1.8.0.2 >> >> >>> --- >>> [ v2: git dislikes lines beginning with hash ('#'). ] >>> >>> include/net/sock.h | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/net/sock.h b/include/net/sock.h >>> index ea6206c..ad4bf7f 100644 >>> --- a/include/net/sock.h >>> +++ b/include/net/sock.h >>> @@ -71,6 +71,7 @@ >>> struct cgroup; >>> struct cgroup_subsys; >>> #ifdef CONFIG_NET >>> +#ifdef CONFIG_MEMCG_KMEM >> >> #if defined(CONFIG_NET) && defined(CONFIG_MEMCG_KMEM) >> >>> int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss); >>> void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg); >>> #else >>> @@ -83,7 +84,8 @@ static inline >>> void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg) >>> { >>> } >>> -#endif >>> +#endif /* CONFIG_NET */ >>> +#endif /* CONFIG_MEMCG_KMEM */ >>> /* >>> * This structure really needs to be cleaned up. >>> * Most of it is for TCP, and not used by any of >>> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 3, 2013 at 2:51 PM, Li Zefan <lizefan@huawei.com> wrote: > On 2013/7/3 20:19, Sedat Dilek wrote: >> When "CONFIG_MEMCG_KMEM=n" I see this in my build-log: >> >> LD init/built-in.o >> mm/built-in.o: In function `mem_cgroup_css_free': >> memcontrol.c:(.text+0x5caa6): undefined reference to `mem_cgroup_sockets_destroy' >> make[2]: *** [vmlinux] Error 1 >> >> Inspired by the ifdef for mem_cgroup_sockets_{init,destroy} here... >> >> [ net/core/sock.c ] >> >> #ifdef CONFIG_MEMCG_KMEM >> int mem_cgroup_sockets_init() >> ... >> void mem_cgroup_sockets_destroy() >> ... >> #endif >> >> ...I did the the same for both in "include/net/sock.h". >> >> This fixes the issue for me in next-20130703. >> >> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com> > > Maybe it's better to add memcg_destroy_kmem(), to pair with > memcg_init_kmem(). > > This patch can be folded into "memcg: use css_get/put when charging/uncharging kmem" > That text can be placed into the below changelog as it explains what the root-cause is. "memcg: use css_get/put when charging/uncharging kmem" dropped 1st into Linux-next with next-20130703. Please, test your changes also with CONFIG_MEMCG_KMEM=n next time. Thanks! > ======================= > > [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n > > Fix this build error: > > mm/built-in.o: In function `mem_cgroup_css_free': > memcontrol.c:(.text+0x5caa6): undefined reference to > 'mem_cgroup_sockets_destroy' > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Reported-by: Sedat Dilek <sedat.dilek@gmail.com> Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Li Zefan <lizefan@huawei.com> > --- > mm/memcontrol.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 234f311..59ea6f9 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5876,6 +5876,11 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > return mem_cgroup_sockets_init(memcg, ss); > } > > +static void memcg_destroy_kmem(struct mem_cgroup *memcg) > +{ > + mem_cgroup_sockets_destroy(memcg); > +} > + > static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > { > if (!memcg_kmem_is_active(memcg)) > @@ -5915,6 +5920,10 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > return 0; > } > > +static void memcg_destroy_kmem(struct mem_cgroup *memcg) > +{ > +} > + > static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > { > } > @@ -6312,8 +6321,7 @@ static void mem_cgroup_css_free(struct cgroup *cont) > { > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); > > - mem_cgroup_sockets_destroy(memcg); > - > + memcg_destroy_kmem(memcg); > __mem_cgroup_free(memcg); > } > > -- > 1.8.0.2 > > >> --- >> [ v2: git dislikes lines beginning with hash ('#'). ] >> >> include/net/sock.h | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/net/sock.h b/include/net/sock.h >> index ea6206c..ad4bf7f 100644 >> --- a/include/net/sock.h >> +++ b/include/net/sock.h >> @@ -71,6 +71,7 @@ >> struct cgroup; >> struct cgroup_subsys; >> #ifdef CONFIG_NET >> +#ifdef CONFIG_MEMCG_KMEM > > #if defined(CONFIG_NET) && defined(CONFIG_MEMCG_KMEM) > >> int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss); >> void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg); >> #else >> @@ -83,7 +84,8 @@ static inline >> void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg) >> { >> } >> -#endif >> +#endif /* CONFIG_NET */ >> +#endif /* CONFIG_MEMCG_KMEM */ >> /* >> * This structure really needs to be cleaned up. >> * Most of it is for TCP, and not used by any of >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 03-07-13 20:51:00, Li Zefan wrote: [...] > [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n > > Fix this build error: > > mm/built-in.o: In function `mem_cgroup_css_free': > memcontrol.c:(.text+0x5caa6): undefined reference to > 'mem_cgroup_sockets_destroy' > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Li Zefan <lizefan@huawei.com> I am seeing the same thing I just didn't get to reporting it. The other approach is not bad as well but I find this tiny better because mem_cgroup_css_free should care only about a single cleanup function for whole kmem. If that one needs to do tcp kmem specific cleanup then it should be done inside kmem_cgroup_css_offline. Andrew could you add this as memcg-use-css_get-put-when-charging-uncharging-kmem-fix.patch, please? Acked-by: Michal Hocko <mhocko@suse.cz> Thanks > --- > mm/memcontrol.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 234f311..59ea6f9 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5876,6 +5876,11 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > return mem_cgroup_sockets_init(memcg, ss); > } > > +static void memcg_destroy_kmem(struct mem_cgroup *memcg) > +{ > + mem_cgroup_sockets_destroy(memcg); > +} > + > static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > { > if (!memcg_kmem_is_active(memcg)) > @@ -5915,6 +5920,10 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) > return 0; > } > > +static void memcg_destroy_kmem(struct mem_cgroup *memcg) > +{ > +} > + > static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) > { > } > @@ -6312,8 +6321,7 @@ static void mem_cgroup_css_free(struct cgroup *cont) > { > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); > > - mem_cgroup_sockets_destroy(memcg); > - > + memcg_destroy_kmem(memcg); > __mem_cgroup_free(memcg); > } > > -- > 1.8.0.2 > > > > --- > > [ v2: git dislikes lines beginning with hash ('#'). ] > > > > include/net/sock.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/sock.h b/include/net/sock.h > > index ea6206c..ad4bf7f 100644 > > --- a/include/net/sock.h > > +++ b/include/net/sock.h > > @@ -71,6 +71,7 @@ > > struct cgroup; > > struct cgroup_subsys; > > #ifdef CONFIG_NET > > +#ifdef CONFIG_MEMCG_KMEM > > #if defined(CONFIG_NET) && defined(CONFIG_MEMCG_KMEM) > > > int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss); > > void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg); > > #else > > @@ -83,7 +84,8 @@ static inline > > void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg) > > { > > } > > -#endif > > +#endif /* CONFIG_NET */ > > +#endif /* CONFIG_MEMCG_KMEM */ > > /* > > * This structure really needs to be cleaned up. > > * Most of it is for TCP, and not used by any of > > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Wed, Jul 3, 2013 at 5:20 PM, Michal Hocko <mhocko@suse.cz> wrote: > On Wed 03-07-13 20:51:00, Li Zefan wrote: > [...] >> [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n >> >> Fix this build error: >> >> mm/built-in.o: In function `mem_cgroup_css_free': >> memcontrol.c:(.text+0x5caa6): undefined reference to >> 'mem_cgroup_sockets_destroy' >> >> Reported-by: Fengguang Wu <fengguang.wu@intel.com> >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> >> Signed-off-by: Li Zefan <lizefan@huawei.com> > > I am seeing the same thing I just didn't get to reporting it. > The other approach is not bad as well but I find this tiny better > because mem_cgroup_css_free should care only about a single cleanup > function for whole kmem. If that one needs to do tcp kmem specific > cleanup then it should be done inside kmem_cgroup_css_offline. > As said in my other mail, for me this makes sense as it is a followup. But, still I don't know why sock.c has is own mem_cgroup_sockets_{init,destroy}. It's interesting, that include/net/sock.h is included by sock.c, too. If they are different, why do both function use the same name? > Andrew could you add this as > memcg-use-css_get-put-when-charging-uncharging-kmem-fix.patch, please? > *-fix-2 as there is *-fix already around. - Sedat - http://ozlabs.org/~akpm/mmotm/broken-out/memcg-use-css_get-put-when-charging-uncharging-kmem-fix.patch > Acked-by: Michal Hocko <mhocko@suse.cz> > > Thanks > >> --- >> mm/memcontrol.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 234f311..59ea6f9 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -5876,6 +5876,11 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) >> return mem_cgroup_sockets_init(memcg, ss); >> } >> >> +static void memcg_destroy_kmem(struct mem_cgroup *memcg) >> +{ >> + mem_cgroup_sockets_destroy(memcg); >> +} >> + >> static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) >> { >> if (!memcg_kmem_is_active(memcg)) >> @@ -5915,6 +5920,10 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) >> return 0; >> } >> >> +static void memcg_destroy_kmem(struct mem_cgroup *memcg) >> +{ >> +} >> + >> static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) >> { >> } >> @@ -6312,8 +6321,7 @@ static void mem_cgroup_css_free(struct cgroup *cont) >> { >> struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); >> >> - mem_cgroup_sockets_destroy(memcg); >> - >> + memcg_destroy_kmem(memcg); >> __mem_cgroup_free(memcg); >> } >> >> -- >> 1.8.0.2 >> >> >> > --- >> > [ v2: git dislikes lines beginning with hash ('#'). ] >> > >> > include/net/sock.h | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/include/net/sock.h b/include/net/sock.h >> > index ea6206c..ad4bf7f 100644 >> > --- a/include/net/sock.h >> > +++ b/include/net/sock.h >> > @@ -71,6 +71,7 @@ >> > struct cgroup; >> > struct cgroup_subsys; >> > #ifdef CONFIG_NET >> > +#ifdef CONFIG_MEMCG_KMEM >> >> #if defined(CONFIG_NET) && defined(CONFIG_MEMCG_KMEM) >> >> > int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss); >> > void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg); >> > #else >> > @@ -83,7 +84,8 @@ static inline >> > void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg) >> > { >> > } >> > -#endif >> > +#endif /* CONFIG_NET */ >> > +#endif /* CONFIG_MEMCG_KMEM */ >> > /* >> > * This structure really needs to be cleaned up. >> > * Most of it is for TCP, and not used by any of >> > >> >> -- >> To unsubscribe, send a message with 'unsubscribe linux-mm' in >> the body to majordomo@kvack.org. For more info on Linux MM, >> see: http://www.linux-mm.org/ . >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > -- > Michal Hocko > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 03-07-13 17:53:21, Sedat Dilek wrote: > On Wed, Jul 3, 2013 at 5:20 PM, Michal Hocko <mhocko@suse.cz> wrote: > > On Wed 03-07-13 20:51:00, Li Zefan wrote: > > [...] > >> [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n > >> > >> Fix this build error: > >> > >> mm/built-in.o: In function `mem_cgroup_css_free': > >> memcontrol.c:(.text+0x5caa6): undefined reference to > >> 'mem_cgroup_sockets_destroy' > >> > >> Reported-by: Fengguang Wu <fengguang.wu@intel.com> > >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > >> Signed-off-by: Li Zefan <lizefan@huawei.com> > > > > I am seeing the same thing I just didn't get to reporting it. > > The other approach is not bad as well but I find this tiny better > > because mem_cgroup_css_free should care only about a single cleanup > > function for whole kmem. If that one needs to do tcp kmem specific > > cleanup then it should be done inside kmem_cgroup_css_offline. > > > > As said in my other mail, for me this makes sense as it is a followup. > > But, still I don't know why sock.c has is own mem_cgroup_sockets_{init,destroy}. That is the only definition AFAICS (except for !CONFIG_NET where it expands to NOOP). Please note that memcg_init_kmem is a common kmem initializator and it needs to be prepared for !CONFIG_NET. The same applies to _destroy. Makes more sense now? [...]
On Wed, Jul 3, 2013 at 5:59 PM, Michal Hocko <mhocko@suse.cz> wrote: > On Wed 03-07-13 17:53:21, Sedat Dilek wrote: >> On Wed, Jul 3, 2013 at 5:20 PM, Michal Hocko <mhocko@suse.cz> wrote: >> > On Wed 03-07-13 20:51:00, Li Zefan wrote: >> > [...] >> >> [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n >> >> >> >> Fix this build error: >> >> >> >> mm/built-in.o: In function `mem_cgroup_css_free': >> >> memcontrol.c:(.text+0x5caa6): undefined reference to >> >> 'mem_cgroup_sockets_destroy' >> >> >> >> Reported-by: Fengguang Wu <fengguang.wu@intel.com> >> >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> >> >> Signed-off-by: Li Zefan <lizefan@huawei.com> >> > >> > I am seeing the same thing I just didn't get to reporting it. >> > The other approach is not bad as well but I find this tiny better >> > because mem_cgroup_css_free should care only about a single cleanup >> > function for whole kmem. If that one needs to do tcp kmem specific >> > cleanup then it should be done inside kmem_cgroup_css_offline. >> > >> >> As said in my other mail, for me this makes sense as it is a followup. >> >> But, still I don't know why sock.c has is own mem_cgroup_sockets_{init,destroy}. > > That is the only definition AFAICS (except for !CONFIG_NET where it > expands to NOOP). Please note that memcg_init_kmem is a common kmem > initializator and it needs to be prepared for !CONFIG_NET. > > The same applies to _destroy. > Makes more sense now? > So, that stuff comes originally from the net-tree. I understand the !CONFIG_NET case, but lack the understanding why memcontrol.c needs _destroy. Can you explain that (sorry /me is no mm-geek)? - Sedat - [1] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/net/core/sock.c?id=next-20130703#n147 [2] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/include/net/sock.h?id=next-20130703#n73 > [...] > -- > Michal Hocko > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 03-07-13 18:11:28, Sedat Dilek wrote: > On Wed, Jul 3, 2013 at 5:59 PM, Michal Hocko <mhocko@suse.cz> wrote: > > On Wed 03-07-13 17:53:21, Sedat Dilek wrote: > >> On Wed, Jul 3, 2013 at 5:20 PM, Michal Hocko <mhocko@suse.cz> wrote: > >> > On Wed 03-07-13 20:51:00, Li Zefan wrote: > >> > [...] > >> >> [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n > >> >> > >> >> Fix this build error: > >> >> > >> >> mm/built-in.o: In function `mem_cgroup_css_free': > >> >> memcontrol.c:(.text+0x5caa6): undefined reference to > >> >> 'mem_cgroup_sockets_destroy' > >> >> > >> >> Reported-by: Fengguang Wu <fengguang.wu@intel.com> > >> >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > >> >> Signed-off-by: Li Zefan <lizefan@huawei.com> > >> > > >> > I am seeing the same thing I just didn't get to reporting it. > >> > The other approach is not bad as well but I find this tiny better > >> > because mem_cgroup_css_free should care only about a single cleanup > >> > function for whole kmem. If that one needs to do tcp kmem specific > >> > cleanup then it should be done inside kmem_cgroup_css_offline. > >> > > >> > >> As said in my other mail, for me this makes sense as it is a followup. > >> > >> But, still I don't know why sock.c has is own mem_cgroup_sockets_{init,destroy}. > > > > That is the only definition AFAICS (except for !CONFIG_NET where it > > expands to NOOP). Please note that memcg_init_kmem is a common kmem > > initializator and it needs to be prepared for !CONFIG_NET. > > > > The same applies to _destroy. > > Makes more sense now? > > > > So, that stuff comes originally from the net-tree. No, it all came from tcp kmem accounting. It is a memcg thingy and I guess it was placed into sock.c because it depends on some static symbols there (e.g. proto_list_mutex). > I understand the !CONFIG_NET case, but lack the understanding why > memcontrol.c needs _destroy. Because it is memcg specific and it has to be called when a group is destroyed. > Can you explain that (sorry /me is no mm-geek)? > > - Sedat - > > [1] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/net/core/sock.c?id=next-20130703#n147 > [2] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/include/net/sock.h?id=next-20130703#n73 > > > [...] > > -- > > Michal Hocko > > SUSE Labs
On Wed, Jul 3, 2013 at 6:34 PM, Michal Hocko <mhocko@suse.cz> wrote: > On Wed 03-07-13 18:11:28, Sedat Dilek wrote: >> On Wed, Jul 3, 2013 at 5:59 PM, Michal Hocko <mhocko@suse.cz> wrote: >> > On Wed 03-07-13 17:53:21, Sedat Dilek wrote: >> >> On Wed, Jul 3, 2013 at 5:20 PM, Michal Hocko <mhocko@suse.cz> wrote: >> >> > On Wed 03-07-13 20:51:00, Li Zefan wrote: >> >> > [...] >> >> >> [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n >> >> >> >> >> >> Fix this build error: >> >> >> >> >> >> mm/built-in.o: In function `mem_cgroup_css_free': >> >> >> memcontrol.c:(.text+0x5caa6): undefined reference to >> >> >> 'mem_cgroup_sockets_destroy' >> >> >> >> >> >> Reported-by: Fengguang Wu <fengguang.wu@intel.com> >> >> >> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> >> >> >> Signed-off-by: Li Zefan <lizefan@huawei.com> >> >> > >> >> > I am seeing the same thing I just didn't get to reporting it. >> >> > The other approach is not bad as well but I find this tiny better >> >> > because mem_cgroup_css_free should care only about a single cleanup >> >> > function for whole kmem. If that one needs to do tcp kmem specific >> >> > cleanup then it should be done inside kmem_cgroup_css_offline. >> >> > >> >> >> >> As said in my other mail, for me this makes sense as it is a followup. >> >> >> >> But, still I don't know why sock.c has is own mem_cgroup_sockets_{init,destroy}. >> > >> > That is the only definition AFAICS (except for !CONFIG_NET where it >> > expands to NOOP). Please note that memcg_init_kmem is a common kmem >> > initializator and it needs to be prepared for !CONFIG_NET. >> > >> > The same applies to _destroy. >> > Makes more sense now? >> > >> >> So, that stuff comes originally from the net-tree. > > No, it all came from tcp kmem accounting. It is a memcg thingy and I > guess it was placed into sock.c because it depends on some static > symbols there (e.g. proto_list_mutex). > memcg thingies should belong to mm-tree :-). >> I understand the !CONFIG_NET case, but lack the understanding why >> memcontrol.c needs _destroy. > > Because it is memcg specific and it has to be called when a group is > destroyed. > I looked again into my local GIT tree where I applied Li Zefan's patch. memcg_destroy_kmem() makes sense with existing memcg_init_kmem(). I have now a better picture, but it's balck/white not coloured :-). Thanks for your patience and explanations. - Sedat - >> Can you explain that (sorry /me is no mm-geek)? >> >> - Sedat - >> >> [1] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/net/core/sock.c?id=next-20130703#n147 >> [2] http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/include/net/sock.h?id=next-20130703#n73 >> >> > [...] >> > -- >> > Michal Hocko >> > SUSE Labs > > -- > Michal Hocko > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
======================= [PATCH] memcg: fix build error if CONFIG_MEMCG_KMEM=n Fix this build error: mm/built-in.o: In function `mem_cgroup_css_free': memcontrol.c:(.text+0x5caa6): undefined reference to 'mem_cgroup_sockets_destroy' Reported-by: Fengguang Wu <fengguang.wu@intel.com> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: Li Zefan <lizefan@huawei.com> --- mm/memcontrol.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 234f311..59ea6f9 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5876,6 +5876,11 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) return mem_cgroup_sockets_init(memcg, ss); } +static void memcg_destroy_kmem(struct mem_cgroup *memcg) +{ + mem_cgroup_sockets_destroy(memcg); +} + static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) { if (!memcg_kmem_is_active(memcg)) @@ -5915,6 +5920,10 @@ static int memcg_init_kmem(struct mem_cgroup *memcg, struct cgroup_subsys *ss) return 0; } +static void memcg_destroy_kmem(struct mem_cgroup *memcg) +{ +} + static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) { } @@ -6312,8 +6321,7 @@ static void mem_cgroup_css_free(struct cgroup *cont) { struct mem_cgroup *memcg = mem_cgroup_from_cont(cont); - mem_cgroup_sockets_destroy(memcg); - + memcg_destroy_kmem(memcg); __mem_cgroup_free(memcg); }