Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Fri, Nov 20, 2015 at 01:58:57PM +0300, Vladimir Davydov wrote: > On Thu, Nov 12, 2015 at 06:41:27PM -0500, Johannes Weiner wrote: > > There won't be a tcp control soft limit, so integrating the memcg code > > into the global skmem limiting scheme complicates things > > unnecessarily. Replace this with simple and clear charge and uncharge > > calls--hidden behind a jump label--to account skb memory. > > > > Note that this is not purely aesthetic: as a result of shoehorning the > > per-memcg code into the same memory accounting functions that handle > > the global level, the old code would compare the per-memcg consumption > > against the smaller of the per-memcg limit and the global limit. This > > allowed the total consumption of multiple sockets to exceed the global > > limit, as long as the individual sockets stayed within bounds. After > > this change, the code will always compare the per-memcg consumption to > > the per-memcg limit, and the global consumption to the global limit, > > and thus close this loophole. > > > > Without a soft limit, the per-memcg memory pressure state in sockets > > is generally questionable. However, we did it until now, so we > > continue to enter it when the hard limit is hit, and packets are > > dropped, to let other sockets in the cgroup know that they shouldn't > > grow their transmit windows, either. However, keep it simple in the > > new callback model and leave memory pressure lazily when the next > > packet is accepted (as opposed to doing it synchroneously when packets > > are processed). When packets are dropped, network performance will > > already be in the toilet, so that should be a reasonable trade-off. > > > > As described above, consumption is now checked on the per-memcg level > > and the global level separately. Likewise, memory pressure states are > > maintained on both the per-memcg level and the global level, and a > > socket is considered under pressure when either level asserts as much. > > > > Signed-off-by: Johannes Weiner > > It leaves the legacy functionality intact, while making the code look > much better. > > Reviewed-by: Vladimir Davydov Thank you very much! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Thu, Nov 12, 2015 at 06:41:27PM -0500, Johannes Weiner wrote: > There won't be a tcp control soft limit, so integrating the memcg code > into the global skmem limiting scheme complicates things > unnecessarily. Replace this with simple and clear charge and uncharge > calls--hidden behind a jump label--to account skb memory. > > Note that this is not purely aesthetic: as a result of shoehorning the > per-memcg code into the same memory accounting functions that handle > the global level, the old code would compare the per-memcg consumption > against the smaller of the per-memcg limit and the global limit. This > allowed the total consumption of multiple sockets to exceed the global > limit, as long as the individual sockets stayed within bounds. After > this change, the code will always compare the per-memcg consumption to > the per-memcg limit, and the global consumption to the global limit, > and thus close this loophole. > > Without a soft limit, the per-memcg memory pressure state in sockets > is generally questionable. However, we did it until now, so we > continue to enter it when the hard limit is hit, and packets are > dropped, to let other sockets in the cgroup know that they shouldn't > grow their transmit windows, either. However, keep it simple in the > new callback model and leave memory pressure lazily when the next > packet is accepted (as opposed to doing it synchroneously when packets > are processed). When packets are dropped, network performance will > already be in the toilet, so that should be a reasonable trade-off. > > As described above, consumption is now checked on the per-memcg level > and the global level separately. Likewise, memory pressure states are > maintained on both the per-memcg level and the global level, and a > socket is considered under pressure when either level asserts as much. > > Signed-off-by: Johannes Weiner It leaves the legacy functionality intact, while making the code look much better. Reviewed-by: Vladimir Davydov -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Thu, Nov 12, 2015 at 06:41:27PM -0500, Johannes Weiner wrote: > There won't be a tcp control soft limit, so integrating the memcg code > into the global skmem limiting scheme complicates things > unnecessarily. Replace this with simple and clear charge and uncharge > calls--hidden behind a jump label--to account skb memory. > > Note that this is not purely aesthetic: as a result of shoehorning the > per-memcg code into the same memory accounting functions that handle > the global level, the old code would compare the per-memcg consumption > against the smaller of the per-memcg limit and the global limit. This > allowed the total consumption of multiple sockets to exceed the global > limit, as long as the individual sockets stayed within bounds. After > this change, the code will always compare the per-memcg consumption to > the per-memcg limit, and the global consumption to the global limit, > and thus close this loophole. > > Without a soft limit, the per-memcg memory pressure state in sockets > is generally questionable. However, we did it until now, so we > continue to enter it when the hard limit is hit, and packets are > dropped, to let other sockets in the cgroup know that they shouldn't > grow their transmit windows, either. However, keep it simple in the > new callback model and leave memory pressure lazily when the next > packet is accepted (as opposed to doing it synchroneously when packets > are processed). When packets are dropped, network performance will > already be in the toilet, so that should be a reasonable trade-off. > > As described above, consumption is now checked on the per-memcg level > and the global level separately. Likewise, memory pressure states are > maintained on both the per-memcg level and the global level, and a > socket is considered under pressure when either level asserts as much. > > Signed-off-by: Johannes WeinerIt leaves the legacy functionality intact, while making the code look much better. Reviewed-by: Vladimir Davydov -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Fri, Nov 20, 2015 at 01:58:57PM +0300, Vladimir Davydov wrote: > On Thu, Nov 12, 2015 at 06:41:27PM -0500, Johannes Weiner wrote: > > There won't be a tcp control soft limit, so integrating the memcg code > > into the global skmem limiting scheme complicates things > > unnecessarily. Replace this with simple and clear charge and uncharge > > calls--hidden behind a jump label--to account skb memory. > > > > Note that this is not purely aesthetic: as a result of shoehorning the > > per-memcg code into the same memory accounting functions that handle > > the global level, the old code would compare the per-memcg consumption > > against the smaller of the per-memcg limit and the global limit. This > > allowed the total consumption of multiple sockets to exceed the global > > limit, as long as the individual sockets stayed within bounds. After > > this change, the code will always compare the per-memcg consumption to > > the per-memcg limit, and the global consumption to the global limit, > > and thus close this loophole. > > > > Without a soft limit, the per-memcg memory pressure state in sockets > > is generally questionable. However, we did it until now, so we > > continue to enter it when the hard limit is hit, and packets are > > dropped, to let other sockets in the cgroup know that they shouldn't > > grow their transmit windows, either. However, keep it simple in the > > new callback model and leave memory pressure lazily when the next > > packet is accepted (as opposed to doing it synchroneously when packets > > are processed). When packets are dropped, network performance will > > already be in the toilet, so that should be a reasonable trade-off. > > > > As described above, consumption is now checked on the per-memcg level > > and the global level separately. Likewise, memory pressure states are > > maintained on both the per-memcg level and the global level, and a > > socket is considered under pressure when either level asserts as much. > > > > Signed-off-by: Johannes Weiner> > It leaves the legacy functionality intact, while making the code look > much better. > > Reviewed-by: Vladimir Davydov Thank you very much! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Thu, Nov 12, 2015 at 08:53:38PM -0800, Eric Dumazet wrote: > On Thu, 2015-11-12 at 18:41 -0500, Johannes Weiner wrote: > > @@ -711,6 +705,12 @@ 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) > > +{ > > + return proto->memory_pressure; > > +} > > #endif /* CONFIG_INET && CONFIG_MEMCG_KMEM */ > > > > #ifdef CONFIG_MEMCG_KMEM > > diff --git a/include/net/sock.h b/include/net/sock.h > > index 2eefc99..8cc7613 100644 > > --- a/include/net/sock.h > > +++ b/include/net/sock.h > > @@ -1126,8 +1126,8 @@ static inline bool sk_under_memory_pressure(const > > struct sock *sk) > > if (!sk->sk_prot->memory_pressure) > > return false; > > > > - if (mem_cgroup_sockets_enabled && sk->sk_cgrp) > > - return !!sk->sk_cgrp->memory_pressure; > > + if (mem_cgroup_sockets_enabled && sk->sk_cgrp && > > + mem_cgroup_under_socket_pressure(sk->sk_cgrp)) > > > > return !!*sk->sk_prot->memory_pressure; > > } > > > This looks wrong ? > > if (A && B && C) > return !!*sk->sk_prot->memory_pressure; > > as this function should not return void> Yikes, you're right. This is missing a return true. [ Just forced a complete rebuild and of course it warns at control reaching end of non-void function. ] I'm stumped by how I could have missed it as I rebuild after every commit with make -s, so a warning should stand out. And it should definitely rebuild the callers frequently as most patches change memcontrol.h. Probably a screwup in the final series polishing. I'm going to go over this carefully one more time tomorrow. Meanwhile, this is the missing piece and the updated patch. Thanks Eric. diff --git a/include/net/sock.h b/include/net/sock.h index 8cc7613..f954e2a 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -1128,6 +1128,7 @@ static inline bool sk_under_memory_pressure(const struct sock *sk) if (mem_cgroup_sockets_enabled && sk->sk_cgrp && mem_cgroup_under_socket_pressure(sk->sk_cgrp)) + return true; return !!*sk->sk_prot->memory_pressure; } --- >From 4a24ca67e5b0f651a68807ee99f714437ffd6109 Mon Sep 17 00:00:00 2001 From: Johannes Weiner Date: Tue, 10 Nov 2015 17:14:41 -0500 Subject: [PATCH v2] net: tcp_memcontrol: sanitize tcp memory accounting callbacks There won't be a tcp control soft limit, so integrating the memcg code into the global skmem limiting scheme complicates things unnecessarily. Replace this with simple and clear charge and uncharge calls--hidden behind a jump label--to account skb memory. Note that this is not purely aesthetic: as a result of shoehorning the per-memcg code into the same memory accounting functions that handle the global level, the old code would compare the per-memcg consumption against the smaller of the per-memcg limit and the global limit. This allowed the total consumption of multiple sockets to exceed the global limit, as long as the individual sockets stayed within bounds. After this change, the code will always compare the per-memcg consumption to the per-memcg limit, and the global consumption to the global limit, and thus close this loophole. Without a soft limit, the per-memcg memory pressure state in sockets is generally questionable. However, we did it until now, so we continue to enter it when the hard limit is hit, and packets are dropped, to let other sockets in the cgroup know that they shouldn't grow their transmit windows, either. However, keep it simple in the new callback model and leave memory pressure lazily when the next packet is accepted (as opposed to doing it synchroneously when packets are processed). When packets are dropped, network performance will already be in the toilet, so that should be a reasonable trade-off. As described above, consumption is now checked on the per-memcg level and the global level separately. Likewise, memory pressure states are maintained on both the per-memcg level and the global level, and a socket is considered under pressure when either level asserts as much. Signed-off-by: Johannes Weiner --- include/linux/memcontrol.h | 12 - include/net/sock.h | 64 ++ include/net/tcp.h | 5 ++-- mm/memcontrol.c| 32 +++ net/core/sock.c| 26 +++ net/ipv4/tcp_output.c | 7 +++-- 6 files changed, 70 insertions(+), 76 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 96ca3d3..906dfff 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -676,12
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Thu, 2015-11-12 at 18:41 -0500, Johannes Weiner wrote: > @@ -711,6 +705,12 @@ 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) > +{ > + return proto->memory_pressure; > +} > #endif /* CONFIG_INET && CONFIG_MEMCG_KMEM */ > > #ifdef CONFIG_MEMCG_KMEM > diff --git a/include/net/sock.h b/include/net/sock.h > index 2eefc99..8cc7613 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -1126,8 +1126,8 @@ static inline bool sk_under_memory_pressure(const > struct sock *sk) > if (!sk->sk_prot->memory_pressure) > return false; > > - if (mem_cgroup_sockets_enabled && sk->sk_cgrp) > - return !!sk->sk_cgrp->memory_pressure; > + if (mem_cgroup_sockets_enabled && sk->sk_cgrp && > + mem_cgroup_under_socket_pressure(sk->sk_cgrp)) > > return !!*sk->sk_prot->memory_pressure; > } This looks wrong ? if (A && B && C) return !!*sk->sk_prot->memory_pressure; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Thu, Nov 12, 2015 at 08:53:38PM -0800, Eric Dumazet wrote: > On Thu, 2015-11-12 at 18:41 -0500, Johannes Weiner wrote: > > @@ -711,6 +705,12 @@ 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) > > +{ > > + return proto->memory_pressure; > > +} > > #endif /* CONFIG_INET && CONFIG_MEMCG_KMEM */ > > > > #ifdef CONFIG_MEMCG_KMEM > > diff --git a/include/net/sock.h b/include/net/sock.h > > index 2eefc99..8cc7613 100644 > > --- a/include/net/sock.h > > +++ b/include/net/sock.h > > @@ -1126,8 +1126,8 @@ static inline bool sk_under_memory_pressure(const > > struct sock *sk) > > if (!sk->sk_prot->memory_pressure) > > return false; > > > > - if (mem_cgroup_sockets_enabled && sk->sk_cgrp) > > - return !!sk->sk_cgrp->memory_pressure; > > + if (mem_cgroup_sockets_enabled && sk->sk_cgrp && > > + mem_cgroup_under_socket_pressure(sk->sk_cgrp)) > > > > return !!*sk->sk_prot->memory_pressure; > > } > > > This looks wrong ? > > if (A && B && C) > return !!*sk->sk_prot->memory_pressure; > > as this function should not return void> Yikes, you're right. This is missing a return true. [ Just forced a complete rebuild and of course it warns at control reaching end of non-void function. ] I'm stumped by how I could have missed it as I rebuild after every commit with make -s, so a warning should stand out. And it should definitely rebuild the callers frequently as most patches change memcontrol.h. Probably a screwup in the final series polishing. I'm going to go over this carefully one more time tomorrow. Meanwhile, this is the missing piece and the updated patch. Thanks Eric. diff --git a/include/net/sock.h b/include/net/sock.h index 8cc7613..f954e2a 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -1128,6 +1128,7 @@ static inline bool sk_under_memory_pressure(const struct sock *sk) if (mem_cgroup_sockets_enabled && sk->sk_cgrp && mem_cgroup_under_socket_pressure(sk->sk_cgrp)) + return true; return !!*sk->sk_prot->memory_pressure; } --- >From 4a24ca67e5b0f651a68807ee99f714437ffd6109 Mon Sep 17 00:00:00 2001 From: Johannes WeinerDate: Tue, 10 Nov 2015 17:14:41 -0500 Subject: [PATCH v2] net: tcp_memcontrol: sanitize tcp memory accounting callbacks There won't be a tcp control soft limit, so integrating the memcg code into the global skmem limiting scheme complicates things unnecessarily. Replace this with simple and clear charge and uncharge calls--hidden behind a jump label--to account skb memory. Note that this is not purely aesthetic: as a result of shoehorning the per-memcg code into the same memory accounting functions that handle the global level, the old code would compare the per-memcg consumption against the smaller of the per-memcg limit and the global limit. This allowed the total consumption of multiple sockets to exceed the global limit, as long as the individual sockets stayed within bounds. After this change, the code will always compare the per-memcg consumption to the per-memcg limit, and the global consumption to the global limit, and thus close this loophole. Without a soft limit, the per-memcg memory pressure state in sockets is generally questionable. However, we did it until now, so we continue to enter it when the hard limit is hit, and packets are dropped, to let other sockets in the cgroup know that they shouldn't grow their transmit windows, either. However, keep it simple in the new callback model and leave memory pressure lazily when the next packet is accepted (as opposed to doing it synchroneously when packets are processed). When packets are dropped, network performance will already be in the toilet, so that should be a reasonable trade-off. As described above, consumption is now checked on the per-memcg level and the global level separately. Likewise, memory pressure states are maintained on both the per-memcg level and the global level, and a socket is considered under pressure when either level asserts as much. Signed-off-by: Johannes Weiner --- include/linux/memcontrol.h | 12 - include/net/sock.h | 64 ++ include/net/tcp.h | 5 ++-- mm/memcontrol.c| 32 +++ net/core/sock.c| 26 +++ net/ipv4/tcp_output.c | 7 +++-- 6 files changed, 70 insertions(+), 76 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 96ca3d3..906dfff 100644 --- a/include/linux/memcontrol.h +++
Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks
On Thu, 2015-11-12 at 18:41 -0500, Johannes Weiner wrote: > @@ -711,6 +705,12 @@ 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) > +{ > + return proto->memory_pressure; > +} > #endif /* CONFIG_INET && CONFIG_MEMCG_KMEM */ > > #ifdef CONFIG_MEMCG_KMEM > diff --git a/include/net/sock.h b/include/net/sock.h > index 2eefc99..8cc7613 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -1126,8 +1126,8 @@ static inline bool sk_under_memory_pressure(const > struct sock *sk) > if (!sk->sk_prot->memory_pressure) > return false; > > - if (mem_cgroup_sockets_enabled && sk->sk_cgrp) > - return !!sk->sk_cgrp->memory_pressure; > + if (mem_cgroup_sockets_enabled && sk->sk_cgrp && > + mem_cgroup_under_socket_pressure(sk->sk_cgrp)) > > return !!*sk->sk_prot->memory_pressure; > } This looks wrong ? if (A && B && C) return !!*sk->sk_prot->memory_pressure; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/