[Devel] [PATCH 2/7] tcp: Charge socket buffers into cg memory

[Vladimir Davydov]

On Mon, Jun 01, 2015 at 06:16:50PM +0300, Pavel Emelyanov wrote:
> >> @@ -496,9 +496,28 @@ static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)
> >>  	return (memcg == root_mem_cgroup);
> >>  }
> >>  
> >> +void memcg_charge_kmem_bytes(struct mem_cgroup *cg, unsigned long bytes)
> > 
> > Please name this function memcg_charge_kmem_nofail. And use memcg for
> > instances of mem_cgroup, please. The code is already messy, we'd better
> > avoid making it even more inconsistent.
> 
> :D   OK
> 
> >> +{
> >> +	struct res_counter *fail_res;
> >> +
> >> +	/*
> >> +	 * FIXME -- strictly speaking, this value should _also_
> >> +	 * be charged into kmem counter. But since res_counter_charge
> >> +	 * is sub-optimal (takes locks) AND we do not care much
> >> +	 * about kmem limits (at least for now) we can just directly
> >> +	 * charge into mem counter.
> >> +	 */
> >> +	res_counter_charge_nofail(&cg->res, bytes, &fail_res);
> > 
> > You must also charge cg->memsw if do_swap_account is true.
> 
> O_o  Hm, indeed. Charging each counter takes time due to locks.

I don't think it's a matter of time here. I think it's a matter of
contention. Taking a not contended lock should not take long. And if a
lock is contended, it does not really matter if we take one or two
locks IMO.

> Do you plan to ... fix this?

Of course ... in the future :-D

To be serious, I think we should hide kmem.limit_in_bytes* and drop the
kmem counter and use per-cpu stocks here. It should not be difficult to
implement.

> 
> > And I don't think charging kmem will make any difference. OTOH not
> > charging it may result in mem_cgroup_reparent_charges looping forever.
> > 
> >> +}
> >> +
> >> +void memcg_uncharge_kmem_bytes(struct mem_cgroup *cg, unsigned long bytes)
> >> +{
> >> +	/* FIXME -- uncharge also in kmem counter */
> >> +	res_counter_uncharge(&cg->res, bytes);
> > 
> > You don't check if css reference should be dropped. Please use
> > memcg_uncharge_kmem, which does.
> 
> By the time network counter hits zero the cgroup is still held by the
> socket itself, so what's the point in it?

Hmm, right. Anyway, I'd prefer to use the only function for uncharging
if possible.