Re: [PATCH 08/14] net: tcp_memcontrol: sanitize tcp memory accounting callbacks

[Johannes Weiner]

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;
> 
> <compiler should eventually barf, 
> 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;
 }

---