Thanks for the review. I will send V2.
On Tue, Apr 15, 2014 at 7:14 AM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Sat, Apr 12, 2014 at 3:30 AM, Andy Zhou <az...@nicira.com> wrote:
>> Implements Linux kernel datapath hash action. Hash action computes
>> hash and stores it into current packet key.
>>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>>
>> hash fix
>> ---
>> datapath/actions.c | 19 +++++++++++++++++++
>> datapath/flow.h | 2 ++
>> datapath/flow_netlink.c | 29 ++++++++++++++++++++++++++++-
>> 3 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 0b66e7c..cb239c8 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -460,6 +460,21 @@ static int sample(struct datapath *dp, struct sk_buff
>> *skb,
>> nla_len(acts_list), true);
>> }
>>
>> +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) {
>> + dp_hash = skb_get_rxhash(skb);
>> + if (!dp_hash)
>> + dp_hash = 0x1;
>> + }
>> +
>> + key->dp_hash = dp_hash;
>> +}
> same as userspace datapath, hash algorithm check can be done at flow-install.
O.K.
>
>> +
>> static int execute_set_action(struct sk_buff *skb,
>> const struct nlattr *nested_attr)
>> {
>> @@ -536,6 +551,10 @@ static int do_execute_actions(struct datapath *dp,
>> struct sk_buff *skb,
>> output_userspace(dp, skb, a);
>> break;
>>
>> + case OVS_ACTION_ATTR_HASH:
>> + execute_hash(skb, a);
>> + break;
>> +
>> case OVS_ACTION_ATTR_PUSH_VLAN:
>> err = push_vlan(skb, nla_data(a));
>> if (unlikely(err)) /* skb already freed. */
>> diff --git a/datapath/flow.h b/datapath/flow.h
>> index 1bb6ce0..abbba83 100644
>> --- a/datapath/flow.h
>> +++ b/datapath/flow.h
>> @@ -68,6 +68,8 @@ static inline void ovs_flow_tun_key_init(struct
>> ovs_key_ipv4_tunnel *tun_key,
>> }
>>
>> struct sw_flow_key {
>> + u32 dp_hash; /* Datapath computed hash value. */
>> + u32 recirc_id; /* Recirculation ID */
>
> recirc_id is not used anywhere in this patch.
I will move this into the recirc action patch.
>
>> struct ovs_key_ipv4_tunnel tun_key; /* Encapsulating tunnel key. */
>> struct {
>> u32 priority; /* Packet QoS priority. */
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 5c32cd0..e85326b 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);
>> + }
>> +
>> if (match->key->eth.type == htons(ETH_P_ARP)
>> || match->key->eth.type == htons(ETH_P_RARP)) {
>> key_expected |= 1ULL << OVS_KEY_ATTR_ARP;
>> @@ -252,6 +257,7 @@ static const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
>> [OVS_KEY_ATTR_ICMPV6] = sizeof(struct ovs_key_icmpv6),
>> [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
>> [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
>> + [OVS_KEY_ATTR_DP_HASH] = sizeof(u32),
>> [OVS_KEY_ATTR_TUNNEL] = -1,
>> };
>>
>> @@ -455,6 +461,19 @@ static int ipv4_tun_to_nlattr(struct sk_buff *skb,
>> static int metadata_from_nlattrs(struct sw_flow_match *match, u64 *attrs,
>> const struct nlattr **a, bool is_mask)
>> {
>> +
>> + if (*attrs & (1ULL << OVS_KEY_ATTR_DP_HASH)) {
>> + u32 hash_val = nla_get_u32(a[OVS_KEY_ATTR_DP_HASH]);
>> +
>> + if (!is_mask && !hash_val) {
>> + OVS_NLERR("Hash value can not be zero\n");
>> + return -EINVAL;
>> + }
>> +
>> + SW_FLOW_KEY_PUT(match, dp_hash, hash_val, is_mask);
>> + *attrs &= ~(1ULL << OVS_KEY_ATTR_DP_HASH);
>> + }
> Is hash allowed to be partially masked?
Yes.
>
>> +
>> if (*attrs & (1ULL << OVS_KEY_ATTR_PRIORITY)) {
>> SW_FLOW_KEY_PUT(match, phy.priority,
>> nla_get_u32(a[OVS_KEY_ATTR_PRIORITY]), is_mask);
>> @@ -859,6 +878,7 @@ int ovs_nla_get_flow_metadata(struct sw_flow *flow,
>> flow->key.phy.in_port = DP_MAX_PORTS;
>> flow->key.phy.priority = 0;
>> flow->key.phy.skb_mark = 0;
>> + flow->key.dp_hash = 0;
>
> Comment need to be updated.
>> memset(tun_key, 0, sizeof(flow->key.tun_key));
>>
>> err = parse_flow_nlattrs(attr, a, &attrs);
>> @@ -882,6 +902,10 @@ int ovs_nla_put_flow(const struct sw_flow_key *swkey,
>> struct nlattr *nla, *encap;
>> bool is_mask = (swkey != output);
>>
>> + if (swkey->dp_hash)
>> + if (nla_put_u32(skb, OVS_KEY_ATTR_DP_HASH, output->dp_hash))
>> + goto nla_put_failure;
>> +
> you can use && rather than nested if statement.
O.K.
>
>> if (nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, output->phy.priority))
>> goto nla_put_failure;
>>
>> @@ -1422,7 +1446,8 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
>> [OVS_ACTION_ATTR_PUSH_VLAN] = sizeof(struct
>> ovs_action_push_vlan),
>> [OVS_ACTION_ATTR_POP_VLAN] = 0,
>> [OVS_ACTION_ATTR_SET] = (u32)-1,
>> - [OVS_ACTION_ATTR_SAMPLE] = (u32)-1
>> + [OVS_ACTION_ATTR_SAMPLE] = (u32)-1,
>> + [OVS_ACTION_ATTR_HASH] = sizeof(struct
>> ovs_action_hash)
>> };
>> const struct ovs_action_push_vlan *vlan;
>> int type = nla_type(a);
>> @@ -1449,6 +1474,8 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
>> return -EINVAL;
>> break;
>>
>> + case OVS_ACTION_ATTR_HASH:
>> + break;
>>
>> case OVS_ACTION_ATTR_POP_VLAN:
>> break;
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
