Re: [PATCH 18/30] netvm: INET reserves.

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Wed, 2008-10-01 at 13:38 +0200, Daniel Lezcano wrote:

> I removed a big portion of code because the remarks below apply to the 
> rest of the code.
> 
> > +static int sysctl_intvec_route(struct ctl_table *table,
> > +		int __user *name, int nlen,
> > +		void __user *oldval, size_t __user *oldlenp,
> > +		void __user *newval, size_t newlen)
> > +{
> > +	struct net *net = current->nsproxy->net_ns;
> 
> I think you can use the container_of and get rid of using 
> current->nsproxy->net_ns.
> 
> 	struct net *net = container_of(table->data, struct net,
> 				ipv6.sysctl.ip6_rt_max_size);

D'oh - why didn't I think of that... yes very nice.


> > +	int write = (newval && newlen);
> > +	int new_size, ret;
> > +
> > +	mutex_lock(&net->ipv6.sysctl.ip6_rt_lock);
> > +
> > +	if (write)
> > +		table->data = &new_size;
> > +
> > +	ret = sysctl_intvec(table, name, nlen, oldval, oldlenp, newval, newlen);
> > +
> > +	if (!ret && write) {
> > +		ret = mem_reserve_kmem_cache_set(&net->ipv6.ip6_rt_reserve,
> > +				net->ipv6.ip6_dst_ops.kmem_cachep, new_size);
> > +		if (!ret)
> > +			net->ipv6.sysctl.ip6_rt_max_size = new_size;
> > +	}
> > +
> > +	if (write)
> > +		table->data = &net->ipv6.sysctl.ip6_rt_max_size;
> > +
> > +	mutex_unlock(&net->ipv6.sysctl.ip6_rt_lock);
> > +
> > +	return ret;
> > +}
> 
> Dancing with the table->data looks safe but it is not very nice.
> Isn't possible to use a temporary table like in the function 
> "ipv4_sysctl_local_port_range" ?

Ah, nice solution. Thanks!

> > Index: linux-2.6/net/ipv6/af_inet6.c
> > ===================================================================
> > --- linux-2.6.orig/net/ipv6/af_inet6.c
> > +++ linux-2.6/net/ipv6/af_inet6.c
> > @@ -851,6 +851,20 @@ static int inet6_net_init(struct net *ne
> >  	net->ipv6.sysctl.ip6_rt_min_advmss = IPV6_MIN_MTU - 20 - 40;
> >  	net->ipv6.sysctl.icmpv6_time = 1*HZ;
> > 
> > +	mem_reserve_init(&net->ipv6.ip6_rt_reserve, "IPv6 route cache",
> > +			 &net_rx_reserve);
> > +	/*
> > +	 * XXX: requires that net->ipv6.ip6_dst_ops is already set-up
> > +	 *      but afaikt its impossible to order the various
> > +	 *      pernet_subsys calls so that this one is done after
> > +	 *      ip6_route_net_init().
> > +	 */
> 
> As this code seems related to the routes, is there a particular reason 
> to not put it at the end of "ip6_route_net_init" function ? You will be 
> sure "net->ipv6.ip6_dst_ops is already set-up", no ?

Ah, the problem is that I need both dst_ops and ip6_rt_max_size set.

The former is set in ip6_route_net_init() while the later is set in
inet6_net_init(), both are registered pernet_ops without specified
order.

So where exactly do I hook in?

> > +	err = mem_reserve_kmem_cache_set(&net->ipv6.ip6_rt_reserve,
> > +			net->ipv6.ip6_dst_ops.kmem_cachep,
> > +			net->ipv6.sysctl.ip6_rt_max_size);
> > +	if (err)
> > +		goto reserve_fail;
> > +
> >  #ifdef CONFIG_PROC_FS
> >  	err = udp6_proc_init(net);
> >  	if (err)
> > @@ -861,8 +875,8 @@ static int inet6_net_init(struct net *ne
> >  	err = ac6_proc_init(net);
> >  	if (err)
> >  		goto proc_ac6_fail;
> > -out:
> >  #endif
> > +out:
> >  	return err;
> > 
> >  #ifdef CONFIG_PROC_FS
> > @@ -870,8 +884,10 @@ proc_ac6_fail:
> >  	tcp6_proc_exit(net);
> >  proc_tcp6_fail:
> >  	udp6_proc_exit(net);
> > -	goto out;
> >  #endif
> > +reserve_fail:
> > +	mem_reserve_disconnect(&net->ipv6.ip6_rt_reserve);
> 
> Idem.
> 
> > +	goto out;
> >  }
> > 
> >  static void inet6_net_exit(struct net *net)
> 
> Isn't "mem_reserve_disconnect" missing here ? (but going to 
> ip6_route_net_exit)

Probably, I'll go over the exit paths once I get the init path ;-)

> I hope this review helped :)

It did, much appreciated!