Re: [PATCH net] Revert "defer call to mem_cgroup_sk_alloc()"
On Fri, 2018-02-02 at 19:04 +, Roman Gushchin wrote: > On Fri, Feb 02, 2018 at 10:39:04AM -0800, Eric Dumazet wrote: > > On Fri, 2018-02-02 at 18:06 +, Roman Gushchin wrote: > > > > > > Idk, how even we can hit it? And if so, what scary will happen? > > > > > > If you prefer to have it there, I definitely can return it, > > > but I see no profit so far. > > > > I was simply curious this was not mentioned in the changelog. > > > > A revert is normally a true revert, modulo the changes needed by > > conflicts and possible changes. > > > > I personally do not care of this BUG_ON(), I had not put it in the > > first place. > > Technically it's not a true revert, but you're totally right. > Let me add a note to the commit description. > > Are you ok with the rest? Sure ! Thanks.
Re: [PATCH net] Revert "defer call to mem_cgroup_sk_alloc()"
On Fri, Feb 02, 2018 at 10:39:04AM -0800, Eric Dumazet wrote: > On Fri, 2018-02-02 at 18:06 +, Roman Gushchin wrote: > > > > Idk, how even we can hit it? And if so, what scary will happen? > > > > If you prefer to have it there, I definitely can return it, > > but I see no profit so far. > > I was simply curious this was not mentioned in the changelog. > > A revert is normally a true revert, modulo the changes needed by > conflicts and possible changes. > > I personally do not care of this BUG_ON(), I had not put it in the > first place. Technically it's not a true revert, but you're totally right. Let me add a note to the commit description. Are you ok with the rest? Thanks!
Re: [PATCH net] Revert "defer call to mem_cgroup_sk_alloc()"
On Fri, 2018-02-02 at 18:06 +, Roman Gushchin wrote: > > Idk, how even we can hit it? And if so, what scary will happen? > > If you prefer to have it there, I definitely can return it, > but I see no profit so far. I was simply curious this was not mentioned in the changelog. A revert is normally a true revert, modulo the changes needed by conflicts and possible changes. I personally do not care of this BUG_ON(), I had not put it in the first place. Thanks.
Re: [PATCH net] Revert "defer call to mem_cgroup_sk_alloc()"
On Fri, Feb 02, 2018 at 09:59:27AM -0800, Eric Dumazet wrote: > On Fri, 2018-02-02 at 16:57 +, Roman Gushchin wrote: > > This patch effectively reverts commit 9f1c2674b328 ("net: memcontrol: > > defer call to mem_cgroup_sk_alloc()"). > > > > Moving mem_cgroup_sk_alloc() to the inet_csk_accept() completely breaks > > memcg socket memory accounting, as packets received before memcg > > pointer initialization are not accounted and are causing refcounting > > underflow on socket release. > > > > Actually the free-after-use problem was fixed by > > commit c0576e397508 ("net: call cgroup_sk_alloc() earlier in > > sk_clone_lock()") for the cgroup pointer. > > > > So, let's revert it and call mem_cgroup_sk_alloc() just before > > cgroup_sk_alloc(). This is safe, as we hold a reference to the socket > > we're cloning, and it holds a reference to the memcg. > > > > Signed-off-by: Roman Gushchin> > Cc: Eric Dumazet > > Cc: David S. Miller > > Cc: Johannes Weiner > > Cc: Tejun Heo > > --- > > mm/memcontrol.c | 14 ++ > > net/core/sock.c | 5 + > > net/ipv4/inet_connection_sock.c | 1 - > > 3 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 0ae2dc3a1748..0937f2c52c7d 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -5747,6 +5747,20 @@ void mem_cgroup_sk_alloc(struct sock *sk) > > 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. > > +* > > +* Respecting the original socket's memcg is a better > > +* decision in this case. > > +*/ > > + if (sk->sk_memcg) { > > Original commit had a BUG_ON(mem_cgroup_is_root(sk->sk_memcg)); > > I presume it is no longer useful ? Idk, how even we can hit it? And if so, what scary will happen? If you prefer to have it there, I definitely can return it, but I see no profit so far. Thanks!
Re: [PATCH net] Revert "defer call to mem_cgroup_sk_alloc()"
On Fri, 2018-02-02 at 16:57 +, Roman Gushchin wrote: > This patch effectively reverts commit 9f1c2674b328 ("net: memcontrol: > defer call to mem_cgroup_sk_alloc()"). > > Moving mem_cgroup_sk_alloc() to the inet_csk_accept() completely breaks > memcg socket memory accounting, as packets received before memcg > pointer initialization are not accounted and are causing refcounting > underflow on socket release. > > Actually the free-after-use problem was fixed by > commit c0576e397508 ("net: call cgroup_sk_alloc() earlier in > sk_clone_lock()") for the cgroup pointer. > > So, let's revert it and call mem_cgroup_sk_alloc() just before > cgroup_sk_alloc(). This is safe, as we hold a reference to the socket > we're cloning, and it holds a reference to the memcg. > > Signed-off-by: Roman Gushchin> Cc: Eric Dumazet > Cc: David S. Miller > Cc: Johannes Weiner > Cc: Tejun Heo > --- > mm/memcontrol.c | 14 ++ > net/core/sock.c | 5 + > net/ipv4/inet_connection_sock.c | 1 - > 3 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 0ae2dc3a1748..0937f2c52c7d 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5747,6 +5747,20 @@ void mem_cgroup_sk_alloc(struct sock *sk) > 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. > + * > + * Respecting the original socket's memcg is a better > + * decision in this case. > + */ > + if (sk->sk_memcg) { Original commit had a BUG_ON(mem_cgroup_is_root(sk->sk_memcg)); I presume it is no longer useful ? Thanks > + css_get(>sk_memcg->css); > + return; > + } > + > rcu_read_lock();