On Tue, Jan 10, 2012 at 5:25 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
> diff --git a/datapath/compat.h b/datapath/compat.h
> index efad6a0..f9ec590 100644
> --- a/datapath/compat.h
> +++ b/datapath/compat.h
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32)
> +#define SET_NETNSOK
> +#else
> +#define SET_NETNSOK .netnsok = true,
> +#endif
> +
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,32)
> +#define GENL_SOCK(net) ((net)->genl_sock)
> +#else
> +#define GENL_SOCK(net) (genl_sock)
> +#endif
We might as well merge these into a single conditional, since I think
the underlying condition is the same.
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 17871e4..0563021 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> +static int __net_init ovs_init_net(struct net *net)
> +{
> + struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
> +
> + memset(ovs_net, 0, sizeof(*ovs_net));
This is allocated with kzalloc(), so it shouldn't be necessary to memset it.
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index 27151b9..f1be0d0 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
[...]
> +struct ovs_net {
> + struct list_head dps; /* List of datapaths */
> + struct tnl_net tnl_net; /* Per network namespace data for
> tnl. */
> + struct vport_net vport_net; /* Per network namespace data for
> vport. */
> +};
The kernel comment style that I was referring to before is the one
with the comments above the struct with @, %, etc. similar to struct
dp_upcall_info.
> diff --git a/datapath/dp_sysfs_dp.c b/datapath/dp_sysfs_dp.c
> index 1574a93..0d3cf02 100644
> --- a/datapath/dp_sysfs_dp.c
> +++ b/datapath/dp_sysfs_dp.c
> @@ -395,6 +401,11 @@ int ovs_dp_sysfs_del_dp(struct datapath *dp)
> struct vport *vport = rtnl_dereference(dp->ports[OVSP_LOCAL]);
> struct kobject *kobj = vport->ops->get_kobj(vport);
>
> +#ifdef CONFIG_NET_NS
> + if (!kobj->sd)
> + return -ENOENT;
We should probably return 0 here, since the entry was successfully "deleted".
> diff --git a/datapath/linux/compat/include/net/net_namespace.h
> b/datapath/linux/compat/include/net/net_namespace.h
> index 9ce9fcd..725fc1c 100644
> --- a/datapath/linux/compat/include/net/net_namespace.h
> +++ b/datapath/linux/compat/include/net/net_namespace.h
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,24)
We have a check for >= 2.6.24 and another for < 2.6.24; can you
combine them into an #else since they contain the same things, just
for different kernel versions?
> +static inline struct net *sock_net(const struct sock *sk)
> +{
> + return NULL;
> +}
Isn't this actually defined in net/sock.h?
> +#define __net_init __init
> +#define __net_exit __exit
> +#endif
> +
> +
Extra blank line.
> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index 33d2fe9..c28aefa 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> @@ -16,6 +16,8 @@
> * 02110-1301, USA
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/if_arp.h>
> #include <linux/if_ether.h>
> #include <linux/ip.h>
> @@ -82,23 +84,13 @@
> #define CACHE_DATA_ALIGN 16
> #define PORT_TABLE_SIZE 1024
>
> -static struct hlist_head *port_table __read_mostly;
> -static int port_table_count;
>
Extra blank line.
> -/*
> - * These are just used as an optimization: they don't require any kind of
> - * synchronization because we could have just as easily read the value before
> - * the port change happened.
> - */
> -static unsigned int key_local_remote_ports __read_mostly;
> -static unsigned int key_remote_ports __read_mostly;
> -static unsigned int key_multicast_ports __read_mostly;
> -static unsigned int local_remote_ports __read_mostly;
> -static unsigned int remote_ports __read_mostly;
> -static unsigned int multicast_ports __read_mostly;
If we're now assuming that it's unlikely there will be many tunnels
spread across different namespaces then I'm not sure that we need
per-namespace copies of these variables.
> +static struct hlist_head *port_table __read_mostly;
> +/* Port table count across all namespaces. */
> +static int port_table_count __read_mostly;
I think that port_table_count is read exactly the same number of times
as it is written, so I don't think that it qualifies for
__read_mostly.
> diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
> index 6c1b0da..02ab2b9 100644
> --- a/datapath/vport-capwap.c
> +++ b/datapath/vport-capwap.c
> +static struct vport *capwap_create(const struct vport_parms *parms)
> +{
> + struct capwap_net *capwap_net = ovs_get_capwap_net(parms->dp->net);
> + struct vport *vport;
> +
> + if (!capwap_net->port_count) {
> + int err;
> + err = init_socket(parms->dp->net);
> + if (err)
> + return ERR_PTR(err);
> + }
> +
> + vport = ovs_tnl_create(parms, &ovs_capwap_vport_ops, &capwap_tnl_ops);
> + if (!IS_ERR(vport))
> + capwap_net->port_count++;
> + else
> + release_socket(parms->dp->net);
> + return vport;
> +}
> +
> +
Double blank line.
> +static void capwap_destroy(struct vport *vport)
> +{
> + struct capwap_net *capwap_net = ovs_get_capwap_net(vport->dp->net);
> +
> + capwap_net->port_count--;
> + if (!capwap_net->port_count)
> + release_socket(vport->dp->net);
> +
> + ovs_tnl_destroy(vport);
You should do these in the opposite order so that destruction is the
reverse of initialization.
> diff --git a/datapath/vport-capwap.h b/datapath/vport-capwap.h
> new file mode 100644
> index 0000000..40d6c1d
> --- /dev/null
> +++ b/datapath/vport-capwap.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2007-2011 Nicira Networks.
Probably should use the right copyright year (definitely here but
ideally for all the files that you touched.)
> diff --git a/datapath/vport-patch.c b/datapath/vport-patch.c
> index 53b24b0..9d9a366 100644
> --- a/datapath/vport-patch.c
> +++ b/datapath/vport-patch.c
> -static void update_peers(const char *name, struct vport *);
> +static void update_peers(const char *name, struct net *, struct vport *);
Usually, the net would be the first argument here.
> -static struct hlist_head *hash_bucket(const char *name)
> +static struct hlist_head *hash_bucket(struct vport *vport, const char *name)
I would just pass in the net, since the thing being hashed is actually
the peer, not this vport.
> - unsigned int hash = full_name_hash(name, strlen(name));
> + unsigned int hash = jhash2((u32 *)name, DIV_ROUND_UP(strlen(name),
> sizeof(u32)),
> + (int) (unsigned long) vport->dp->net);
Doesn't this read past the end of the string if the length isn't divisible by 4?
> diff --git a/datapath/vport.c b/datapath/vport.c
> index e9ccdbd..08c0b57 100644
> --- a/datapath/vport.c
> +++ b/datapath/vport.c
> +static struct hlist_head *hash_bucket(struct net *net, const char *name)
> {
> - unsigned int hash = full_name_hash(name, strlen(name));
> + unsigned int hash = jhash2((u32 *)name, DIV_ROUND_UP(strlen(name),
> sizeof(u32)),
> + (int) (unsigned long) net);
Same issue with the string length here.
> -struct vport *ovs_vport_locate(const char *name)
> +struct vport *ovs_vport_locate(struct net *net, const char *name)
[...]
> hlist_for_each_entry_rcu(vport, node, bucket, hash_node)
> - if (!strcmp(name, vport->ops->get_name(vport)))
> + if (!strcmp(name, vport->ops->get_name(vport)) &&
> + vport->dp->net == net)
You should use net_eq() to compare namespaces.
Also in the case where CONFIG_NET_NS isn't defined we shouldn't
include the net in the various data structures. Strictly speaking
though, neither of the net pointers are actually necessary because
they can be derived from an underlying object so that might actually
be the cleaner way to go.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
