Re: [PATCH 09/14] net: tcp_memcontrol: simplify linkage between socket and page counter

[Vladimir Davydov]

On Mon, Nov 23, 2015 at 01:20:37PM -0500, Johannes Weiner wrote:
> On Mon, Nov 23, 2015 at 12:36:46PM +0300, Vladimir Davydov wrote:
> > On Fri, Nov 20, 2015 at 01:56:48PM -0500, Johannes Weiner wrote:
> > > I actually had all this at first, but then wondered if it makes more
> > > sense to keep the legacy code in isolation. Don't you think it would
> > > be easier to keep track of what's v1 and what's v2 if we keep the
> > > legacy stuff physically separate as much as possible? In particular I
> > > found that 'tcp_mem.' marker really useful while working on the code.
> > > 
> > > In the same vein, tcp_memcontrol.c doesn't really hurt anybody and I'd
> > > expect it to remain mostly unopened and unchanged in the future. But
> > > if we merge it into memcontrol.c, that code will likely be in the way
> > > and we'd have to make it explicit somehow that this is not actually
> > > part of the new memory controller anymore.
> > > 
> > > What do you think?
> > 
> > There isn't much code left in tcp_memcontrol.c, and not all of it is
> > legacy. We still want to call tcp_init_cgroup and tcp_destroy_cgroup
> > from memcontrol.c - in fact, it's the only call site, so I think we'd
> > better keep these functions there. Apart from init/destroy, there is
> > only stuff for handling legacy files, which is relatively small and
> > isolated. We can just put it along with memsw and kmem legacy files in
> > the end of memcontrol.c adding a comment that it's legacy. Personally,
> > I'd find the code easier to follow then, because currently the logic
> > behind the ACTIVE flag as well as memcg->tcp_mem init/use/destroy turns
> > out to be scattered between two files in different subsystems for no
> > apparent reason now, as it does not need tcp_prot any more. Besides,
> > this would allow us to accurately reuse the ACTIVE flag in init/destroy
> > for inc/dec static branch and probably in sock_update_memcg instead of
> > sprinkling cgroup_subsys_on_dfl all over the place, which would make the
> > code a bit cleaner IMO (in fact, that's why I proposed to drop ACTIVATED
> > bit and replace cg_proto->flags with ->active bool).
> 
> As far as I can see, all of tcp_memcontrol.c is legacy, including the
> init and destroy functions. We only call them to set up the legacy
> tcp_mem state and do legacy jump-label maintenance. Delete it all and
> the unified hierarchy controller would still work. So I don't really
> see the benefits of consolidating it, and more risk of convoluting.
> 
> That being said, if you care strongly about it and see opportunities
> to cut down code and make things more readable, please feel free to
> turn the flags -> bool patch into a followup series and I'll be happy
> to review it.

OK, I'll look into that.

Regarding this patch, I don't have any questions left,

Reviewed-by: Vladimir Davydov 

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] net: tcp_memcontrol: simplify linkage between socket and page counter

[Johannes Weiner]

On Mon, Nov 23, 2015 at 12:36:46PM +0300, Vladimir Davydov wrote:
> On Fri, Nov 20, 2015 at 01:56:48PM -0500, Johannes Weiner wrote:
> > I actually had all this at first, but then wondered if it makes more
> > sense to keep the legacy code in isolation. Don't you think it would
> > be easier to keep track of what's v1 and what's v2 if we keep the
> > legacy stuff physically separate as much as possible? In particular I
> > found that 'tcp_mem.' marker really useful while working on the code.
> > 
> > In the same vein, tcp_memcontrol.c doesn't really hurt anybody and I'd
> > expect it to remain mostly unopened and unchanged in the future. But
> > if we merge it into memcontrol.c, that code will likely be in the way
> > and we'd have to make it explicit somehow that this is not actually
> > part of the new memory controller anymore.
> > 
> > What do you think?
> 
> There isn't much code left in tcp_memcontrol.c, and not all of it is
> legacy. We still want to call tcp_init_cgroup and tcp_destroy_cgroup
> from memcontrol.c - in fact, it's the only call site, so I think we'd
> better keep these functions there. Apart from init/destroy, there is
> only stuff for handling legacy files, which is relatively small and
> isolated. We can just put it along with memsw and kmem legacy files in
> the end of memcontrol.c adding a comment that it's legacy. Personally,
> I'd find the code easier to follow then, because currently the logic
> behind the ACTIVE flag as well as memcg->tcp_mem init/use/destroy turns
> out to be scattered between two files in different subsystems for no
> apparent reason now, as it does not need tcp_prot any more. Besides,
> this would allow us to accurately reuse the ACTIVE flag in init/destroy
> for inc/dec static branch and probably in sock_update_memcg instead of
> sprinkling cgroup_subsys_on_dfl all over the place, which would make the
> code a bit cleaner IMO (in fact, that's why I proposed to drop ACTIVATED
> bit and replace cg_proto->flags with ->active bool).

As far as I can see, all of tcp_memcontrol.c is legacy, including the
init and destroy functions. We only call them to set up the legacy
tcp_mem state and do legacy jump-label maintenance. Delete it all and
the unified hierarchy controller would still work. So I don't really
see the benefits of consolidating it, and more risk of convoluting.

That being said, if you care strongly about it and see opportunities
to cut down code and make things more readable, please feel free to
turn the flags -> bool patch into a followup series and I'll be happy
to review it.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] net: tcp_memcontrol: simplify linkage between socket and page counter

[Vladimir Davydov]

On Fri, Nov 20, 2015 at 01:56:48PM -0500, Johannes Weiner wrote:
> On Fri, Nov 20, 2015 at 03:42:16PM +0300, Vladimir Davydov wrote:
> > On Thu, Nov 12, 2015 at 06:41:28PM -0500, Johannes Weiner wrote:
> > > There won't be any separate counters for socket memory consumed by
> > > protocols other than TCP in the future. Remove the indirection and
> > 
> > I really want to believe you're right. And with vmpressure propagation
> > implemented properly you are likely to be right.
> > 
> > However, we might still want to account other socket protos to
> > memcg->memory in the unified hierarchy, e.g. UDP, or SCTP, or whatever
> > else. Adding new consumers should be trivial, but it will break the
> > legacy usecase, where only TCP sockets are supposed to be accounted.
> > What about adding a check to sock_update_memcg() so that it would enable
> > accounting only for TCP sockets in case legacy hierarchy is used?
> 
> Yup, I was thinking the same thing. But we can cross that bridge when
> we come to it and are actually adding further packet types.

Fair enough.

> 
> > For the same reason, I think we'd better rename memcg->tcp_mem to
> > something like memcg->sk_mem or we can even drop the cg_proto struct
> > altogether embedding its fields directly to mem_cgroup struct.
> > 
> > Also, I don't see any reason to have tcp_memcontrol.c file. It's tiny
> > and with this patch it does not depend on tcp code any more. Let's move
> > it to memcontrol.c?
> 
> I actually had all this at first, but then wondered if it makes more
> sense to keep the legacy code in isolation. Don't you think it would
> be easier to keep track of what's v1 and what's v2 if we keep the
> legacy stuff physically separate as much as possible? In particular I
> found that 'tcp_mem.' marker really useful while working on the code.
> 
> In the same vein, tcp_memcontrol.c doesn't really hurt anybody and I'd
> expect it to remain mostly unopened and unchanged in the future. But
> if we merge it into memcontrol.c, that code will likely be in the way
> and we'd have to make it explicit somehow that this is not actually
> part of the new memory controller anymore.
> 
> What do you think?

There isn't much code left in tcp_memcontrol.c, and not all of it is
legacy. We still want to call tcp_init_cgroup and tcp_destroy_cgroup
from memcontrol.c - in fact, it's the only call site, so I think we'd
better keep these functions there. Apart from init/destroy, there is
only stuff for handling legacy files, which is relatively small and
isolated. We can just put it along with memsw and kmem legacy files in
the end of memcontrol.c adding a comment that it's legacy. Personally,
I'd find the code easier to follow then, because currently the logic
behind the ACTIVE flag as well as memcg->tcp_mem init/use/destroy turns
out to be scattered between two files in different subsystems for no
apparent reason now, as it does not need tcp_prot any more. Besides,
this would allow us to accurately reuse the ACTIVE flag in init/destroy
for inc/dec static branch and probably in sock_update_memcg instead of
sprinkling cgroup_subsys_on_dfl all over the place, which would make the
code a bit cleaner IMO (in fact, that's why I proposed to drop ACTIVATED
bit and replace cg_proto->flags with ->active bool).

Regarding, tcp_mem marker, well, currently it's OK, because we don't
account anything but TCP sockets, but when it changes (and I'm pretty
sure it will), we'll have to rename it anyway. For now, I'm OK with
leaving it as is though.

Thanks,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] net: tcp_memcontrol: simplify linkage between socket and page counter

[Vladimir Davydov]

On Thu, Nov 12, 2015 at 06:41:28PM -0500, Johannes Weiner wrote:
> There won't be any separate counters for socket memory consumed by
> protocols other than TCP in the future. Remove the indirection and

I really want to believe you're right. And with vmpressure propagation
implemented properly you are likely to be right.

However, we might still want to account other socket protos to
memcg->memory in the unified hierarchy, e.g. UDP, or SCTP, or whatever
else. Adding new consumers should be trivial, but it will break the
legacy usecase, where only TCP sockets are supposed to be accounted.
What about adding a check to sock_update_memcg() so that it would enable
accounting only for TCP sockets in case legacy hierarchy is used?

For the same reason, I think we'd better rename memcg->tcp_mem to
something like memcg->sk_mem or we can even drop the cg_proto struct
altogether embedding its fields directly to mem_cgroup struct.

Also, I don't see any reason to have tcp_memcontrol.c file. It's tiny
and with this patch it does not depend on tcp code any more. Let's move
it to memcontrol.c?

Other than that this patch looks OK to me.

Thanks,
Vladimir

> link sockets directly to their owning memory cgroup.
> 
> Signed-off-by: Johannes Weiner 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/14] net: tcp_memcontrol: simplify linkage between socket and page counter

[Johannes Weiner]

On Fri, Nov 20, 2015 at 03:42:16PM +0300, Vladimir Davydov wrote:
> On Thu, Nov 12, 2015 at 06:41:28PM -0500, Johannes Weiner wrote:
> > There won't be any separate counters for socket memory consumed by
> > protocols other than TCP in the future. Remove the indirection and
> 
> I really want to believe you're right. And with vmpressure propagation
> implemented properly you are likely to be right.
> 
> However, we might still want to account other socket protos to
> memcg->memory in the unified hierarchy, e.g. UDP, or SCTP, or whatever
> else. Adding new consumers should be trivial, but it will break the
> legacy usecase, where only TCP sockets are supposed to be accounted.
> What about adding a check to sock_update_memcg() so that it would enable
> accounting only for TCP sockets in case legacy hierarchy is used?

Yup, I was thinking the same thing. But we can cross that bridge when
we come to it and are actually adding further packet types.

> For the same reason, I think we'd better rename memcg->tcp_mem to
> something like memcg->sk_mem or we can even drop the cg_proto struct
> altogether embedding its fields directly to mem_cgroup struct.
> 
> Also, I don't see any reason to have tcp_memcontrol.c file. It's tiny
> and with this patch it does not depend on tcp code any more. Let's move
> it to memcontrol.c?

I actually had all this at first, but then wondered if it makes more
sense to keep the legacy code in isolation. Don't you think it would
be easier to keep track of what's v1 and what's v2 if we keep the
legacy stuff physically separate as much as possible? In particular I
found that 'tcp_mem.' marker really useful while working on the code.

In the same vein, tcp_memcontrol.c doesn't really hurt anybody and I'd
expect it to remain mostly unopened and unchanged in the future. But
if we merge it into memcontrol.c, that code will likely be in the way
and we'd have to make it explicit somehow that this is not actually
part of the new memory controller anymore.

What do you think?

> Other than that this patch looks OK to me.

Thank you!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/14] net: tcp_memcontrol: simplify linkage between socket and page counter

[Johannes Weiner]

There won't be any separate counters for socket memory consumed by
protocols other than TCP in the future. Remove the indirection and
link sockets directly to their owning memory cgroup.

Signed-off-by: Johannes Weiner 
---
 include/linux/memcontrol.h   | 18 +++-
 include/net/sock.h   | 37 ---
 include/net/tcp.h|  4 +--
 include/net/tcp_memcontrol.h |  1 -
 mm/memcontrol.c  | 57 ++--
 net/core/sock.c  | 52 +---
 net/ipv4/tcp_ipv4.c  |  7 +
 net/ipv4/tcp_memcontrol.c| 70 ++--
 net/ipv4/tcp_output.c|  4 +--
 net/ipv6/tcp_ipv6.c  |  3 --
 10 files changed, 71 insertions(+), 182 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 906dfff..1c71f27 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -99,16 +99,6 @@ struct cg_proto {
struct page_counter memory_allocated;   /* Current allocated 
memory. */
int memory_pressure;
unsigned long   flags;
-   /*
-* memcg field is used to find which memcg we belong directly
-* Each memcg struct can hold more than one cg_proto, so container_of
-* won't really cut.
-*
-* The elegant solution would be having an inverse function to
-* proto_cgroup in struct proto, but that means polluting the structure
-* for everybody, instead of just for memcg users.
-*/
-   struct mem_cgroup   *memcg;
 };
 
 #ifdef CONFIG_MEMCG
@@ -705,11 +695,11 @@ static inline void mem_cgroup_wb_stats(struct 
bdi_writeback *wb,
 struct sock;
 void sock_update_memcg(struct sock *sk);
 void sock_release_memcg(struct sock *sk);
-bool mem_cgroup_charge_skmem(struct cg_proto *proto, unsigned int nr_pages);
-void mem_cgroup_uncharge_skmem(struct cg_proto *proto, unsigned int nr_pages);
-static inline bool mem_cgroup_under_socket_pressure(struct cg_proto *proto)
+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);
+static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
 {
-   return proto->memory_pressure;
+   return memcg->tcp_mem.memory_pressure;
 }
 #endif /* CONFIG_INET && CONFIG_MEMCG_KMEM */
 
diff --git a/include/net/sock.h b/include/net/sock.h
index 8cc7613..b439dcc 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -69,22 +69,6 @@
 #include 
 #include 
 
-struct cgroup;
-struct cgroup_subsys;
-#ifdef CONFIG_NET
-int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys 
*ss);
-void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg);
-#else
-static inline
-int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
-{
-   return 0;
-}
-static inline
-void mem_cgroup_sockets_destroy(struct mem_cgroup *memcg)
-{
-}
-#endif
 /*
  * This structure really needs to be cleaned up.
  * Most of it is for TCP, and not used by any of
@@ -310,7 +294,7 @@ struct cg_proto;
   *@sk_security: used by security modules
   *@sk_mark: generic packet mark
   *@sk_classid: this socket's cgroup classid
-  *@sk_cgrp: this socket's cgroup-specific proto data
+  *@sk_memcg: this socket's memory cgroup association
   *@sk_write_pending: a write to stream socket waits to start
   *@sk_state_change: callback to indicate change in the state of the sock
   *@sk_data_ready: callback to indicate there is data to be processed
@@ -447,7 +431,7 @@ struct sock {
 #ifdef CONFIG_CGROUP_NET_CLASSID
u32 sk_classid;
 #endif
-   struct cg_proto *sk_cgrp;
+   struct mem_cgroup   *sk_memcg;
void(*sk_state_change)(struct sock *sk);
void(*sk_data_ready)(struct sock *sk);
void(*sk_write_space)(struct sock *sk);
@@ -1051,18 +1035,6 @@ struct proto {
 #ifdef SOCK_REFCNT_DEBUG
atomic_tsocks;
 #endif
-#ifdef CONFIG_MEMCG_KMEM
-   /*
-* cgroup specific init/deinit functions. Called once for all
-* protocols that implement it, from cgroups populate function.
-* This function has to setup any files the protocol want to
-* appear in the kmem cgroup filesystem.
-*/
-   int (*init_cgroup)(struct mem_cgroup *memcg,
-  struct cgroup_subsys *ss);
-   void(*destroy_cgroup)(struct mem_cgroup *memcg);
-   struct cg_proto *(*proto_cgroup)(struct mem_cgroup *memcg);
-#endif
 };
 
 int proto_register(struct proto *prot, int alloc_slab);
@@ -1126,8 +1098,9 @@ static inline bool sk_under_memory_pressure(const struct 
sock *sk)