lkml.org 
[lkml]   [2008]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 18/30] netvm: INET reserves.
    > 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




    \
     
     \ /
      Last update: 2008-10-01 13:41    [W:0.048 / U:1.140 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site