[ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.

Pravin Shelar pshelar at nicira.com
Thu Aug 7 22:48:57 UTC 2014


On Thu, Aug 7, 2014 at 2:58 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>
I pushed it to master.

Thanks.
> On Aug 7, 2014, at 12:51 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>
>> There are two separate API to allocate and copy actions list. Anytime
>> OVS needs to copy action list, it needs to call both functions.
>> Following patch moves action allocation to copy function to avoid
>> code duplication.
>>
>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>> ---
>> datapath/datapath.c     | 49 +++++++++----------------------------------------
>> datapath/flow_netlink.c | 24 +++++++++++++++++-------
>> datapath/flow_netlink.h |  1 -
>> 3 files changed, 26 insertions(+), 48 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 19d41c8..62e3c26 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -569,17 +569,12 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>>       if (err)
>>               goto err_flow_free;
>>
>> -     acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_PACKET_ATTR_ACTIONS]));
>> -     err = PTR_ERR(acts);
>> -     if (IS_ERR(acts))
>> -             goto err_flow_free;
>> -
>>       err = ovs_nla_copy_actions(a[OVS_PACKET_ATTR_ACTIONS],
>>                                  &flow->key, &acts);
>> -     rcu_assign_pointer(flow->sf_acts, acts);
>>       if (err)
>>               goto err_flow_free;
>>
>> +     rcu_assign_pointer(flow->sf_acts, acts);
>>       OVS_CB(packet)->flow = flow;
>>       OVS_CB(packet)->pkt_key = &flow->key;
>>       OVS_CB(skb)->egress_tun_info = NULL;
>> @@ -899,11 +894,6 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>       ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
>>
>>       /* Validate actions. */
>> -     acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
>> -     error = PTR_ERR(acts);
>> -     if (IS_ERR(acts))
>> -             goto err_kfree_flow;
>> -
>>       error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
>>                                    &acts);
>>       if (error) {
>> @@ -1000,29 +990,6 @@ error:
>>       return error;
>> }
>>
>> -static struct sw_flow_actions *get_flow_actions(const struct nlattr *a,
>> -                                             const struct sw_flow_key *key,
>> -                                             const struct sw_flow_mask *mask)
>> -{
>> -     struct sw_flow_actions *acts;
>> -     struct sw_flow_key masked_key;
>> -     int error;
>> -
>> -     acts = ovs_nla_alloc_flow_actions(nla_len(a));
>> -     if (IS_ERR(acts))
>> -             return acts;
>> -
>> -     ovs_flow_mask_key(&masked_key, key, mask);
>> -     error = ovs_nla_copy_actions(a, &masked_key, &acts);
>> -     if (error) {
>> -             OVS_NLERR("Flow actions may not be safe on all matching packets.\n");
>> -             kfree(acts);
>> -             return ERR_PTR(error);
>> -     }
>> -
>> -     return acts;
>> -}
>> -
>> static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
>> {
>>       struct nlattr **a = info->attrs;
>> @@ -1051,15 +1018,17 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
>>
>>       /* Validate actions. */
>>       if (a[OVS_FLOW_ATTR_ACTIONS]) {
>> -             acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask);
>> -             if (IS_ERR(acts)) {
>> -                     error = PTR_ERR(acts);
>> +             struct sw_flow_key masked_key;
>> +
>> +             ovs_flow_mask_key(&masked_key, &key, &mask);
>> +             error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
>> +                                          &masked_key, &acts);
>> +             if (error) {
>> +                     OVS_NLERR("Flow actions may not be safe on all matching packets.\n");
>>                       goto error;
>>               }
>> -     }
>>
>> -     /* Can allocate before locking if have acts. */
>> -     if (acts) {
>> +             /* Can allocate before locking if have acts. */
>>               reply = ovs_flow_cmd_alloc_info(acts, info, false);
>>               if (IS_ERR(reply)) {
>>                       error = PTR_ERR(reply);
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 75172de..e4cf535 100644
>> --- a/datapath/flow_netlink.c
>> +++ b/datapath/flow_netlink.c
>> @@ -1239,7 +1239,7 @@ nla_put_failure:
>>
>> #define MAX_ACTIONS_BUFSIZE   (32 * 1024)
>>
>> -struct sw_flow_actions *ovs_nla_alloc_flow_actions(int size)
>> +static struct sw_flow_actions *nla_alloc_flow_actions(int size)
>> {
>>       struct sw_flow_actions *sfa;
>>
>> @@ -1293,7 +1293,7 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa,
>>               new_acts_size = MAX_ACTIONS_BUFSIZE;
>>       }
>>
>> -     acts = ovs_nla_alloc_flow_actions(new_acts_size);
>> +     acts = nla_alloc_flow_actions(new_acts_size);
>>       if (IS_ERR(acts))
>>               return (void *)acts;
>>
>> @@ -1360,7 +1360,7 @@ static inline void add_nested_action_end(struct sw_flow_actions *sfa,
>>       a->nla_len = sfa->actions_len - st_offset;
>> }
>>
>> -static int ovs_nla_copy_actions__(const struct nlattr *attr,
>> +static int __ovs_nla_copy_actions(const struct nlattr *attr,
>>                                 const struct sw_flow_key *key,
>>                                 int depth, struct sw_flow_actions **sfa,
>>                                 __be16 eth_type, __be16 vlan_tci);
>> @@ -1405,7 +1405,7 @@ static int validate_and_copy_sample(const struct nlattr *attr,
>>       if (st_acts < 0)
>>               return st_acts;
>>
>> -     err = ovs_nla_copy_actions__(actions, key, depth + 1, sfa,
>> +     err = __ovs_nla_copy_actions(actions, key, depth + 1, sfa,
>>                                    eth_type, vlan_tci);
>>       if (err)
>>               return err;
>> @@ -1645,7 +1645,7 @@ static int copy_action(const struct nlattr *from,
>>       return 0;
>> }
>>
>> -static int ovs_nla_copy_actions__(const struct nlattr *attr,
>> +static int __ovs_nla_copy_actions(const struct nlattr *attr,
>>                                 const struct sw_flow_key *key,
>>                                 int depth, struct sw_flow_actions **sfa,
>>                                 __be16 eth_type, __be16 vlan_tci)
>> @@ -1794,8 +1794,18 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
>>                        const struct sw_flow_key *key,
>>                        struct sw_flow_actions **sfa)
>> {
>> -     return ovs_nla_copy_actions__(attr, key, 0, sfa, key->eth.type,
>> -                                   key->eth.tci);
>> +     int err;
>> +
>> +     *sfa = nla_alloc_flow_actions(nla_len(attr));
>> +     if (IS_ERR(*sfa))
>> +             return PTR_ERR(*sfa);
>> +
>> +     err = __ovs_nla_copy_actions(attr, key, 0, sfa, key->eth.type,
>> +                                  key->eth.tci);
>> +     if (err)
>> +             kfree(*sfa);
>> +
>> +     return err;
>> }
>>
>> static int sample_action_to_attr(const struct nlattr *attr, struct sk_buff *skb)
>> diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h
>> index 16da264..8ac40b5 100644
>> --- a/datapath/flow_netlink.h
>> +++ b/datapath/flow_netlink.h
>> @@ -54,7 +54,6 @@ int ovs_nla_copy_actions(const struct nlattr *attr,
>> int ovs_nla_put_actions(const struct nlattr *attr,
>>                       int len, struct sk_buff *skb);
>>
>> -struct sw_flow_actions *ovs_nla_alloc_flow_actions(int actions_len);
>> void ovs_nla_free_flow_actions(struct sw_flow_actions *);
>>
>> #endif /* flow_netlink.h */
>> --
>> 1.9.3
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list