[ovs-dev] [PATCH] datapath: Refactor action alloc and copy api.
Jarno Rajahalme
jrajahalme at nicira.com
Thu Aug 7 21:58:52 UTC 2014
Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
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