| Message ID | 1497631551-62258-4-git-send-email-sugesh.chandran@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Joe Stringer |
| Headers | show |
On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chandran@intel.com> wrote: > Every supported action in the datapath cannot combine with tunnel push. > for eg: TRUNCATE action cannot apply with tunnel_push operation due to the > wrong stats update. These functions do a trial translation on a tunnel output > to see the feasibility of combining the post tunnel push actions. > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com> > Signed-off-by: Zoltán Balogh <zoltan.balogh@ericsson.com> > Co-authored-by: Zoltán Balogh <zoltan.balogh@ericsson.com> > --- I have some feedback below, and like patch #2 I think it makes sense to fold this with patch #4. I'll save the higher level feedback for the next patch. > ofproto/ofproto-dpif-xlate.c | 73 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index d3a1624..c110f2a 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3230,6 +3230,79 @@ copy_flow(struct flow *dst, const struct flow *src) > memcpy(dst, src, sizeof *dst); > } > > +static bool > +is_tunnel_actions_clone_ready(struct ofpbuf *actions) > +{ > + struct nlattr *tnl_actions; > + const struct nlattr *a; > + unsigned int left; > + size_t actions_len; > + > + if (!actions) { > + /* No actions, no harm in doing combine */ > + return true; > + } > + actions_len = actions->size; > + > + /* Post tunnel actions */ > + tnl_actions =(struct nlattr *)(actions->data); > + NL_ATTR_FOR_EACH_UNSAFE (a, left, tnl_actions, actions_len) { > + int type = nl_attr_type(a); > + if (type == OVS_ACTION_ATTR_TRUNC) { > + VLOG_DBG("Cannot do tunnel action-combine on trunc action"); > + return false; > + break; > + } > + } > + return true; > +} > + > +static bool > +nested_clone_valid_on_tunnel(struct xlate_ctx *ctx, const struct xport *xport, > + struct xport *out_dev, > + struct ovs_action_push_tnl *tnl_push_data) > +{ > + const struct dpif_flow_stats *bckup_resubmit_stats; > + struct xlate_cache *bckup_xcache; > + uint32_t action_size = 0; > + bool nested_act_flag = false; > + struct flow_wildcards tmp_flow_wc; > + struct flow_wildcards *flow_wc_ptr; > + struct flow bckup_baseflow, bckup_flow; Does dropping the 'a' character from backup save some character limit or something? Why not 'backup..'? > + > + copy_flow(&bckup_baseflow, &ctx->base_flow); > + copy_flow(&bckup_flow, &ctx->xin->flow); > + memset(&tmp_flow_wc, 0 , sizeof tmp_flow_wc); > + > + flow_wc_ptr = ctx->wc; > + ctx->wc = &tmp_flow_wc; > + > + ctx->xin->wc = NULL; > + if (ctx->odp_actions) { > + action_size = ctx->odp_actions->size; > + } > + bckup_resubmit_stats = ctx->xin->resubmit_stats; > + bckup_xcache = ctx->xin->xcache; > + ctx->xin->resubmit_stats = NULL; > + ctx->xin->xcache = NULL; Considering that this function is supposed to be a stateless query on the validity of running a set of actions within a clone, I think you should consider at least setting ctx->xin->allow_side_effects to false, and perhaps also clearing ctx->xin->packet to make sure that it isn't modified by later translation. I don't think it should be modified later, but if it's not available, then there's no chance for it to be. > + odp_put_tnl_push_action(ctx->odp_actions, tnl_push_data); > + apply_nested_clone_actions(ctx, xport, out_dev); > + nested_act_flag = is_tunnel_actions_clone_ready(ctx->odp_actions); > + > + /* restore context status */ > + ctx->xin->resubmit_stats = bckup_resubmit_stats; > + ctx->xin->xcache = bckup_xcache; > + if (ctx->odp_actions) { > + ctx->odp_actions->size = action_size; /* Restore actions */ > + } > + /* revert the flows, as the validation might changed them */ > + copy_flow(&ctx->base_flow, &bckup_baseflow); > + copy_flow(&ctx->xin->flow, &bckup_flow); > + ctx->wc = flow_wc_ptr; > + return nested_act_flag; > + > +} > + > static int > build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, > const struct flow *flow, odp_port_t tunnel_odp_port) > -- > 2.7.4 >
Regards _Sugesh > -----Original Message----- > From: Joe Stringer [mailto:joe@ovn.org] > Sent: Monday, June 26, 2017 6:30 PM > To: Chandran, Sugesh <sugesh.chandran@intel.com> > Cc: ovs dev <dev@openvswitch.org>; Zoltán Balogh > <zoltan.balogh@ericsson.com>; Andy Zhou <azhou@ovn.org> > Subject: Re: [PATCH 3/4] dpif-xlate : Functions to validate the possibility of > combining tunnel actions. > > On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chandran@intel.com> > wrote: > > Every supported action in the datapath cannot combine with tunnel push. > > for eg: TRUNCATE action cannot apply with tunnel_push operation due to > > the wrong stats update. These functions do a trial translation on a > > tunnel output to see the feasibility of combining the post tunnel push > actions. > > > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com> > > Signed-off-by: Zoltán Balogh <zoltan.balogh@ericsson.com> > > Co-authored-by: Zoltán Balogh <zoltan.balogh@ericsson.com> > > --- > > I have some feedback below, and like patch #2 I think it makes sense to fold > this with patch #4. I'll save the higher level feedback for the next patch. [Sugesh] Will merge them into one in v2. > > > ofproto/ofproto-dpif-xlate.c | 73 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 73 insertions(+) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c > > b/ofproto/ofproto-dpif-xlate.c index d3a1624..c110f2a 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -3230,6 +3230,79 @@ copy_flow(struct flow *dst, const struct flow > *src) > > memcpy(dst, src, sizeof *dst); > > } > > > > +static bool > > +is_tunnel_actions_clone_ready(struct ofpbuf *actions) { > > + struct nlattr *tnl_actions; > > + const struct nlattr *a; > > + unsigned int left; > > + size_t actions_len; > > + > > + if (!actions) { > > + /* No actions, no harm in doing combine */ > > + return true; > > + } > > + actions_len = actions->size; > > + > > + /* Post tunnel actions */ > > + tnl_actions =(struct nlattr *)(actions->data); > > + NL_ATTR_FOR_EACH_UNSAFE (a, left, tnl_actions, actions_len) { > > + int type = nl_attr_type(a); > > + if (type == OVS_ACTION_ATTR_TRUNC) { > > + VLOG_DBG("Cannot do tunnel action-combine on trunc action"); > > + return false; > > + break; > > + } > > + } > > + return true; > > +} > > + > > +static bool > > +nested_clone_valid_on_tunnel(struct xlate_ctx *ctx, const struct xport > *xport, > > + struct xport *out_dev, > > + struct ovs_action_push_tnl > > +*tnl_push_data) { > > + const struct dpif_flow_stats *bckup_resubmit_stats; > > + struct xlate_cache *bckup_xcache; > > + uint32_t action_size = 0; > > + bool nested_act_flag = false; > > + struct flow_wildcards tmp_flow_wc; > > + struct flow_wildcards *flow_wc_ptr; > > + struct flow bckup_baseflow, bckup_flow; > > Does dropping the 'a' character from backup save some character limit or > something? Why not 'backup..'? [Sugesh] will change it. > > > + > > + copy_flow(&bckup_baseflow, &ctx->base_flow); > > + copy_flow(&bckup_flow, &ctx->xin->flow); > > + memset(&tmp_flow_wc, 0 , sizeof tmp_flow_wc); > > + > > + flow_wc_ptr = ctx->wc; > > + ctx->wc = &tmp_flow_wc; > > + > > + ctx->xin->wc = NULL; > > + if (ctx->odp_actions) { > > + action_size = ctx->odp_actions->size; > > + } > > + bckup_resubmit_stats = ctx->xin->resubmit_stats; > > + bckup_xcache = ctx->xin->xcache; > > + ctx->xin->resubmit_stats = NULL; > > + ctx->xin->xcache = NULL; > > Considering that this function is supposed to be a stateless query on the > validity of running a set of actions within a clone, I think you should consider > at least setting ctx->xin->allow_side_effects to false, and perhaps also > clearing ctx->xin->packet to make sure that it isn't modified by later > translation. I don't think it should be modified later, but if it's not available, > then there's no chance for it to be. [Sugesh] Sure, will set them while doing the validation. > > > + odp_put_tnl_push_action(ctx->odp_actions, tnl_push_data); > > + apply_nested_clone_actions(ctx, xport, out_dev); > > + nested_act_flag = > > + is_tunnel_actions_clone_ready(ctx->odp_actions); > > + > > + /* restore context status */ > > + ctx->xin->resubmit_stats = bckup_resubmit_stats; > > + ctx->xin->xcache = bckup_xcache; > > + if (ctx->odp_actions) { > > + ctx->odp_actions->size = action_size; /* Restore actions */ > > + } > > + /* revert the flows, as the validation might changed them */ > > + copy_flow(&ctx->base_flow, &bckup_baseflow); > > + copy_flow(&ctx->xin->flow, &bckup_flow); > > + ctx->wc = flow_wc_ptr; > > + return nested_act_flag; > > + > > +} > > + > > static int > > build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, > > const struct flow *flow, odp_port_t > > tunnel_odp_port) > > -- > > 2.7.4 > >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index d3a1624..c110f2a 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3230,6 +3230,79 @@ copy_flow(struct flow *dst, const struct flow *src) memcpy(dst, src, sizeof *dst); } +static bool +is_tunnel_actions_clone_ready(struct ofpbuf *actions) +{ + struct nlattr *tnl_actions; + const struct nlattr *a; + unsigned int left; + size_t actions_len; + + if (!actions) { + /* No actions, no harm in doing combine */ + return true; + } + actions_len = actions->size; + + /* Post tunnel actions */ + tnl_actions =(struct nlattr *)(actions->data); + NL_ATTR_FOR_EACH_UNSAFE (a, left, tnl_actions, actions_len) { + int type = nl_attr_type(a); + if (type == OVS_ACTION_ATTR_TRUNC) { + VLOG_DBG("Cannot do tunnel action-combine on trunc action"); + return false; + break; + } + } + return true; +} + +static bool +nested_clone_valid_on_tunnel(struct xlate_ctx *ctx, const struct xport *xport, + struct xport *out_dev, + struct ovs_action_push_tnl *tnl_push_data) +{ + const struct dpif_flow_stats *bckup_resubmit_stats; + struct xlate_cache *bckup_xcache; + uint32_t action_size = 0; + bool nested_act_flag = false; + struct flow_wildcards tmp_flow_wc; + struct flow_wildcards *flow_wc_ptr; + struct flow bckup_baseflow, bckup_flow; + + copy_flow(&bckup_baseflow, &ctx->base_flow); + copy_flow(&bckup_flow, &ctx->xin->flow); + memset(&tmp_flow_wc, 0 , sizeof tmp_flow_wc); + + flow_wc_ptr = ctx->wc; + ctx->wc = &tmp_flow_wc; + + ctx->xin->wc = NULL; + if (ctx->odp_actions) { + action_size = ctx->odp_actions->size; + } + bckup_resubmit_stats = ctx->xin->resubmit_stats; + bckup_xcache = ctx->xin->xcache; + ctx->xin->resubmit_stats = NULL; + ctx->xin->xcache = NULL; + odp_put_tnl_push_action(ctx->odp_actions, tnl_push_data); + apply_nested_clone_actions(ctx, xport, out_dev); + nested_act_flag = is_tunnel_actions_clone_ready(ctx->odp_actions); + + /* restore context status */ + ctx->xin->resubmit_stats = bckup_resubmit_stats; + ctx->xin->xcache = bckup_xcache; + if (ctx->odp_actions) { + ctx->odp_actions->size = action_size; /* Restore actions */ + } + /* revert the flows, as the validation might changed them */ + copy_flow(&ctx->base_flow, &bckup_baseflow); + copy_flow(&ctx->xin->flow, &bckup_flow); + ctx->wc = flow_wc_ptr; + return nested_act_flag; + +} + static int build_tunnel_send(struct xlate_ctx *ctx, const struct xport *xport, const struct flow *flow, odp_port_t tunnel_odp_port)