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

[Daniel Lezcano <dlezcano@fr.ibm.com>]

> 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 <a.p.zijlstra@chello.nl>
> ---
>  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" ?

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

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