Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
[Fixup Vladimir's email] same here I do not feel familiar with the code enough to give my ack but Vladimir might be in a better position On Wed 14-09-16 15:48:46, Johannes Weiner wrote: > The cgroup core and the memory controller need to track socket > ownership for different purposes, but the tracking sites being > entirely different is kind of ugly. > > Be a better citizen and rename the memory controller callbacks to > match the cgroup core callbacks, then move them to the same place. > > Signed-off-by: Johannes Weiner > --- > include/linux/memcontrol.h | 4 ++-- > mm/memcontrol.c| 19 +++ > net/core/sock.c| 6 +++--- > net/ipv4/tcp.c | 2 -- > net/ipv4/tcp_ipv4.c| 3 --- > 5 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 0710143723bc..ca11b3e6dd65 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -773,8 +773,8 @@ static inline void mem_cgroup_wb_stats(struct > bdi_writeback *wb, > #endif /* CONFIG_CGROUP_WRITEBACK */ > > struct sock; > -void sock_update_memcg(struct sock *sk); > -void sock_release_memcg(struct sock *sk); > +void mem_cgroup_sk_alloc(struct sock *sk); > +void mem_cgroup_sk_free(struct sock *sk); > bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int > nr_pages); > void mem_cgroup_uncharge_skmem(struct mem_cgroup *memcg, unsigned int > nr_pages); > #ifdef CONFIG_MEMCG > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 60bb830abc34..2caf1ee86e78 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2939,7 +2939,7 @@ static int memcg_update_tcp_limit(struct mem_cgroup > *memcg, unsigned long limit) > /* >* The active flag needs to be written after the static_key >* update. This is what guarantees that the socket activation > - * function is the last one to run. See sock_update_memcg() for > + * function is the last one to run. See mem_cgroup_sk_alloc() > for >* details, and note that we don't mark any socket as belonging >* to this memcg until that flag is up. >* > @@ -2948,7 +2948,7 @@ static int memcg_update_tcp_limit(struct mem_cgroup > *memcg, unsigned long limit) >* as accounted, but the accounting functions are not patched in >* yet, we'll lose accounting. >* > - * We never race with the readers in sock_update_memcg(), > + * We never race with the readers in mem_cgroup_sk_alloc(), >* because when this value change, the code to process it is not >* patched in yet. >*/ > @@ -5651,11 +5651,15 @@ void mem_cgroup_migrate(struct page *oldpage, struct > page *newpage) > DEFINE_STATIC_KEY_FALSE(memcg_sockets_enabled_key); > EXPORT_SYMBOL(memcg_sockets_enabled_key); > > -void sock_update_memcg(struct sock *sk) > +void mem_cgroup_sk_alloc(struct sock *sk) > { > struct mem_cgroup *memcg; > > - /* Socket cloning can throw us here with sk_cgrp already > + if (!mem_cgroup_sockets_enabled) > + return; > + > + /* > + * Socket cloning can throw us here with sk_memcg already >* filled. It won't however, necessarily happen from >* process context. So the test for root memcg given >* the current task's memcg won't help us in this case. > @@ -5680,12 +5684,11 @@ void sock_update_memcg(struct sock *sk) > out: > rcu_read_unlock(); > } > -EXPORT_SYMBOL(sock_update_memcg); > > -void sock_release_memcg(struct sock *sk) > +void mem_cgroup_sk_free(struct sock *sk) > { > - WARN_ON(!sk->sk_memcg); > - css_put(&sk->sk_memcg->css); > + if (sk->sk_memcg) > + css_put(&sk->sk_memcg->css); > } > > /** > diff --git a/net/core/sock.c b/net/core/sock.c > index 038e660ef844..c73e28fc9c2a 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1363,6 +1363,7 @@ static void sk_prot_free(struct proto *prot, struct > sock *sk) > slab = prot->slab; > > cgroup_sk_free(&sk->sk_cgrp_data); > + mem_cgroup_sk_free(sk); > security_sk_free(sk); > if (slab != NULL) > kmem_cache_free(slab, sk); > @@ -1399,6 +1400,7 @@ struct sock *sk_alloc(struct net *net, int family, > gfp_t priority, > sock_net_set(sk, net); > atomic_set(&sk->sk_wmem_alloc, 1); > > + mem_cgroup_sk_alloc(sk); > cgroup_sk_alloc(&sk->sk_cgrp_data); > sock_update_classid(&sk->sk_cgrp_data); > sock_update_netprioidx(&sk->sk_cgrp_data); > @@ -1545,6 +1547,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const > gfp_t priority) > newsk->sk_incoming_cpu = raw_smp_processor_id(); > atomic64_set(&newsk->sk_cookie, 0); > > + mem_
Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
On Wed, Sep 14, 2016 at 03:17:14PM -0700, Andrew Morton wrote: > On Thu, 15 Sep 2016 13:34:24 +0800 kbuild test robot wrote: > > > Hi Johannes, > > > > [auto build test ERROR on net/master] > > [also build test ERROR on v4.8-rc6 next-20160914] > > [if your patch is applied to the wrong git tree, please drop us a note to > > help improve the system] > > [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto > > for convenience) to record what (public, well-known) commit your patch > > series was built on] > > [Check https://git-scm.com/docs/git-format-patch for more information] > > > > url: > > https://github.com/0day-ci/linux/commits/Johannes-Weiner/mm-memcontrol-make-per-cpu-charge-cache-IRQ-safe-for-socket-accounting/20160915-035634 > > config: m68k-sun3_defconfig (attached as .config) > > compiler: m68k-linux-gcc (GCC) 4.9.0 > > reproduce: > > wget > > https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross > > -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # save the attached .config to linux build tree > > make.cross ARCH=m68k > > > > All errors (new ones prefixed by >>): > > > >net/built-in.o: In function `sk_alloc': > > >> (.text+0x4076): undefined reference to `mem_cgroup_sk_alloc' > >net/built-in.o: In function `__sk_destruct': > > >> sock.c:(.text+0x457e): undefined reference to `mem_cgroup_sk_free' > >net/built-in.o: In function `sk_clone_lock': > >(.text+0x4f1c): undefined reference to `mem_cgroup_sk_alloc' > > This? Thanks for fixing it up, Andrew. I think it'd be nicer to declare the dummy functions for !CONFIG_MEMCG; it also doesn't look like a hotpath that would necessitate the jump label in that place. Dave, any preference either way? Signed-off-by: Johannes Weiner --- diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index ca11b3e6dd65..61d20c17f3b7 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -773,13 +773,13 @@ static inline void mem_cgroup_wb_stats(struct bdi_writeback *wb, #endif /* CONFIG_CGROUP_WRITEBACK */ struct sock; -void mem_cgroup_sk_alloc(struct sock *sk); -void mem_cgroup_sk_free(struct sock *sk); bool mem_cgroup_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages); void mem_cgroup_uncharge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages); #ifdef CONFIG_MEMCG extern struct static_key_false memcg_sockets_enabled_key; #define mem_cgroup_sockets_enabled static_branch_unlikely(&memcg_sockets_enabled_key) +void mem_cgroup_sk_alloc(struct sock *sk); +void mem_cgroup_sk_free(struct sock *sk); static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) { if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->tcpmem_pressure) @@ -792,6 +792,8 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) } #else #define mem_cgroup_sockets_enabled 0 +static inline void mem_cgroup_sk_alloc(struct sock *sk) { }; +static inline void mem_cgroup_sk_free(struct sock *sk) { }; static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg) { return false;
Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
On Thu, 15 Sep 2016 13:34:24 +0800 kbuild test robot wrote: > Hi Johannes, > > [auto build test ERROR on net/master] > [also build test ERROR on v4.8-rc6 next-20160914] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for > convenience) to record what (public, well-known) commit your patch series was > built on] > [Check https://git-scm.com/docs/git-format-patch for more information] > > url: > https://github.com/0day-ci/linux/commits/Johannes-Weiner/mm-memcontrol-make-per-cpu-charge-cache-IRQ-safe-for-socket-accounting/20160915-035634 > config: m68k-sun3_defconfig (attached as .config) > compiler: m68k-linux-gcc (GCC) 4.9.0 > reproduce: > wget > https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross > -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=m68k > > All errors (new ones prefixed by >>): > >net/built-in.o: In function `sk_alloc': > >> (.text+0x4076): undefined reference to `mem_cgroup_sk_alloc' >net/built-in.o: In function `__sk_destruct': > >> sock.c:(.text+0x457e): undefined reference to `mem_cgroup_sk_free' >net/built-in.o: In function `sk_clone_lock': >(.text+0x4f1c): undefined reference to `mem_cgroup_sk_alloc' This? --- a/mm/memcontrol.c~mm-memcontrol-consolidate-cgroup-socket-tracking-fix +++ a/mm/memcontrol.c @@ -5655,9 +5655,6 @@ void mem_cgroup_sk_alloc(struct sock *sk { struct mem_cgroup *memcg; - if (!mem_cgroup_sockets_enabled) - return; - /* * Socket cloning can throw us here with sk_memcg already * filled. It won't however, necessarily happen from --- a/net/core/sock.c~mm-memcontrol-consolidate-cgroup-socket-tracking-fix +++ a/net/core/sock.c @@ -1385,7 +1385,8 @@ static void sk_prot_free(struct proto *p slab = prot->slab; cgroup_sk_free(&sk->sk_cgrp_data); - mem_cgroup_sk_free(sk); + if (mem_cgroup_sockets_enabled) + mem_cgroup_sk_free(sk); security_sk_free(sk); if (slab != NULL) kmem_cache_free(slab, sk); @@ -1422,7 +1423,8 @@ struct sock *sk_alloc(struct net *net, i sock_net_set(sk, net); atomic_set(&sk->sk_wmem_alloc, 1); - mem_cgroup_sk_alloc(sk); + if (mem_cgroup_sockets_enabled) + mem_cgroup_sk_alloc(sk); cgroup_sk_alloc(&sk->sk_cgrp_data); sock_update_classid(&sk->sk_cgrp_data); sock_update_netprioidx(&sk->sk_cgrp_data); @@ -1569,7 +1571,8 @@ struct sock *sk_clone_lock(const struct newsk->sk_incoming_cpu = raw_smp_processor_id(); atomic64_set(&newsk->sk_cookie, 0); - mem_cgroup_sk_alloc(newsk); + if (mem_cgroup_sockets_enabled) + mem_cgroup_sk_alloc(newsk); cgroup_sk_alloc(&newsk->sk_cgrp_data); /* _
Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
Hi Johannes, [auto build test ERROR on net/master] [also build test ERROR on v4.8-rc6 next-20160914] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Johannes-Weiner/mm-memcontrol-make-per-cpu-charge-cache-IRQ-safe-for-socket-accounting/20160915-035634 config: m68k-sun3_defconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 4.9.0 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=m68k All errors (new ones prefixed by >>): net/built-in.o: In function `sk_alloc': >> (.text+0x4076): undefined reference to `mem_cgroup_sk_alloc' net/built-in.o: In function `__sk_destruct': >> sock.c:(.text+0x457e): undefined reference to `mem_cgroup_sk_free' net/built-in.o: In function `sk_clone_lock': (.text+0x4f1c): undefined reference to `mem_cgroup_sk_alloc' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
Hi Johannes, [auto build test ERROR on net/master] [also build test ERROR on v4.8-rc6 next-20160914] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Johannes-Weiner/mm-memcontrol-make-per-cpu-charge-cache-IRQ-safe-for-socket-accounting/20160915-035634 config: parisc-c3000_defconfig (attached as .config) compiler: hppa-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=parisc All errors (new ones prefixed by >>): net/built-in.o: In function `sk_alloc': >> (.text.sk_alloc+0x88): undefined reference to `mem_cgroup_sk_alloc' net/built-in.o: In function `__sk_destruct': >> net/core/sock.o:(.text.__sk_destruct+0xbc): undefined reference to >> `mem_cgroup_sk_free' net/built-in.o: In function `sk_clone_lock': >> (.text.sk_clone_lock+0x164): undefined reference to `mem_cgroup_sk_alloc' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
Hi Johannes, [auto build test ERROR on net/master] [also build test ERROR on v4.8-rc6 next-20160914] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Johannes-Weiner/mm-memcontrol-make-per-cpu-charge-cache-IRQ-safe-for-socket-accounting/20160915-035634 config: parisc-b180_defconfig (attached as .config) compiler: hppa-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=parisc All errors (new ones prefixed by >>): net/built-in.o: In function `sk_alloc': (.text.sk_alloc+0xb4): undefined reference to `mem_cgroup_sk_alloc' net/built-in.o: In function `__sk_destruct': >> net/core/.tmp_sock.o:(.text.__sk_destruct+0xdc): undefined reference to >> `mem_cgroup_sk_free' net/built-in.o: In function `sk_clone_lock': (.text.sk_clone_lock+0x184): undefined reference to `mem_cgroup_sk_alloc' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH 3/3] mm: memcontrol: consolidate cgroup socket tracking
On Wed, Sep 14, 2016 at 03:48:46PM -0400, Johannes Weiner wrote: > The cgroup core and the memory controller need to track socket > ownership for different purposes, but the tracking sites being > entirely different is kind of ugly. > > Be a better citizen and rename the memory controller callbacks to > match the cgroup core callbacks, then move them to the same place. > > Signed-off-by: Johannes Weiner For 1-3, Acked-by: Tejun Heo Thanks. -- tejun