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

Peter Zijlstra
2008-10-01T18:57:23Z

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!