[ovs-dev] [PATCH v2 4/4] tunneling: Avoid datapath-recirc by combining recirc actions at xlate.

Chandran, Sugesh sugesh.chandran at intel.com
Tue Jul 18 17:50:21 UTC 2017


Thank you Joe,
I sent out the v3 patch . Please have a look


Regards
_Sugesh


> -----Original Message-----
> From: Joe Stringer [mailto:joe at ovn.org]
> Sent: Tuesday, July 18, 2017 6:29 PM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> Cc: Zoltán Balogh <zoltan.balogh at ericsson.com>; ovs dev
> <dev at openvswitch.org>; Andy Zhou <azhou at ovn.org>
> Subject: Re: [PATCH v2 4/4] tunneling: Avoid datapath-recirc by combining
> recirc actions at xlate.
> 
> On 18 July 2017 at 01:59, Chandran, Sugesh <sugesh.chandran at intel.com>
> wrote:
> >
> >
> > Regards
> > _Sugesh
> >
> >
> >> -----Original Message-----
> >> From: Joe Stringer [mailto:joe at ovn.org]
> >> Sent: Tuesday, July 18, 2017 6:42 AM
> >> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> >> Cc: Zoltán Balogh <zoltan.balogh at ericsson.com>; ovs dev
> >> <dev at openvswitch.org>; Andy Zhou <azhou at ovn.org>
> >> Subject: Re: [PATCH v2 4/4] tunneling: Avoid datapath-recirc by
> >> combining recirc actions at xlate.
> >>
> >> On 17 July 2017 at 15:17, Chandran, Sugesh
> >> <sugesh.chandran at intel.com>
> >> wrote:
> >> > Comments inline
> >> >
> >> > Regards
> >> > _Sugesh
> >> >
> >> >> -----Original Message-----
> >> >> From: Joe Stringer [mailto:joe at ovn.org]
> >> >> Sent: Monday, July 17, 2017 7:33 PM
> >> >> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> >> >> Cc: Zoltán Balogh <zoltan.balogh at ericsson.com>; ovs dev
> >> >> <dev at openvswitch.org>; Andy Zhou <azhou at ovn.org>
> >> >> Subject: Re: [PATCH v2 4/4] tunneling: Avoid datapath-recirc by
> >> >> combining recirc actions at xlate.
> >> >>
> >> >> On 17 July 2017 at 10:27, Chandran, Sugesh
> >> >> <sugesh.chandran at intel.com>
> >> >> wrote:
> >> >> >> > -----Original Message-----
> >> >> >> > From: Joe Stringer [mailto:joe at ovn.org]
> >> >> >> > Sent: Saturday, July 15, 2017 1:46 AM
> >> >> >> > To: Sugesh Chandran <sugesh.chandran at intel.com>
> >> >> >> > Cc: ovs dev <dev at openvswitch.org>; Andy Zhou
> <azhou at ovn.org>;
> >> >> >> > Zoltán Balogh <zoltan.balogh at ericsson.com>
> >> >> >> > Subject: Re: [PATCH v2 4/4] tunneling: Avoid datapath-recirc
> >> >> >> > by combining
> >> >> >> recirc actions at xlate.
> >> >> >> >
> >> >> >> > On 4 July 2017 at 02:46, Sugesh Chandran
> >> >> >> > <sugesh.chandran at intel.com>
> >> >> >> wrote:
> >> >>
> >> >> <snip>
> >> >>
> >> >> >> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> >> >> >> > > 4e29085..4d996c1 100644
> >> >> >> > > --- a/lib/dpif-netdev.c
> >> >> >> > > +++ b/lib/dpif-netdev.c
> >> >> >> > > @@ -5028,24 +5028,8 @@ dp_execute_cb(void *aux_, struct
> >> >> >> > > dp_packet_batch *packets_,
> >> >> >> > >
> >> >> >> > >      case OVS_ACTION_ATTR_TUNNEL_PUSH:
> >> >> >> > >          if (*depth < MAX_RECIRC_DEPTH) {
> >> >> >> > > -            struct dp_packet_batch tnl_pkt;
> >> >> >> > > -            struct dp_packet_batch *orig_packets_ = packets_;
> >> >> >> > > -            int err;
> >> >> >> > > -
> >> >> >> > > -            if (!may_steal) {
> >> >> >> > > -                dp_packet_batch_clone(&tnl_pkt, packets_);
> >> >> >> > > -                packets_ = &tnl_pkt;
> >> >> >> > > -                dp_packet_batch_reset_cutlen(orig_packets_);
> >> >> >> > > -            }
> >> >> >> > > -
> >> >> >> > >              dp_packet_batch_apply_cutlen(packets_);
> >> >> >> > > -
> >> >> >> > > -            err = push_tnl_action(pmd, a, packets_);
> >> >> >> > > -            if (!err) {
> >> >> >> > > -                (*depth)++;
> >> >> >> > > -                dp_netdev_recirculate(pmd, packets_);
> >> >> >> > > -                (*depth)--;
> >> >> >> > > -            }
> >> >> >> > > +            push_tnl_action(pmd, a, packets_);
> >> >> >> >
> >> >> >> > If I follow, this was the previous datapath-level code to
> >> >> >> > ensure that when packets are output to a tunnel, only the
> >> >> >> > copy of the packet that goes to the tunnel gets the cutlen
> >> >> >> > applied. With the new version of the code, we replace this by
> >> >> >> > making sure that the ofproto layer generates a clone to wrap
> >> >> >> > the push_tunnel and all subsequent bridge translation, so
> >> >> >> > then it results in the equivalent
> >> >> >> > behaviour: The cutlen is only ever applied to a clone of the
> >> >> >> > packets. Is
> >> >> that right?
> >> >> > [Sugesh] Hmm. That’s the good catch. Looks like the packets are
> >> >> > not cloned when combine_tunnel_action is found impossible in
> >> >> > ofproto
> >> layer.
> >> >> >
> >> >> > As a fix we should keep the code block in 'may_steal' to handle
> >> >> > the non tunnel-combine case.
> >> >>
> >> >> Oh, really? I thought that we always end up with a clone wrapping
> >> >> the tunnel_push now, which means we shouldn't need this bit any
> >> >> more. My comment was intended just to ask if I understood
> >> >> correctly to make sure we don't miss anything.
> >> >>
> >> >> > Any other suggestion to apply cut_len in ofproto?
> >> >>
> >> >> Unfortunately the cut_len is set up through a previous truncate
> >> >> action, and it is supposed to apply to the "next output action"
> >> >> (and reset afterwards), so it is part of the datapath actions
> >> >> interface definition; I don't think there's a way to push it up to ofproto.
> >> > [Sugesh] Or at ofproto layer,  the non-combine tunnel push also
> >> > placed in a clone, similar to the combine case? That will help to
> >> > keep the datapath
> >> as simple as just a push.
> >>
> >> That sounds simpler, but I don't think we can get rid of applying the
> >> cut_len just yet. This is related to some other discussions going on
> >> around the interaction between truncate() and clone(), so it's
> >> probably a bit safer to just leave this code to apply cut len in place for
> now.
> > [Sugesh] I don’t meant to get rid of cut_len. Instead we make sure any
> > tunnel-push always happen on a clone. We keep the push +cut_len as is in
> the patch.
> > So the ofproto actions would become
> > clone(tnl_push), trunc(200) -->Non-combine case. (instead of just
> > [tnl_push, trunc(200)] clone(tnl_push, output(1)) ---> Combine case Do
> > you think it has any other implication that we may missed?
> 
> OK, that sounds about right, except that the truncate action is serialized first -
> see xlate_output_trunc_action(). Ie, from a translation perspective it
> generates trunc(),clone(tnl_push), then from datapath perspective when
> executing, the trunc() sets the cut_len, then the clone(tnl_push) is
> responsible for cutting the packet before pushing the header.
> 
> I'm not aware of any other implications of this.
> 
> >> >> >> > > +static bool
> >> >> >> > > +validate_and_combine_post_tnl_actions(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 *backup_resubmit_stats;
> >> >> >> > > +    struct xlate_cache *backup_xcache;
> >> >> >> > > +    bool nested_act_flag = false;
> >> >> >> > > +    struct flow_wildcards tmp_flow_wc;
> >> >> >> > > +    struct flow_wildcards *backup_flow_wc_ptr;
> >> >> >> > > +    bool backup_side_effects;
> >> >> >> > > +    const struct dp_packet *backup_pkt;
> >> >> >> > > +
> >> >> >> > > +    memset(&tmp_flow_wc, 0 , sizeof tmp_flow_wc);
> >> >> >> > > +    backup_flow_wc_ptr = ctx->wc;
> >> >> >> > > +    ctx->wc = &tmp_flow_wc;
> >> >> >> > > +    ctx->xin->wc = NULL;
> >> >> >> > > +    backup_resubmit_stats = ctx->xin->resubmit_stats;
> >> >> >> > > +    backup_xcache = ctx->xin->xcache;
> >> >> >> > > +    backup_side_effects = ctx->xin->allow_side_effects;
> >> >> >> > > +    backup_pkt = ctx->xin->packet;
> >> >> >> > > +
> >> >> >> > > +    size_t push_action_size = 0;
> >> >> >> > > +    size_t clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> >> >> >> > > +                                           OVS_ACTION_ATTR_CLONE);
> >> >> >> > > +    odp_put_tnl_push_action(ctx->odp_actions,
> &tnl_push_data);
> >> >> >> > > +    push_action_size = ctx->odp_actions->size;
> >> >> >> > > +
> >> >> >> > > +    ctx->xin->resubmit_stats =  NULL;
> >> >> >> > > +    ctx->xin->xcache = xlate_cache_new(); /* Use new
> >> >> >> > > + temporary
> >> >> cache.
> >> >> >> */
> >> >> >> > > +    ctx->xin->allow_side_effects = false;
> >> >> >> > > +    ctx->xin->packet = NULL;
> >> >> >> >
> >> >> >> > I realised that you asked about this after the previous
> >> >> >> > patch, but I didn't understand the implications of it. I
> >> >> >> > believe that this packet is how, for instance, the second
> >> >> >> > bridge can forward packets to the controller. If you reset it
> >> >> >> > to NULL here, then I don't think that the controller action
> >> >> >> > is executed correctly during the second bridge translation. (That's
> a test we could add).
> >> >> > [Sugesh] Yes, that’s a good test case for it.
> >> >> >> >
> >> >> >> > I suspect that some multicast snooping stuff and some other
> >> >> >> > pieces that would be affected by this.
> >> >> > [Sugesh] This is interesting. So does the controller expects a
> >> >> > encaped packets in that case? Do you think, the controller
> >> >> > action also be an exception case like the TRUNC?
> >> >>
> >> >> Yes, I think the controller expects encapped packets in this case.
> >> >> Yes, I think that the easiest way to handle this is to put
> >> >> controller in that exception case with truncate.
> >> > [Sugesh] Ok.  Will add that case as an exception.
> >> >
> >> > Before that, Would like to clarify the controller output action bit
> >> > more How does the following case are currently handled that
> >> > involves controller action (We haven’t run any test case that
> >> > involves a controller and very limited knowledge on that side)
> >> >
> >> > MOD_VLAN(10) --> (PATCH-PORT) --> (SECOND-BR) What if there is no
> >> > rules to handle the packet in 'second-br' and wanted to report to
> >> controller?
> >> >
> >> > Will the packet report with VLAN 10 to controller?
> >>
> >> Currently, this will work because patch port doesn't NULLify this
> >> ctx->xin->packet pointer, then when it translates through the second
> >> bridge and gets to execute_controller_action(), that function will
> >> make a clone, apply the current actions to that packet, then send the
> >> modified packet to the controller.
> > [Sugesh] Ok. So all the pre-actions will be applied before cloning the
> > packets(applying mod_vlan) using the callback.
> 
> Yup, that's right.
> 
> >> So, longer term I think that this NULLification should be removed and
> >> we keep the same packet attached to the context. However at the
> >> moment, from execute_controller_action() ->
> >> xlate_execute_odp_actions() -> odp_execute_actions() there is no
> >> datapath callback provided for implementing the tunnel_push
> >> functionality. Maybe it's as simple as plugging
> >> dpif_execute_helper_cb() into the odp_execute_actions() call there,
> >> but
> > [Sugesh] I think we should offer the callback to do the modification.
> > Will work on when this patch series is accepted.
> >> that sounds like a separate change that should get some thorough
> >> review and testing as well. I don't think it's strictly necessary for
> >> getting this series in, this series can function correctly using the
> >> fallback path as we discussed above. It'd be nice to see this at some
> >> point in future though to consolidate the paths through this code.
> > [Sugesh] Will try to do as a separate patch as you suggested.
> 
> OK, thanks!


More information about the dev mailing list