[ovs-dev] [PATCH 3/4] dpif-xlate : Functions to validate the possibility of combining tunnel actions.
Joe Stringer
joe at ovn.org
Mon Jun 26 17:30:14 UTC 2017
On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chandran at 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 at intel.com>
> Signed-off-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
> Co-authored-by: Zoltán Balogh <zoltan.balogh at 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
>
More information about the dev
mailing list