On Tue, Jan 17, 2012 at 3:06 PM, Jesse Gross <[email protected]> wrote:
> On Fri, Jan 13, 2012 at 4:38 PM, Pravin B Shelar <[email protected]> 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.
ok, I will move it to caller.
>
>> 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
>
ok.
>> 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.
>
ok.
>> +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.
>
ok.
>> 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.
>
ok.
>> 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.
OK, I will use jhash()
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
