| Message ID | 20170303032952.1125-3-joe@ovn.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Mar 2, 2017, at 7:29 PM, Joe Stringer <joe@ovn.org> wrote: > > From: andy zhou <azhou@ovn.org> > > Upstream commit: > commit 5b8784aaf29be20ba8d363e1124d7436d42ef9bf > Author: Andy Zhou <azhou@ovn.org> > Date: Fri Jan 27 13:45:28 2017 -0800 > > openvswitch: Simplify do_execute_actions(). > > do_execute_actions() implements a worthwhile optimization: in case > an output action is the last action in an action list, skb_clone() > can be avoided by outputing the current skb. However, the > implementation is more complicated than necessary. This patch > simplify this logic. > > 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> > > Upstream: 5b8784aaf29b ("openvswitch: Simplify do_execute_actions().") > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > datapath/actions.c | 42 ++++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 22 deletions(-) > > diff --git a/datapath/actions.c b/datapath/actions.c > index 3af34357ecd0..abb6637133b0 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > @@ -1125,12 +1125,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > struct sw_flow_key *key, > const struct nlattr *attr, int len) > { > - /* Every output action needs a separate clone of 'skb', but the common > - * case is just a single output action, so that doing a clone and > - * then freeing the original skbuff is wasteful. So the following code > - * is slightly obscure just to avoid that. > - */ > - int prev_port = -1; > const struct nlattr *a; > int rem; > > @@ -1138,20 +1132,28 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > a = nla_next(a, &rem)) { > int err = 0; > > - if (unlikely(prev_port != -1)) { > - struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC); > - > - if (out_skb) > - do_output(dp, out_skb, prev_port, key); > + switch (nla_type(a)) { > + case OVS_ACTION_ATTR_OUTPUT: { > + int port = nla_get_u32(a); > + struct sk_buff *clone; > + > + /* Every output action needs a separate clone > + * of 'skb', In case the output action is the > + * last action, cloning can be avoided. > + */ > + if (nla_is_last(a, rem)) { > + do_output(dp, skb, port, key); > + /* 'skb' has been used for output. > + */ > + return 0; > + } > > + clone = skb_clone(skb, GFP_ATOMIC); > + if (clone) > + do_output(dp, clone, port, key); > OVS_CB(skb)->cutlen = 0; > - prev_port = -1; > - } > - > - switch (nla_type(a)) { > - case OVS_ACTION_ATTR_OUTPUT: > - prev_port = nla_get_u32(a); > break; > + } > > case OVS_ACTION_ATTR_TRUNC: { > struct ovs_action_trunc *trunc = nla_data(a); > @@ -1241,11 +1243,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > } > } > > - if (prev_port != -1) > - do_output(dp, skb, prev_port, key); > - else > - consume_skb(skb); > - > + consume_skb(skb); > return 0; > } > > -- > 2.11.1 >
On 3 March 2017 at 11:03, Jarno Rajahalme <jarno@ovn.org> wrote:
> Acked-by: Jarno Rajahalme <jarno@ovn.org>
Thanks, applied.
diff --git a/datapath/actions.c b/datapath/actions.c index 3af34357ecd0..abb6637133b0 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -1125,12 +1125,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, struct sw_flow_key *key, const struct nlattr *attr, int len) { - /* Every output action needs a separate clone of 'skb', but the common - * case is just a single output action, so that doing a clone and - * then freeing the original skbuff is wasteful. So the following code - * is slightly obscure just to avoid that. - */ - int prev_port = -1; const struct nlattr *a; int rem; @@ -1138,20 +1132,28 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, a = nla_next(a, &rem)) { int err = 0; - if (unlikely(prev_port != -1)) { - struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC); - - if (out_skb) - do_output(dp, out_skb, prev_port, key); + switch (nla_type(a)) { + case OVS_ACTION_ATTR_OUTPUT: { + int port = nla_get_u32(a); + struct sk_buff *clone; + + /* Every output action needs a separate clone + * of 'skb', In case the output action is the + * last action, cloning can be avoided. + */ + if (nla_is_last(a, rem)) { + do_output(dp, skb, port, key); + /* 'skb' has been used for output. + */ + return 0; + } + clone = skb_clone(skb, GFP_ATOMIC); + if (clone) + do_output(dp, clone, port, key); OVS_CB(skb)->cutlen = 0; - prev_port = -1; - } - - switch (nla_type(a)) { - case OVS_ACTION_ATTR_OUTPUT: - prev_port = nla_get_u32(a); break; + } case OVS_ACTION_ATTR_TRUNC: { struct ovs_action_trunc *trunc = nla_data(a); @@ -1241,11 +1243,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, } } - if (prev_port != -1) - do_output(dp, skb, prev_port, key); - else - consume_skb(skb); - + consume_skb(skb); return 0; }