[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