Message ID | 1544826743-14826-1-git-send-email-pkusunyifeng@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v2] openvswitch: kernel datapath clone action | expand |
On 12/14/2018 2:32 PM, Yifeng Sun wrote: > Upstream commit: > commit b233504033dbd65740e59681820ccfd0a2a8ec53 > Author: Yifeng Sun <pkusunyifeng@gmail.com> > Date: Mon Jul 2 08:18:03 2018 -0700 > > openvswitch: kernel datapath clone action > > Add 'clone' action to kernel datapath by using existing functions. > When actions within clone don't modify the current flow, the flow > key is not cloned before executing clone actions. > > This is a follow up patch for this incomplete work: > https://patchwork.ozlabs.org/patch/722096/ > > v1 -> v2: > Refactor as advised by reviewer. > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > Signed-off-by: Andy Zhou <azhou@ovn.org> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Co-authored-by: Andy Zhou <azhou@ovn.org> > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > Signed-off-by: Andy Zhou <azhou@ovn.org> > --- > v1->v2: Use stand format for Linux upstream backports, thanks Greg! Looks good, thanks Yifeng! Tested-by: Greg Rose <gvrose8192@gmail.com> Reviewed-by: Greg Rose <gvrose8192@gmail.com> > > datapath/actions.c | 32 ++++++++++ > datapath/flow_netlink.c | 73 +++++++++++++++++++++++ > datapath/linux/compat/include/linux/openvswitch.h | 11 +++- > 3 files changed, 114 insertions(+), 2 deletions(-) > > diff --git a/datapath/actions.c b/datapath/actions.c > index 56b013601393..8abe70aa5109 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > @@ -1068,6 +1068,28 @@ static int sample(struct datapath *dp, struct sk_buff *skb, > clone_flow_key); > } > > +/* When 'last' is true, clone() should always consume the 'skb'. > + * Otherwise, clone() should keep 'skb' intact regardless what > + * actions are executed within clone(). > + */ > +static int clone(struct datapath *dp, struct sk_buff *skb, > + struct sw_flow_key *key, const struct nlattr *attr, > + bool last) > +{ > + struct nlattr *actions; > + struct nlattr *clone_arg; > + int rem = nla_len(attr); > + bool dont_clone_flow_key; > + > + /* The first action is always 'OVS_CLONE_ATTR_ARG'. */ > + clone_arg = nla_data(attr); > + dont_clone_flow_key = nla_get_u32(clone_arg); > + actions = nla_next(clone_arg, &rem); > + > + return clone_execute(dp, skb, key, 0, actions, rem, last, > + !dont_clone_flow_key); > +} > + > static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key, > const struct nlattr *attr) > { > @@ -1347,6 +1369,16 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > consume_skb(skb); > return 0; > } > + break; > + > + case OVS_ACTION_ATTR_CLONE: { > + bool last = nla_is_last(a, rem); > + > + err = clone(dp, skb, key, a, last); > + if (last) > + return err; > + break; > + } > } > > if (unlikely(err)) { > diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c > index ee0c18422236..a572ed3e7345 100644 > --- a/datapath/flow_netlink.c > +++ b/datapath/flow_netlink.c > @@ -2465,6 +2465,40 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr, > return 0; > } > > +static int validate_and_copy_clone(struct net *net, > + const struct nlattr *attr, > + const struct sw_flow_key *key, > + struct sw_flow_actions **sfa, > + __be16 eth_type, __be16 vlan_tci, > + bool log, bool last) > +{ > + int start, err; > + u32 exec; > + > + if (nla_len(attr) && nla_len(attr) < NLA_HDRLEN) > + return -EINVAL; > + > + start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CLONE, log); > + if (start < 0) > + return start; > + > + exec = last || !actions_may_change_flow(attr); > + > + err = ovs_nla_add_action(sfa, OVS_CLONE_ATTR_EXEC, &exec, > + sizeof(exec), log); > + if (err) > + return err; > + > + err = __ovs_nla_copy_actions(net, attr, key, sfa, > + eth_type, vlan_tci, log); > + if (err) > + return err; > + > + add_nested_action_end(*sfa, start); > + > + return 0; > +} > + > void ovs_match_init(struct sw_flow_match *match, > struct sw_flow_key *key, > bool reset_key, > @@ -2852,6 +2886,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > [OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1, > [OVS_ACTION_ATTR_POP_NSH] = 0, > [OVS_ACTION_ATTR_METER] = sizeof(u32), > + [OVS_ACTION_ATTR_CLONE] = (u32)-1, > }; > const struct ovs_action_push_vlan *vlan; > int type = nla_type(a); > @@ -3041,6 +3076,18 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, > /* Non-existent meters are simply ignored. */ > break; > > + case OVS_ACTION_ATTR_CLONE: { > + bool last = nla_is_last(a, rem); > + > + err = validate_and_copy_clone(net, a, key, sfa, > + eth_type, vlan_tci, > + log, last); > + if (err) > + return err; > + skip_copy = true; > + break; > + } > + > default: > OVS_NLERR(log, "Unknown Action type %d", type); > return -EINVAL; > @@ -3119,6 +3166,26 @@ out: > return err; > } > > +static int clone_action_to_attr(const struct nlattr *attr, > + struct sk_buff *skb) > +{ > + struct nlattr *start; > + int err = 0, rem = nla_len(attr); > + > + start = nla_nest_start(skb, OVS_ACTION_ATTR_CLONE); > + if (!start) > + return -EMSGSIZE; > + > + err = ovs_nla_put_actions(nla_data(attr), rem, skb); > + > + if (err) > + nla_nest_cancel(skb, start); > + else > + nla_nest_end(skb, start); > + > + return err; > +} > + > static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb) > { > const struct nlattr *ovs_key = nla_data(a); > @@ -3207,6 +3274,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb) > return err; > break; > > + case OVS_ACTION_ATTR_CLONE: > + err = clone_action_to_attr(a, skb); > + if (err) > + return err; > + break; > + > default: > if (nla_put(skb, type, nla_len(a), nla_data(a))) > return -EMSGSIZE; > diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h > index aaeb0341ab51..9b087f1b0620 100644 > --- a/datapath/linux/compat/include/linux/openvswitch.h > +++ b/datapath/linux/compat/include/linux/openvswitch.h > @@ -909,6 +909,8 @@ enum ovs_nat_attr { > * tunnel header. > * @OVS_ACTION_ATTR_METER: Run packet through a meter, which may drop the > * packet, or modify the packet (e.g., change the DSCP field). > + * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of > + * actions without affecting the original packet and key. > */ > > enum ovs_action_attr { > @@ -934,12 +936,12 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */ > OVS_ACTION_ATTR_PUSH_NSH, /* Nested OVS_NSH_KEY_ATTR_*. */ > OVS_ACTION_ATTR_POP_NSH, /* No argument. */ > - OVS_ACTION_ATTR_METER, /* u32 meter number. */ > + OVS_ACTION_ATTR_METER, /* u32 meter number. */ > + OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ > > #ifndef __KERNEL__ > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */ > - OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ > #endif > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > * from userspace. */ > @@ -1032,4 +1034,9 @@ struct ovs_zone_limit { > __u32 count; > }; > > +#define OVS_CLONE_ATTR_EXEC 0 /* Specify an u32 value. When nonzero, > + * actions in clone will not change flow > + * keys. False otherwise. > + */ > + > #endif /* _LINUX_OPENVSWITCH_H */
On Mon, Dec 17, 2018 at 01:38:05PM -0800, Gregory Rose wrote: > > On 12/14/2018 2:32 PM, Yifeng Sun wrote: > >Upstream commit: > > commit b233504033dbd65740e59681820ccfd0a2a8ec53 > > Author: Yifeng Sun <pkusunyifeng@gmail.com> > > Date: Mon Jul 2 08:18:03 2018 -0700 > > > > openvswitch: kernel datapath clone action > > > > Add 'clone' action to kernel datapath by using existing functions. > > When actions within clone don't modify the current flow, the flow > > key is not cloned before executing clone actions. > > > > This is a follow up patch for this incomplete work: > > https://patchwork.ozlabs.org/patch/722096/ > > > > v1 -> v2: > > Refactor as advised by reviewer. > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > Acked-by: Pravin B Shelar <pshelar@ovn.org> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > >Co-authored-by: Andy Zhou <azhou@ovn.org> > >Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > >Signed-off-by: Andy Zhou <azhou@ovn.org> > >--- > >v1->v2: Use stand format for Linux upstream backports, thanks Greg! > > Looks good, thanks Yifeng! > > Tested-by: Greg Rose <gvrose8192@gmail.com> > Reviewed-by: Greg Rose <gvrose8192@gmail.com> Thanks Yifeng and Greg, I applied this to master.
diff --git a/datapath/actions.c b/datapath/actions.c index 56b013601393..8abe70aa5109 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -1068,6 +1068,28 @@ static int sample(struct datapath *dp, struct sk_buff *skb, clone_flow_key); } +/* When 'last' is true, clone() should always consume the 'skb'. + * Otherwise, clone() should keep 'skb' intact regardless what + * actions are executed within clone(). + */ +static int clone(struct datapath *dp, struct sk_buff *skb, + struct sw_flow_key *key, const struct nlattr *attr, + bool last) +{ + struct nlattr *actions; + struct nlattr *clone_arg; + int rem = nla_len(attr); + bool dont_clone_flow_key; + + /* The first action is always 'OVS_CLONE_ATTR_ARG'. */ + clone_arg = nla_data(attr); + dont_clone_flow_key = nla_get_u32(clone_arg); + actions = nla_next(clone_arg, &rem); + + return clone_execute(dp, skb, key, 0, actions, rem, last, + !dont_clone_flow_key); +} + static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key, const struct nlattr *attr) { @@ -1347,6 +1369,16 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, consume_skb(skb); return 0; } + break; + + case OVS_ACTION_ATTR_CLONE: { + bool last = nla_is_last(a, rem); + + err = clone(dp, skb, key, a, last); + if (last) + return err; + break; + } } if (unlikely(err)) { diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c index ee0c18422236..a572ed3e7345 100644 --- a/datapath/flow_netlink.c +++ b/datapath/flow_netlink.c @@ -2465,6 +2465,40 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr, return 0; } +static int validate_and_copy_clone(struct net *net, + const struct nlattr *attr, + const struct sw_flow_key *key, + struct sw_flow_actions **sfa, + __be16 eth_type, __be16 vlan_tci, + bool log, bool last) +{ + int start, err; + u32 exec; + + if (nla_len(attr) && nla_len(attr) < NLA_HDRLEN) + return -EINVAL; + + start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CLONE, log); + if (start < 0) + return start; + + exec = last || !actions_may_change_flow(attr); + + err = ovs_nla_add_action(sfa, OVS_CLONE_ATTR_EXEC, &exec, + sizeof(exec), log); + if (err) + return err; + + err = __ovs_nla_copy_actions(net, attr, key, sfa, + eth_type, vlan_tci, log); + if (err) + return err; + + add_nested_action_end(*sfa, start); + + return 0; +} + void ovs_match_init(struct sw_flow_match *match, struct sw_flow_key *key, bool reset_key, @@ -2852,6 +2886,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, [OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1, [OVS_ACTION_ATTR_POP_NSH] = 0, [OVS_ACTION_ATTR_METER] = sizeof(u32), + [OVS_ACTION_ATTR_CLONE] = (u32)-1, }; const struct ovs_action_push_vlan *vlan; int type = nla_type(a); @@ -3041,6 +3076,18 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, /* Non-existent meters are simply ignored. */ break; + case OVS_ACTION_ATTR_CLONE: { + bool last = nla_is_last(a, rem); + + err = validate_and_copy_clone(net, a, key, sfa, + eth_type, vlan_tci, + log, last); + if (err) + return err; + skip_copy = true; + break; + } + default: OVS_NLERR(log, "Unknown Action type %d", type); return -EINVAL; @@ -3119,6 +3166,26 @@ out: return err; } +static int clone_action_to_attr(const struct nlattr *attr, + struct sk_buff *skb) +{ + struct nlattr *start; + int err = 0, rem = nla_len(attr); + + start = nla_nest_start(skb, OVS_ACTION_ATTR_CLONE); + if (!start) + return -EMSGSIZE; + + err = ovs_nla_put_actions(nla_data(attr), rem, skb); + + if (err) + nla_nest_cancel(skb, start); + else + nla_nest_end(skb, start); + + return err; +} + static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb) { const struct nlattr *ovs_key = nla_data(a); @@ -3207,6 +3274,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb) return err; break; + case OVS_ACTION_ATTR_CLONE: + err = clone_action_to_attr(a, skb); + if (err) + return err; + break; + default: if (nla_put(skb, type, nla_len(a), nla_data(a))) return -EMSGSIZE; diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index aaeb0341ab51..9b087f1b0620 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -909,6 +909,8 @@ enum ovs_nat_attr { * tunnel header. * @OVS_ACTION_ATTR_METER: Run packet through a meter, which may drop the * packet, or modify the packet (e.g., change the DSCP field). + * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of + * actions without affecting the original packet and key. */ enum ovs_action_attr { @@ -934,12 +936,12 @@ enum ovs_action_attr { OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */ OVS_ACTION_ATTR_PUSH_NSH, /* Nested OVS_NSH_KEY_ATTR_*. */ OVS_ACTION_ATTR_POP_NSH, /* No argument. */ - OVS_ACTION_ATTR_METER, /* u32 meter number. */ + OVS_ACTION_ATTR_METER, /* u32 meter number. */ + OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ #ifndef __KERNEL__ OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */ - OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ #endif __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted * from userspace. */ @@ -1032,4 +1034,9 @@ struct ovs_zone_limit { __u32 count; }; +#define OVS_CLONE_ATTR_EXEC 0 /* Specify an u32 value. When nonzero, + * actions in clone will not change flow + * keys. False otherwise. + */ + #endif /* _LINUX_OPENVSWITCH_H */