| Message ID | 1495856865-7457-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, May 26, 2017 at 08:47:44PM -0700, Andy Zhou wrote: > There are three methods of translating openflow layer clone action. > Using datapath clone action, datapath sample action or using actions > to negating the any changes within a clone. Xlate logic selects > a specific method depends on datapath features detected at the boot time. > > Currently only xlate_clone() implements the selection logic since it > is the solo user, but patches will add more users. Introduce > new APIs that hide the details of generating datapath clone action. > > Signed-off-by: Andy Zhou <azhou@ovn.org> This adds a nice layer of abstraction. Thanks! I don't think it's necessary to use malloc and free to allocate these structures, if the caller provides an instance of struct compose_clone_info. That seems like a better choice, in my opinion. I think that this implementation fails when it chooses COMPOSE_CLONE_USING_ACTIONS, because in that case it does not save and restore the base flow. A possible improvement to the implementation would be to keep track of the nesting level and fall back from COMPOSE_CLONE_USING_SAMPLE to COMPOSE_CLONE_USING_ACTIONS when it exceeds the datapath's capability. The code might be a little more straightforward without breaking compose_clone_method() into a separate function, because then there is no need to have a separate switch statement to again discover what method to use. But if you prefer this implementation, I understand that too.
On Tue, Jun 06, 2017 at 04:55:19PM -0700, Ben Pfaff wrote: > I think that this implementation fails when it chooses > COMPOSE_CLONE_USING_ACTIONS, because in that case it does not save and > restore the base flow. I see now that I misunderstood. Never mind, on this point.
On Tue, Jun 6, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > On Fri, May 26, 2017 at 08:47:44PM -0700, Andy Zhou wrote: >> There are three methods of translating openflow layer clone action. >> Using datapath clone action, datapath sample action or using actions >> to negating the any changes within a clone. Xlate logic selects >> a specific method depends on datapath features detected at the boot time. >> >> Currently only xlate_clone() implements the selection logic since it >> is the solo user, but patches will add more users. Introduce >> new APIs that hide the details of generating datapath clone action. >> >> Signed-off-by: Andy Zhou <azhou@ovn.org> > > This adds a nice layer of abstraction. Thanks! Thanks for the careful review and comments. > > I don't think it's necessary to use malloc and free to allocate these > structures, if the caller provides an instance of struct > compose_clone_info. That seems like a better choice, in my opinion. > Good point. Malloc is not essential here. Will drop. > I think that this implementation fails when it chooses > COMPOSE_CLONE_USING_ACTIONS, because in that case it does not save and > restore the base flow. > > A possible improvement to the implementation would be to keep track of > the nesting level and fall back from COMPOSE_CLONE_USING_SAMPLE to > COMPOSE_CLONE_USING_ACTIONS when it exceeds the datapath's capability. > If datapath is actually required, then the translation won't be correct. Should we just abort the translation and log an error? > The code might be a little more straightforward without breaking > compose_clone_method() into a separate function, because then there is > no need to have a separate switch statement to again discover what > method to use. But if you prefer this implementation, I understand that > too. It is likely I will drop the compose_clone_method() when the enhancements required to address your comment for the next patch.
On Wed, Jun 07, 2017 at 11:26:56AM -0700, Andy Zhou wrote: > On Tue, Jun 6, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Fri, May 26, 2017 at 08:47:44PM -0700, Andy Zhou wrote: > >> There are three methods of translating openflow layer clone action. > >> Using datapath clone action, datapath sample action or using actions > >> to negating the any changes within a clone. Xlate logic selects > >> a specific method depends on datapath features detected at the boot time. > >> > >> Currently only xlate_clone() implements the selection logic since it > >> is the solo user, but patches will add more users. Introduce > >> new APIs that hide the details of generating datapath clone action. > >> > > A possible improvement to the implementation would be to keep track of > > the nesting level and fall back from COMPOSE_CLONE_USING_SAMPLE to > > COMPOSE_CLONE_USING_ACTIONS when it exceeds the datapath's capability. > > > If datapath is actually required, then the translation won't be > correct. Should we just abort the translation > and log an error? That would imply that we should not provide any support for datapaths that don't support COMPOSE_CLONE_USING_SAMPLE or COMPOSE_CLONE_USING_CLONE, because translation might not be correct in any case. I think it is obvious that that position is too extreme. I would argue that it is ideal if: - We only use "sample" or "clone" in cases where it is actually warranted based on the actions that would be nested inside. - We log an error (and abort?) if we need to use "sample" or "clone" and the datapath can't implement it correctly to the needed depth. Is it difficult to identify cases where we really need "sample" or "clone"?
On Wed, Jun 7, 2017 at 11:40 AM, Ben Pfaff <blp@ovn.org> wrote: > On Wed, Jun 07, 2017 at 11:26:56AM -0700, Andy Zhou wrote: >> On Tue, Jun 6, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: >> > On Fri, May 26, 2017 at 08:47:44PM -0700, Andy Zhou wrote: >> >> There are three methods of translating openflow layer clone action. >> >> Using datapath clone action, datapath sample action or using actions >> >> to negating the any changes within a clone. Xlate logic selects >> >> a specific method depends on datapath features detected at the boot time. >> >> >> >> Currently only xlate_clone() implements the selection logic since it >> >> is the solo user, but patches will add more users. Introduce >> >> new APIs that hide the details of generating datapath clone action. >> >> >> > A possible improvement to the implementation would be to keep track of >> > the nesting level and fall back from COMPOSE_CLONE_USING_SAMPLE to >> > COMPOSE_CLONE_USING_ACTIONS when it exceeds the datapath's capability. >> > >> If datapath is actually required, then the translation won't be >> correct. Should we just abort the translation >> and log an error? > > That would imply that we should not provide any support for datapaths > that don't support COMPOSE_CLONE_USING_SAMPLE or > COMPOSE_CLONE_USING_CLONE, because translation might not be correct in > any case. I think it is obvious that that position is too extreme. I agree. My comment is in the context of exceeding nesting level. I believe it is consist with your #2 point below. > > I would argue that it is ideal if: > > - We only use "sample" or "clone" in cases where it is actually > warranted based on the actions that would be nested inside. > > - We log an error (and abort?) if we need to use "sample" or > "clone" and the datapath can't implement it correctly to the > needed depth. > Agreed. I will post a new version that implements them. > Is it difficult to identify cases where we really need "sample" or > "clone"? I don't think it is. Currently, only nat and meter actions require clone. In addition, the ct() action, without nat, should also be enclosed in clone() so that fragments of an IP packet will behave more naturally.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index f71a9db0a6b3..de419402b9de 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4956,36 +4956,151 @@ xlate_sample_action(struct xlate_ctx *ctx, tunnel_out_port, false); } -/* Use datapath 'clone' or sample to enclose the translation of 'oc'. */ -static void -compose_clone_action(struct xlate_ctx *ctx, const struct ofpact_nest *oc) +enum clone_method { + /* For datapath that does not support clone, nor have a suitable + * sample action that can be used for clone. The xlate logic generates + * actions that restore the packets post clone. + * + * The draw back is not all packet changes inside a clone can be restored. + * For example, NAT'd packet can not be restored easily. Metered packet + * that dropped inside a clone can not be restored. + * + * This method is only used as the last resort for supporting older + * datapaths, and is used mainly for backward compatibility. */ + COMPOSE_CLONE_USING_ACTIONS, + + /* For datapth that supports the 'clone' action. Only OVS + * userspace datapath implements this action. */ + COMPOSE_CLONE_USING_CLONE, + + /* For datapath that does not support 'clone' action, but have a suitable + * sample action implementation for clone. The upstream Linux kernel + * version 4.11 or greater, and kernel module from OVS version 2.8 or + * greater version have suitable sample action implementations. */ + COMPOSE_CLONE_USING_SAMPLE +}; + +struct compose_clone_info { + enum clone_method method; + struct flow old_base_flow; + + union { + struct { + size_t offset; + } clone; + struct { + size_t offset; + size_t ac_offset; + } sample; + }; +}; + +static enum clone_method +compose_clone_method(struct xlate_ctx *ctx) { - size_t clone_offset = nl_msg_start_nested(ctx->odp_actions, - OVS_ACTION_ATTR_CLONE); - do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx); - nl_msg_end_non_empty_nested(ctx->odp_actions, clone_offset); + if (ctx->xbridge->support.clone) { + return COMPOSE_CLONE_USING_CLONE; + } + + if (ctx->xbridge->support.sample_nesting > 3) { + return COMPOSE_CLONE_USING_SAMPLE; + } + + return COMPOSE_CLONE_USING_ACTIONS; } -/* Use datapath 'sample' action to translate clone. */ -static void -compose_clone_action_using_sample(struct xlate_ctx *ctx, - const struct ofpact_nest *oc) +/* Generic datapath clone generation API + * + * This API generates the appropriate datapath nested clone + * netlink messages that surrounds inner actions. The inner + * action are generated with any logic that <xlate actions> + * below implements. + * + * The specific datapath clone action (clone, sample or none) is + * selected based on the datapath features detected at boot time. + * + * Usage + * ===== + * + * compose_clone_info *info; + * info = compose_clone_open(ctx); + * ... + * <xlate actions> + * ... + * compose_clone_close(ctx, info); + * + * These calls can be nested. + * + * 'info' returned from compose_clone_open() can be NULL, or it + * is created within this function. 'info' should be treated as + * an opaque structure, and should be passed later into + * compose_clone_close(), which will properly close the nested datapath + * clone action and dispose memory of 'info'. */ +static struct compose_clone_info * +compose_clone_open(struct xlate_ctx *ctx) { - size_t offset = nl_msg_start_nested(ctx->odp_actions, - OVS_ACTION_ATTR_SAMPLE); + enum clone_method method = compose_clone_method(ctx); - size_t ac_offset = nl_msg_start_nested(ctx->odp_actions, - OVS_SAMPLE_ATTR_ACTIONS); + if (method == COMPOSE_CLONE_USING_ACTIONS) { + return NULL; + } - do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx); + struct compose_clone_info *info = xmalloc(sizeof *info); - if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { - nl_msg_cancel_nested(ctx->odp_actions, offset); - } else { - nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, - UINT32_MAX); /* 100% probability. */ - nl_msg_end_nested(ctx->odp_actions, offset); + info->method = method; + /* Datapath clone action will make sure the pre clone packets + * are used for actions after clone. Save and restore + * ctx->base_flow to reflect this for the openflow pipeline. */ + info->old_base_flow = ctx->base_flow; + + switch (method) { + case COMPOSE_CLONE_USING_CLONE: + info->clone.offset = nl_msg_start_nested(ctx->odp_actions, + OVS_ACTION_ATTR_CLONE); + break; + case COMPOSE_CLONE_USING_SAMPLE: + info->sample.offset = nl_msg_start_nested(ctx->odp_actions, + OVS_ACTION_ATTR_SAMPLE); + + info->sample.ac_offset = nl_msg_start_nested(ctx->odp_actions, + OVS_SAMPLE_ATTR_ACTIONS); + break; + case COMPOSE_CLONE_USING_ACTIONS: + default: + OVS_NOT_REACHED(); + } + + return info; +} + +static void +compose_clone_close(struct xlate_ctx *ctx, struct compose_clone_info *info) +{ + if (!info) { + return; + } + + switch (info->method) { + case COMPOSE_CLONE_USING_CLONE: + nl_msg_end_non_empty_nested(ctx->odp_actions, info->clone.offset); + break; + case COMPOSE_CLONE_USING_SAMPLE: + if (nl_msg_end_non_empty_nested(ctx->odp_actions, + info->sample.ac_offset)) { + nl_msg_cancel_nested(ctx->odp_actions, info->sample.offset); + } else { + nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, + UINT32_MAX); /* 100% probability. */ + nl_msg_end_nested(ctx->odp_actions, info->sample.offset); + } + break; + case COMPOSE_CLONE_USING_ACTIONS: + default: + OVS_NOT_REACHED(); } + + ctx->base_flow = info->old_base_flow; + free(info); } static void @@ -5005,23 +5120,9 @@ xlate_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc) ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size); - /* Datapath clone action will make sure the pre clone packets - * are used for actions after clone. Save and restore - * ctx->base_flow to reflect this for the openflow pipeline. */ - if (ctx->xbridge->support.clone) { - struct flow old_base_flow = ctx->base_flow; - compose_clone_action(ctx, oc); - ctx->base_flow = old_base_flow; - } else if (ctx->xbridge->support.sample_nesting > 3) { - /* Avoid generate sample action if datapath - * only allow small number of nesting. Deeper nesting - * can cause the datapath to reject the generated flow. */ - struct flow old_base_flow = ctx->base_flow; - compose_clone_action_using_sample(ctx, oc); - ctx->base_flow = old_base_flow; - } else { - do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx); - } + struct compose_clone_info *info = compose_clone_open(ctx); + do_xlate_actions(oc->actions, ofpact_nest_get_action_len(oc), ctx); + compose_clone_close(ctx, info); ofpbuf_uninit(&ctx->action_set); ctx->action_set = old_action_set;
There are three methods of translating openflow layer clone action. Using datapath clone action, datapath sample action or using actions to negating the any changes within a clone. Xlate logic selects a specific method depends on datapath features detected at the boot time. Currently only xlate_clone() implements the selection logic since it is the solo user, but patches will add more users. Introduce new APIs that hide the details of generating datapath clone action. Signed-off-by: Andy Zhou <azhou@ovn.org> --- ofproto/ofproto-dpif-xlate.c | 179 +++++++++++++++++++++++++++++++++---------- 1 file changed, 140 insertions(+), 39 deletions(-)