diff mbox

[next-20130703] net: sock: Add ifdef CONFIG_MEMCG_KMEM for mem_cgroup_sockets_{init,destroy}

Message ID 51D41E34.5010802@huawei.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Zefan Li July 3, 2013, 12:51 p.m. UTC 
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"

Comments

Sedat Dilek July 3, 2013, 1:09 p.m. UTC | #1 
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
Sedat Dilek July 3, 2013, 1:10 p.m. UTC | #2 
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
Sedat Dilek July 3, 2013, 1:20 p.m. UTC | #3 
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
Michal Hocko July 3, 2013, 3:20 p.m. UTC | #4 
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>
Sedat Dilek July 3, 2013, 3:53 p.m. UTC | #5 
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
Michal Hocko July 3, 2013, 3:59 p.m. UTC | #6 
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?

[...]
Sedat Dilek July 3, 2013, 4:11 p.m. UTC | #7 
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
Michal Hocko July 3, 2013, 4:34 p.m. UTC | #8 
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
Sedat Dilek July 3, 2013, 4:42 p.m. UTC | #9 
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
diff mbox

Patch

=======================

[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);
 }