On Wed, Apr 16, 2014 at 1:34 PM, Jesse Gross <[email protected]> wrote:
> On Tue, Apr 15, 2014 at 6:21 PM, Andy Zhou <[email protected]> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 0b66e7c..cb239c8 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
>> +{
>> + const struct ovs_action_hash *act_hash = nla_data(attr);
>> + struct sw_flow_key *key = OVS_CB(skb)->pkt_key;
>> + u32 dp_hash = 0;
>> +
>> + if (act_hash->hash_alg == OVS_HASH_ALG_L4) {
>
> Since this is checked at flow install time and there's only one hash
> algorithm, I don't think that it's necessary to do this check at all.
Sure, will drop the check
>
>> + dp_hash = skb_get_rxhash(skb);
>> + if (!dp_hash)
>> + dp_hash = 0x1;
>
> I don't know if we want to rely on this but I think the check for a
> zero hash is already done in skb_get_rxhash.
I could be wrong on this. It seems some earlier kernel may return 0 on
error, Recent kernels may
return whatever is in skb->rxhash on error.
>
>> diff --git a/datapath/flow.h b/datapath/flow.h
>> index 1bb6ce0..bcc36d2 100644
>> --- a/datapath/flow.h
>> +++ b/datapath/flow.h
>> @@ -74,6 +74,7 @@ struct sw_flow_key {
>> u32 skb_mark; /* SKB mark. */
>> u16 in_port; /* Input switch port (or
>> DP_MAX_PORTS). */
>> } __packed phy; /* Safe when right after 'tun_key'. */
>> + u32 dp_hash; /* Datapath computed hash value. */
>
> The name "dp_hash" sounds like it is somehow a property of the
> datapath. What about "flow_hash" or just "hash"?
I would like to indicate this value is computed in datapath. I don't
mind changing it to
a better name, but flow_hash or hash sounds too generic to me.
>
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 5c32cd0..0a1effa 100644
>> --- a/datapath/flow_netlink.c
>> +++ b/datapath/flow_netlink.c
>> @@ -123,6 +123,7 @@ static bool match_validate(const struct sw_flow_match
>> *match,
>> | (1ULL << OVS_KEY_ATTR_ICMP)
>> | (1ULL << OVS_KEY_ATTR_ICMPV6)
>> | (1ULL << OVS_KEY_ATTR_ARP)
>> + | (1ULL << OVS_KEY_ATTR_DP_HASH)
>> | (1ULL << OVS_KEY_ATTR_ND));
>>
>> /* Always allowed mask fields. */
>> @@ -131,6 +132,10 @@ static bool match_validate(const struct sw_flow_match
>> *match,
>> | (1ULL << OVS_KEY_ATTR_ETHERTYPE));
>>
>> /* Check key attributes. */
>> + if (match->key->dp_hash) {
>> + mask_allowed |= (1ULL << OVS_KEY_ATTR_DP_HASH);
>> + }
>
> I'm not sure that this is necessary - we already limit masks to the
> keys that are present.
O.K. I wlll drop this check.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
