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

From: Daniel Lezcano <hidden>
Date: 2008-10-01 11:38:48
Also in: linux-mm, lkml

Add reserves for INET.

The two big users seem to be the route cache and ip-fragment cache.

Reserve the route cache under generic RX reserve, its usage is bounded by
the high reclaim watermark, and thus does not need further accounting.

Reserve the ip-fragement caches under SKB data reserve, these add to the
SKB RX limit. By ensuring we can at least receive as much data as fits in
the reassmbly line we avoid fragment attack deadlocks.

Adds to the reserve tree:

  total network reserve      
    network TX reserve       
      protocol TX pages      
    network RX reserve       
+     IPv6 route cache       
+     IPv4 route cache       
      SKB data reserve       
+       IPv6 fragment cache  
+       IPv4 fragment cache  

Signed-off-by: Peter Zijlstra <redacted>
---
 include/net/inet_frag.h  |    7 +++
 include/net/netns/ipv6.h |    4 ++
 net/ipv4/inet_fragment.c |    3 +
 net/ipv4/ip_fragment.c   |   89 +++++++++++++++++++++++++++++++++++++++++++++--
 net/ipv4/route.c         |   72 +++++++++++++++++++++++++++++++++++++-
 net/ipv6/af_inet6.c      |   20 +++++++++-
 net/ipv6/reassembly.c    |   88 +++++++++++++++++++++++++++++++++++++++++++++-
 net/ipv6/route.c         |   66 ++++++++++++++++++++++++++++++++++
 8 files changed, 341 insertions(+), 8 deletions(-)

Sorry for the delay ...

[ cut ]

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);

Another solution could be to pass directly the sysctl structure pointer 
in the table data instead of
".data = &init_net.ipv6.sysctl.ip6_rt_max_size" when initializing the 
sysctl table below. But you have to set the right field value yourself.

+	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" ?

quoted hunk ↗ jump to hunk

 ctl_table ipv6_route_table_template[] = {
 	{
 		.procname	=	"flush",

@@ -2520,7 +2581,8 @@ ctl_table ipv6_route_table_template[] = 
 		.data		=	&init_net.ipv6.sysctl.ip6_rt_max_size,
 		.maxlen		=	sizeof(int),
 		.mode		=	0644,
-		.proc_handler	=	&proc_dointvec,
+		.proc_handler	=	&proc_dointvec_route,
+		.strategy	= 	&sysctl_intvec_route,
 	},
 	{
 		.ctl_name	=	NET_IPV6_ROUTE_GC_MIN_INTERVAL,
@@ -2608,6 +2670,8 @@ struct ctl_table *ipv6_route_sysctl_init
 		table[8].data = &net->ipv6.sysctl.ip6_rt_min_advmss;
 	}

+	mutex_init(&net->ipv6.sysctl.ip6_rt_lock);
+
 	return table;
 }
 #endif

Index: linux-2.6/include/net/inet_frag.h
===================================================================

--- linux-2.6.orig/include/net/inet_frag.h
+++ linux-2.6/include/net/inet_frag.h

@@ -1,6 +1,9 @@
 #ifndef __NET_FRAG_H__
 #define __NET_FRAG_H__

+#include <linux/reserve.h>
+#include <linux/mutex.h>
+
 struct netns_frags {
 	int			nqueues;
 	atomic_t		mem;

@@ -10,6 +13,10 @@ struct netns_frags {
 	int			timeout;
 	int			high_thresh;
 	int			low_thresh;
+
+	/* reserves */
+	struct mutex		lock;
+	struct mem_reserve	reserve;
 };

 struct inet_frag_queue {

Index: linux-2.6/net/ipv4/inet_fragment.c
===================================================================

--- linux-2.6.orig/net/ipv4/inet_fragment.c
+++ linux-2.6/net/ipv4/inet_fragment.c

@@ -19,6 +19,7 @@
 #include <linux/random.h>
 #include <linux/skbuff.h>
 #include <linux/rtnetlink.h>
+#include <linux/reserve.h>

 #include <net/inet_frag.h>

@@ -74,6 +75,8 @@ void inet_frags_init_net(struct netns_fr
 	nf->nqueues = 0;
 	atomic_set(&nf->mem, 0);
 	INIT_LIST_HEAD(&nf->lru_list);
+	mutex_init(&nf->lock);
+	mem_reserve_init(&nf->reserve, "IP fragement cache", NULL);
 }
 EXPORT_SYMBOL(inet_frags_init_net);

Index: linux-2.6/include/net/netns/ipv6.h
===================================================================

--- linux-2.6.orig/include/net/netns/ipv6.h
+++ linux-2.6/include/net/netns/ipv6.h

@@ -24,6 +24,8 @@ struct netns_sysctl_ipv6 {
 	int ip6_rt_mtu_expires;
 	int ip6_rt_min_advmss;
 	int icmpv6_time;
+
+	struct mutex ip6_rt_lock;
 };

 struct netns_ipv6 {

@@ -55,5 +57,7 @@ struct netns_ipv6 {
 	struct sock             *ndisc_sk;
 	struct sock             *tcp_sk;
 	struct sock             *igmp_sk;
+
+	struct mem_reserve	ip6_rt_reserve;
 };
 #endif

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 ?

quoted hunk ↗ jump to hunk

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


I hope this review helped :)

Thanks
	--Daniel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help