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

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.

quoted

+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.

quoted

+	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!

quoted

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?

quoted

+	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.

quoted

+	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!