[ovs-dev] [recirc datapath V2 2/3] datapath: add hash action

[Jesse Gross]

On Tue, Apr 15, 2014 at 6:21 PM, Andy Zhou <azhou at nicira.com> 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.

> +               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.

> 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"?

> 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.