Thanks Greg, I will fix it in a new patch.
Yifeng
On Thu, Dec 13, 2018 at 8:38 PM Gregory Rose <[email protected]> wrote:
> On 12/7/2018 11:20 AM, Yifeng Sun wrote:
> > This backport is based on upstream net-next commit b233504033db
> > ("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/
> >
> > Co-authored-by: Andy Zhou <[email protected]>
> > Signed-off-by: Yifeng Sun <[email protected]>
> > Signed-off-by: Andy Zhou <[email protected]>
> > Acked-by: Pravin B Shelar <[email protected]>
> > ---
> > datapath/actions.c | 32 ++++++++++
> > datapath/flow_netlink.c | 73
> +++++++++++++++++++++++
> > datapath/linux/compat/include/linux/openvswitch.h | 11 +++-
> > 3 files changed, 114 insertions(+), 2 deletions(-)
>
> Hi Yifeng,
>
> thanks for backporting this upstream patch. Everything looks fine to me
> so far as the patch is concerned but
> the commit message does not follow the standard format we use for Linux
> kernel upstream backports.
> Those generally follow the form used in this example:
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-December/354523.html
>
> If you could rework the commit message then I'll provide a 'reviewed-by'
> tag.
>
> Thanks!
>
> - Greg
>
>
> > 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 */
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
