| Message ID | 1497631551-62258-2-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: > Outputting to a patch port is translated by its peer patch port actions. > Refactoring the translation part to use later in the patch series for the > tunnel push. > > 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> Hi Sugesh, Thanks for splitting this out into a separate patch. However, I notice that this patch makes some subtle changes to the logic between the set of code being removed and the set of code being added. Please don't do this; refactoring patches should be purely cosmetic, moving the exact code from one place to another. Then, to make subsequent changes to the logic, those should be in a separate patch with clear reasoning. This helps reviewers because we don't have to try to reason about subtle changes to behaviour in the middle of large code block moves, and it helps developers later on if there needs to be bisection, because the patches which potentially make behaviour changes are separated from cosmetic changes. Narrowing down which patch introduced a behaviour change is easier when it's not hidden in a refactor. A trivial comment on patch titles: usually we use the imperative form, as described here: http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/#email-subject For the module, "ofproto-dpif-xlate" can be pretty long, I see that in the past people have shortened to "xlate" to save characters. So, the title could be more tersely, "xlate: Refactor translation of patch port output actions." I have one more minor comment below. > --- > ofproto/ofproto-dpif-xlate.c | 270 ++++++++++++++++++++++--------------------- > 1 file changed, 136 insertions(+), 134 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index e15e3de..3a2b1d3 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -426,6 +426,10 @@ static void xlate_action_set(struct xlate_ctx *ctx); > static void xlate_commit_actions(struct xlate_ctx *ctx); > > static void > +apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport *in_dev, > + struct xport *out_dev); > + > +static void > ctx_trigger_freeze(struct xlate_ctx *ctx) > { > ctx->exit = true; > @@ -3255,6 +3259,136 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, const struct flow *flow, co > xport_in->xbundle->protected && xport_out->xbundle->protected); > } > > +/* Populate and apply nested actions on 'out_dev'. > + * The nested actions are applied on cloned packets in dp while outputting to > + * either patch or tunnel ports. Where does the packet clone occur? I think I saw some discussion with Andy on this previously, but I'm not seeing it right now. If the packets are not actually being cloned, then at the least I think that this description should more clearly describe what is happening. I see other parts of the code describing something like 'this is equivalent to cloning the packet for the duration of execution'. Alternatively you could describe this function as pushing the OpenFlow state onto the stack, clearing it out, executing the packet through the device, then finally popping the OpenFlow state back off the stack.
Hi Joe, Thank you for looking into the patch series. Please find my answers inline below. Regards _Sugesh > -----Original Message----- > From: Joe Stringer [mailto:joe@ovn.org] > Sent: Monday, June 26, 2017 6:25 PM > To: Chandran, Sugesh <sugesh.chandran@intel.com> > Cc: ovs dev <dev@openvswitch.org>; Zoltán Balogh > <zoltan.balogh@ericsson.com> > Subject: Re: [PATCH 1/4] dpif-xlate : Refactoring translation of patch port > output actions. > > On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chandran@intel.com> > wrote: > > Outputting to a patch port is translated by its peer patch port actions. > > Refactoring the translation part to use later in the patch series for > > the tunnel push. > > > > 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> > > Hi Sugesh, > > Thanks for splitting this out into a separate patch. However, I notice that this > patch makes some subtle changes to the logic between the set of code being > removed and the set of code being added. Please don't do this; refactoring > patches should be purely cosmetic, moving the exact code from one place to > another. Then, to make subsequent changes to the logic, those should be in > a separate patch with clear reasoning. [Sugesh] We haven’t changed any logic between these two block. Only change that we made is, instead of using the peer, we use the out-port param. This allows us to use the same logic for the tunneling. Can you point out what logic change you are specifically referring to. > This helps reviewers because we don't have to try to reason about subtle > changes to behaviour in the middle of large code block moves, and it helps > developers later on if there needs to be bisection, because the patches > which potentially make behaviour changes are separated from cosmetic > changes. Narrowing down which patch introduced a behaviour change is > easier when it's not hidden in a refactor. > > A trivial comment on patch titles: usually we use the imperative form, as > described here: > http://docs.openvswitch.org/en/latest/internals/contributing/submitting- > patches/#email-subject [Sugesh] Ok, Will change in the next series of patch. > > For the module, "ofproto-dpif-xlate" can be pretty long, I see that in the past > people have shortened to "xlate" to save characters. > > So, the title could be more tersely, "xlate: Refactor translation of patch port > output actions." [Sugesh] Sure, will change it in v2 > > I have one more minor comment below. > > > --- > > ofproto/ofproto-dpif-xlate.c | 270 > > ++++++++++++++++++++++--------------------- > > 1 file changed, 136 insertions(+), 134 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c > > b/ofproto/ofproto-dpif-xlate.c index e15e3de..3a2b1d3 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -426,6 +426,10 @@ static void xlate_action_set(struct xlate_ctx > > *ctx); static void xlate_commit_actions(struct xlate_ctx *ctx); > > > > static void > > +apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport > *in_dev, > > + struct xport *out_dev); > > + > > +static void > > ctx_trigger_freeze(struct xlate_ctx *ctx) { > > ctx->exit = true; > > @@ -3255,6 +3259,136 @@ xlate_flow_is_protected(const struct xlate_ctx > *ctx, const struct flow *flow, co > > xport_in->xbundle->protected && > > xport_out->xbundle->protected); } > > > > +/* Populate and apply nested actions on 'out_dev'. > > + * The nested actions are applied on cloned packets in dp while > > +outputting to > > + * either patch or tunnel ports. > > Where does the packet clone occur? I think I saw some discussion with Andy > on this previously, but I'm not seeing it right now. If the packets are not > actually being cloned, then at the least I think that this description should > more clearly describe what is happening. I see other parts of the code > describing something like 'this is equivalent to cloning the packet for the > duration of execution'. Alternatively you could describe this function as > pushing the OpenFlow state onto the stack, clearing it out, executing the > packet through the device, then finally popping the OpenFlow state back off > the stack. [Sugesh] The name clone comes from the past combine patch series. It basically combine the actions from a different device into current one. We could add description saying its equivalent to cloning packets at the time of translation. Will add it in the next series of patch.
On 27 June 2017 at 08:12, Chandran, Sugesh <sugesh.chandran@intel.com> wrote: > Hi Joe, > Thank you for looking into the patch series. > Please find my answers inline below. > > > > Regards > _Sugesh > > >> -----Original Message----- >> From: Joe Stringer [mailto:joe@ovn.org] >> Sent: Monday, June 26, 2017 6:25 PM >> To: Chandran, Sugesh <sugesh.chandran@intel.com> >> Cc: ovs dev <dev@openvswitch.org>; Zoltán Balogh >> <zoltan.balogh@ericsson.com> >> Subject: Re: [PATCH 1/4] dpif-xlate : Refactoring translation of patch port >> output actions. >> >> On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chandran@intel.com> >> wrote: >> > Outputting to a patch port is translated by its peer patch port actions. >> > Refactoring the translation part to use later in the patch series for >> > the tunnel push. >> > >> > 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> >> >> Hi Sugesh, >> >> Thanks for splitting this out into a separate patch. However, I notice that this >> patch makes some subtle changes to the logic between the set of code being >> removed and the set of code being added. Please don't do this; refactoring >> patches should be purely cosmetic, moving the exact code from one place to >> another. Then, to make subsequent changes to the logic, those should be in >> a separate patch with clear reasoning. > [Sugesh] We haven’t changed any logic between these two block. Only change that we > made is, instead of using the peer, we use the out-port param. > This allows us to use the same logic for the tunneling. > Can you point out what logic change you are specifically referring to. Look for "memset.*tunn" in this patch and compare how many such lines are in the new version and how many are in the old. <snip> >> > @@ -3255,6 +3259,136 @@ xlate_flow_is_protected(const struct xlate_ctx >> *ctx, const struct flow *flow, co >> > xport_in->xbundle->protected && >> > xport_out->xbundle->protected); } >> > >> > +/* Populate and apply nested actions on 'out_dev'. >> > + * The nested actions are applied on cloned packets in dp while >> > +outputting to >> > + * either patch or tunnel ports. >> >> Where does the packet clone occur? I think I saw some discussion with Andy >> on this previously, but I'm not seeing it right now. If the packets are not >> actually being cloned, then at the least I think that this description should >> more clearly describe what is happening. I see other parts of the code >> describing something like 'this is equivalent to cloning the packet for the >> duration of execution'. Alternatively you could describe this function as >> pushing the OpenFlow state onto the stack, clearing it out, executing the >> packet through the device, then finally popping the OpenFlow state back off >> the stack. > [Sugesh] The name clone comes from the past combine patch series. > It basically combine the actions from a different device into current one. > We could add description saying its equivalent to cloning packets at the time of translation. > Will add it in the next series of patch. Right, I figured that that's what you were trying to get at but it'd be nice to be just a little more explicit in the comments that this is the case.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e15e3de..3a2b1d3 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -426,6 +426,10 @@ static void xlate_action_set(struct xlate_ctx *ctx); static void xlate_commit_actions(struct xlate_ctx *ctx); static void +apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport *in_dev, + struct xport *out_dev); + +static void ctx_trigger_freeze(struct xlate_ctx *ctx) { ctx->exit = true; @@ -3255,6 +3259,136 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, const struct flow *flow, co xport_in->xbundle->protected && xport_out->xbundle->protected); } +/* Populate and apply nested actions on 'out_dev'. + * The nested actions are applied on cloned packets in dp while outputting to + * either patch or tunnel ports. + * On output to a patch port, the output action will be replaced with set of + * nested actions on the peer patch port. + * Similarly on output to a tunnel port, the post nested actions on + * tunnel are chained up with the tunnel-push action. + */ +static void +apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport *in_dev, + struct xport *out_dev) +{ + struct flow *flow = &ctx->xin->flow; + struct flow old_flow = ctx->xin->flow; + struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; + bool old_conntrack = ctx->conntracked; + bool old_was_mpls = ctx->was_mpls; + ovs_version_t old_version = ctx->xin->tables_version; + struct ofpbuf old_stack = ctx->stack; + union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; + struct ofpbuf old_action_set = ctx->action_set; + struct ovs_list *old_trace = ctx->xin->trace; + uint64_t actset_stub[1024 / 8]; + + ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); + ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); + flow->in_port.ofp_port = out_dev->ofp_port; + flow->metadata = htonll(0); + memset(&flow->tunnel, 0, sizeof flow->tunnel); + memset(&ctx->wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel); + flow->tunnel.metadata.tab = + ofproto_get_tun_tab(&out_dev->xbridge->ofproto->up); + ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; + memset(flow->regs, 0, sizeof flow->regs); + flow->actset_output = OFPP_UNSET; + ctx->conntracked = false; + clear_conntrack(ctx); + ctx->xin->trace = xlate_report(ctx, OFT_BRIDGE, + "bridge(\"%s\")", + out_dev->xbridge->name); + mirror_mask_t old_mirrors = ctx->mirrors; + bool independent_mirrors = out_dev->xbridge != ctx->xbridge; + if (independent_mirrors) { + ctx->mirrors = 0; + } + ctx->xbridge = out_dev->xbridge; + + /* The bridge is now known so obtain its table version. */ + ctx->xin->tables_version + = ofproto_dpif_get_tables_version(ctx->xbridge->ofproto); + + if (!process_special(ctx, out_dev) && may_receive(out_dev, ctx)) { + if (xport_stp_forward_state(out_dev) && + xport_rstp_forward_state(out_dev)) { + xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, + false); + if (!ctx->freezing) { + xlate_action_set(ctx); + } + if (ctx->freezing) { + finish_freezing(ctx); + } + } else { + /* Forwarding is disabled by STP and RSTP. Let OFPP_NORMAL and + * the learning action look at the packet, then drop it. */ + struct flow old_base_flow = ctx->base_flow; + size_t old_size = ctx->odp_actions->size; + mirror_mask_t old_mirrors2 = ctx->mirrors; + + xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, + false); + ctx->mirrors = old_mirrors2; + ctx->base_flow = old_base_flow; + ctx->odp_actions->size = old_size; + + /* Undo changes that may have been done for freezing. */ + ctx_cancel_freeze(ctx); + } + } + + ctx->xin->trace = old_trace; + if (independent_mirrors) { + ctx->mirrors = old_mirrors; + } + ctx->xin->flow = old_flow; + ctx->xbridge = in_dev->xbridge; + ofpbuf_uninit(&ctx->action_set); + ctx->action_set = old_action_set; + ofpbuf_uninit(&ctx->stack); + ctx->stack = old_stack; + + /* Restore calling bridge's lookup version. */ + ctx->xin->tables_version = old_version; + + /* Restore to calling bridge tunneling information */ + ctx->wc->masks.tunnel = old_flow_tnl_wc; + + /* The out bridge popping MPLS should have no effect on the original + * bridge. */ + ctx->was_mpls = old_was_mpls; + + /* The out bridge's conntrack execution should have no effect on the + * original bridge. */ + ctx->conntracked = old_conntrack; + + /* The fact that the out bridge exits (for any reason) does not mean + * that the original bridge should exit. Specifically, if the out + * bridge freezes translation, the original bridge must continue + * processing with the original, not the frozen packet! */ + ctx->exit = false; + + /* Out bridge errors do not propagate back. */ + ctx->error = XLATE_OK; + + if (ctx->xin->resubmit_stats) { + netdev_vport_inc_tx(in_dev->netdev, ctx->xin->resubmit_stats); + netdev_vport_inc_rx(out_dev->netdev, ctx->xin->resubmit_stats); + if (out_dev->bfd) { + bfd_account_rx(out_dev->bfd, ctx->xin->resubmit_stats); + } + } + if (ctx->xin->xcache) { + struct xc_entry *entry; + entry = xlate_cache_add_entry(ctx->xin->xcache, XC_NETDEV); + entry->dev.tx = netdev_ref(in_dev->netdev); + entry->dev.rx = netdev_ref(out_dev->netdev); + entry->dev.bfd = bfd_ref(out_dev->bfd); + } +} + static bool check_output_prerequisites(struct xlate_ctx *ctx, const struct xport *xport, @@ -3363,140 +3497,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, } if (xport->peer) { - const struct xport *peer = xport->peer; - struct flow old_flow = ctx->xin->flow; - struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; - bool old_conntrack = ctx->conntracked; - bool old_was_mpls = ctx->was_mpls; - ovs_version_t old_version = ctx->xin->tables_version; - struct ofpbuf old_stack = ctx->stack; - uint8_t new_stack[1024]; - struct ofpbuf old_action_set = ctx->action_set; - struct ovs_list *old_trace = ctx->xin->trace; - uint64_t actset_stub[1024 / 8]; - - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); - flow->in_port.ofp_port = peer->ofp_port; - flow->metadata = htonll(0); - memset(&flow->tunnel, 0, sizeof flow->tunnel); - flow->tunnel.metadata.tab = ofproto_get_tun_tab( - &peer->xbridge->ofproto->up); - ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; - memset(flow->regs, 0, sizeof flow->regs); - flow->actset_output = OFPP_UNSET; - clear_conntrack(ctx); - ctx->xin->trace = xlate_report(ctx, OFT_BRIDGE, - "bridge(\"%s\")", peer->xbridge->name); - - /* When the patch port points to a different bridge, then the mirrors - * for that bridge clearly apply independently to the packet, so we - * reset the mirror bitmap to zero and then restore it after the packet - * returns. - * - * When the patch port points to the same bridge, this is more of a - * design decision: can mirrors be re-applied to the packet after it - * re-enters the bridge, or should we treat that as doubly mirroring a - * single packet? The former may be cleaner, since it respects the - * model in which a patch port is like a physical cable plugged from - * one switch port to another, but the latter may be less surprising to - * users. We take the latter choice, for now at least. (To use the - * former choice, hard-code 'independent_mirrors' to "true".) */ - mirror_mask_t old_mirrors = ctx->mirrors; - bool independent_mirrors = peer->xbridge != ctx->xbridge; - if (independent_mirrors) { - ctx->mirrors = 0; - } - ctx->xbridge = peer->xbridge; - - /* The bridge is now known so obtain its table version. */ - ctx->xin->tables_version - = ofproto_dpif_get_tables_version(ctx->xbridge->ofproto); - - if (!process_special(ctx, peer) && may_receive(peer, ctx)) { - if (xport_stp_forward_state(peer) && xport_rstp_forward_state(peer)) { - xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, - false); - if (!ctx->freezing) { - xlate_action_set(ctx); - } - if (ctx->freezing) { - finish_freezing(ctx); - } - } else { - /* Forwarding is disabled by STP and RSTP. Let OFPP_NORMAL and - * the learning action look at the packet, then drop it. */ - struct flow old_base_flow = ctx->base_flow; - size_t old_size = ctx->odp_actions->size; - mirror_mask_t old_mirrors2 = ctx->mirrors; - - xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, - false); - ctx->mirrors = old_mirrors2; - ctx->base_flow = old_base_flow; - ctx->odp_actions->size = old_size; - - /* Undo changes that may have been done for freezing. */ - ctx_cancel_freeze(ctx); - } - } - - ctx->xin->trace = old_trace; - if (independent_mirrors) { - ctx->mirrors = old_mirrors; - } - ctx->xin->flow = old_flow; - ctx->xbridge = xport->xbridge; - ofpbuf_uninit(&ctx->action_set); - ctx->action_set = old_action_set; - ofpbuf_uninit(&ctx->stack); - ctx->stack = old_stack; - - /* Restore calling bridge's lookup version. */ - ctx->xin->tables_version = old_version; - - /* Since this packet came in on a patch port (from the perspective of - * the peer bridge), it cannot have useful tunnel information. As a - * result, any wildcards generated on that tunnel also cannot be valid. - * The tunnel wildcards must be restored to their original version since - * the peer bridge uses a separate tunnel metadata table and therefore - * any generated wildcards will be garbage in the context of our - * metadata table. */ - ctx->wc->masks.tunnel = old_flow_tnl_wc; - - /* The peer bridge popping MPLS should have no effect on the original - * bridge. */ - ctx->was_mpls = old_was_mpls; - - /* The peer bridge's conntrack execution should have no effect on the - * original bridge. */ - ctx->conntracked = old_conntrack; - - /* The fact that the peer bridge exits (for any reason) does not mean - * that the original bridge should exit. Specifically, if the peer - * bridge freezes translation, the original bridge must continue - * processing with the original, not the frozen packet! */ - ctx->exit = false; - - /* Peer bridge errors do not propagate back. */ - ctx->error = XLATE_OK; - - if (ctx->xin->resubmit_stats) { - netdev_vport_inc_tx(xport->netdev, ctx->xin->resubmit_stats); - netdev_vport_inc_rx(peer->netdev, ctx->xin->resubmit_stats); - if (peer->bfd) { - bfd_account_rx(peer->bfd, ctx->xin->resubmit_stats); - } - } - if (ctx->xin->xcache) { - struct xc_entry *entry; - - entry = xlate_cache_add_entry(ctx->xin->xcache, XC_NETDEV); - entry->dev.tx = netdev_ref(xport->netdev); - entry->dev.rx = netdev_ref(peer->netdev); - entry->dev.bfd = bfd_ref(peer->bfd); - } - return; + apply_nested_clone_actions(ctx, xport, xport->peer); + return; } memcpy(flow_vlans, flow->vlans, sizeof flow_vlans);