[ovs-dev] [recirc datapath V3 RFC 1/2] datapath: add hash action

[Andy Zhou]

On Thu, Apr 17, 2014 at 2:48 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Wed, Apr 16, 2014 at 11:49 PM, Andy Zhou <azhou at nicira.com> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 82cfd2d..820075f 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
>> +{
>> +       struct sw_flow_key *key = OVS_CB(skb)->pkt_key;
>> +       u32 hash = 0;
>> +
>> +       /* OVS_HASH_ALG_L4 is the only possible hash algorithm.  */
>> +       hash = skb_get_rxhash(skb);
>> +       if (!hash)
>> +               hash = 0x1;
>> +
>> +       key->ovs_flow_hash = hash;
>
> I just realized that this ignores the basis. Isn't that a problem?
>
> We could potentially rehash the hash - we do this in
> flow_table.c:find_bucket() although it's not a perfect solution.
This is a reasonable solution. I will make the change.  WIll fix the
reset in the next rev.  Thanks.
>
>> @@ -511,7 +524,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>                         const struct nlattr *attr, int len, bool keep_skb)
>>  {
>>         /* Every output action needs a separate clone of 'skb', but the common
>> -        * case is just a single output action, so that doing a clone and
>> +        * case is just a single output action, so that doin a clone and
>
> This introduces a typo.
>
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 5c32cd0..f464a5b 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)
>
> I don't think this should be in the list, since these are for things
> that require additional validation.
>
>> @@ -130,7 +131,6 @@ static bool match_validate(const struct sw_flow_match *match,
>>                        | (1ULL << OVS_KEY_ATTR_IN_PORT)
>>                        | (1ULL << OVS_KEY_ATTR_ETHERTYPE));
>>
>> -       /* Check key attributes. */
>
> I think this comment should remain.