Re: [PATCH v7 04/10] tcp memory pressure controls

[Glauber Costa]

On 11/29/2011 11:49 PM, KAMEZAWA Hiroyuki wrote:
> > -static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
> > +struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
> >   {
> >   	return container_of(cgroup_subsys_state(cont,
> >   				mem_cgroup_subsys_id), struct mem_cgroup,
> > @@ -4717,14 +4732,27 @@ static int register_kmem_files(struct cgroup *cont, struct cgroup_subsys *ss)
> >
> >   	ret = cgroup_add_files(cont, ss, kmem_cgroup_files,
> >   			       ARRAY_SIZE(kmem_cgroup_files));
> > +
> > +	if (!ret)
> > +		ret = mem_cgroup_sockets_init(cont, ss);
> >   	return ret;
> >   };
>
> You does initizalication here. The reason what I think is
> 1. 'proto_list' is not available at createion of root cgroup and
>      you need to delay set up until mounting.
>
> If so, please add comment or find another way.
> This seems not very clean to me.

Yes, we do can run into some ordering issues. A part of the 
initialization can be done earlier. But I preferred to move it all later
instead of creating two functions for it. But I can change that if you 
want, no big deal.

> > +static DEFINE_RWLOCK(proto_list_lock);
> > +static LIST_HEAD(proto_list);
> > +
> > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
> > +int mem_cgroup_sockets_init(struct cgroup *cgrp, struct cgroup_subsys *ss)
> > +{
> > +	struct proto *proto;
> > +	int ret = 0;
> > +
> > +	read_lock(&proto_list_lock);
> > +	list_for_each_entry(proto,&proto_list, node) {
> > +		if (proto->init_cgroup)
> > +			ret = proto->init_cgroup(cgrp, ss);
> > +			if (ret)
> > +				goto out;
> > +	}
>
> seems indent is bad or {} is missing.
Thanks. I'll rewrite it, since I did miss {} around the first if. But no 
test could possibly catch it, since what I wanted to write, and what I 
wrote by mistake end up being equivalent.

> > +EXPORT_SYMBOL(memcg_tcp_enter_memory_pressure);
> > +
> > +int tcp_init_cgroup(struct cgroup *cgrp, struct cgroup_subsys *ss)
> > +{
> > +	/*
> > +	 * The root cgroup does not use res_counters, but rather,
> > +	 * rely on the data already collected by the network
> > +	 * subsystem
> > +	 */
> > +	struct res_counter *res_parent = NULL;
> > +	struct cg_proto *cg_proto;
> > +	struct tcp_memcontrol *tcp;
> > +	struct mem_cgroup *memcg = mem_cgroup_from_cont(cgrp);
> > +	struct mem_cgroup *parent = parent_mem_cgroup(memcg);
> > +
> > +	cg_proto = tcp_prot.proto_cgroup(memcg);
> > +	if (!cg_proto)
> > +		return 0;
> > +
> > +	tcp = tcp_from_cgproto(cg_proto);
> > +	cg_proto->parent = tcp_prot.proto_cgroup(parent);
> > +
> > +	tcp->tcp_prot_mem[0] = sysctl_tcp_mem[0];
> > +	tcp->tcp_prot_mem[1] = sysctl_tcp_mem[1];
> > +	tcp->tcp_prot_mem[2] = sysctl_tcp_mem[2];
> > +	tcp->tcp_memory_pressure = 0;
>
> Question:
>
> Is this value will be updated when an admin chages sysctl ?

yes.

> I guess, this value is set at system init script or some which may
> happen later than mounting cgroup.
> I don't like to write a guideline 'please set sysctl val before
> mounting cgroup'

Agreed.

This code is in patch 6 (together with the limiting):

+#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
+       rcu_read_lock();
+       memcg = mem_cgroup_from_task(current);
+
+       tcp_prot_mem(memcg, vec[0], 0);
+       tcp_prot_mem(memcg, vec[1], 1);
+       tcp_prot_mem(memcg, vec[2], 2);
+       rcu_read_unlock();
+#endif

tcp_prot_mem is just a wrapper around the assignment so we can access 
memcg's inner fields.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/