[ovs-dev] [PATCH v5] datapath: Add support for namespace.

Jesse Gross jesse at nicira.com
Tue Jan 17 23:06:22 UTC 2012


On Fri, Jan 13, 2012 at 4:38 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index 27151b9..d5b44b7 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> +static inline void ovs_dp_set_net(struct datapath *dp, struct net *net)
> +{
> +       write_pnet(&dp->net, hold_net(net));
> +}

This is somewhat asymmetric for hold_net() vs. release_net() since
they are done at different levels.  I would either call hold_net()
from the caller or just use read/write_pnet directly since they
already wrap the differences between CONFIG_NET_NS and not.

> diff --git a/datapath/linux/compat/include/net/net_namespace.h b/datapath/linux/compat/include/net/net_namespace.h
> index 9ce9fcd..0e85023 100644
> --- a/datapath/linux/compat/include/net/net_namespace.h
> +++ b/datapath/linux/compat/include/net/net_namespace.h
> @@ -4,12 +4,85 @@
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,24)
>  /* <net/net_namespace.h> exists, go ahead and include it. */
>  #include_next <net/net_namespace.h>

This file is now pretty difficult to read and has duplicate code due
to overlapping kernel version ranges.  I know that before I said to
combine them but now that there is no longer a simple disjunctive set
it makes less sense.  Can you just do an ordered list of code blocks?
Something like this:

#if >= 2.6.24
#include_net net/net_namespace.h
#else
struct net;
for_each_net
for_each_net_rcu
hold_net()
release_net()
__net_init
__net_exit
#endif

#if < 2.6.26
net_eq()
#endif

#if < 2.6.29
write_pnet()
read_pnet()
#endif

#if < 2.6.33
struct pernet_operations
register_pernet_device()
unregister_pernet_device()
#endif

> diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
> index 6c1b0da..4851abd 100644
> --- a/datapath/vport-capwap.c
> +++ b/datapath/vport-capwap.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2010, 2011 Nicira Networks.
> + * Copyright (c) 2010, 2012 Nicira Networks.

Hmm, apparently I missed this file when making all of the copyright
notices consistent.  However, in any case you shouldn't replace 2011
with 2012 but rather extend the range.  Or conversely you could just
fix my mistake and put the correct notice there.

> +static void capwap_destroy(struct vport *vport)
> +{
> +       struct capwap_net *capwap_net = ovs_get_capwap_net(ovs_dp_get_net(vport->dp));
> +
> +       ovs_tnl_destroy(vport);
> +       capwap_net->port_count--;
> +       if (!capwap_net->port_count)
> +               release_socket(ovs_dp_get_net(vport->dp));
> +
> +}

Extra blank line.

> diff --git a/datapath/vport-patch.c b/datapath/vport-patch.c
> index 53b24b0..bad08ec 100644
> --- a/datapath/vport-patch.c
> +++ b/datapath/vport-patch.c
> @@ -163,9 +165,11 @@ static struct vport *patch_create(const struct vport_parms *parms)
>        rcu_assign_pointer(patch_vport->patchconf, patchconf);
>
>        peer_name = patchconf->peer_name;
> -       hlist_add_head(&patch_vport->hash_node, hash_bucket(peer_name));
> -       rcu_assign_pointer(patch_vport->peer, ovs_vport_locate(peer_name));
> -       update_peers(patch_vport->name, vport);
> +       hlist_add_head(&patch_vport->hash_node,
> +                      hash_bucket(ovs_dp_get_net(vport->dp), peer_name));
> +       rcu_assign_pointer(patch_vport->peer,
> +                          ovs_vport_locate(ovs_dp_get_net(vport->dp), peer_name));
> +       update_peers(ovs_dp_get_net(vport->dp), patch_vport->name, vport);

In these places were we access the net a lot, it probably makes sense
to put it in a temporary variable rather than calling ovs_dp_get_net()
over and over again.

> diff --git a/datapath/vport.h b/datapath/vport.h
> index 44cf603..58d78ce 100644
> --- a/datapath/vport.h
> +++ b/datapath/vport.h
> +static inline unsigned int full_name_hash2(const unsigned char *name,
> +                                          unsigned int len, unsigned long hash)

This name isn't really all that descriptive of what it does.

> +{
> +       while (len--)
> +               hash = partial_name_hash(*name++, hash);
> +       return end_name_hash(hash);

I was expecting that you would just do what you did before but use
jhash() instead of jhash2(), which doesn't require that the length is
a multiple of 4.  Otherwise, this bit of open coding seems
unnecessary.



More information about the dev mailing list