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

Chandran, Sugesh sugesh.chandran at intel.com
Wed Jul 19 08:26:38 UTC 2017



Regards
_Sugesh


> -----Original Message-----
> From: Joe Stringer [mailto:joe at ovn.org]
> Sent: Wednesday, July 19, 2017 2:34 AM
> To: Chandran, Sugesh <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 v3 3/3] tunneling: Avoid datapath-recirc by combining
> recirc actions at xlate.
> 
> On 18 July 2017 at 17:40, Joe Stringer <joe at ovn.org> wrote:
> > On 18 July 2017 at 10:49, Sugesh Chandran <sugesh.chandran at intel.com>
> wrote:
> >> +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) {
> > ...
> >> +    if (ctx->odp_actions->size > push_action_size) {
> >> +        /* Update the CLONE action only when combined. */
> >> +        nl_msg_end_nested(ctx->odp_actions, clone_ofs);
> >> +    } else {
> >> +        /* No actions after the tunnel, no need of clone. */
> >> +        nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> >> +        odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
> >
> > I looked at this line again for this version since I didn't understand
> > it last time around, and I'm still a bit confused. If there's no
> > actions to run on the second bridge, then the copy of the packet which
> > is tunneled is effectively dropped. If that copy of the packet is
> > dropped, then why do we need the tunnel action at all? If I follow
> > correctly, then this means that if you have two bridges, where the
> > first bridge has output(tunnel),output(other_device) then the second
> > bridge where the tunneling occurs has no flows, then the datapath flow
> > will end up as something like:
> >
> > push_tnl(...),output(other_device).
> >
> > I realise that the testsuite breaks if you remove this line, but maybe
> > the testsuite needs fixing for these cases?
> 
> On second thought, this should probably be a logically separate follow-up
> patch even if we agree to change it.
[Sugesh] That make sense. Perhaps we can add comment like
/*
  * XXX : Only needed for the unit test cases. Few tests in 'make check'
  * doesn’t have any actions to handle encaped packets.
  */

Is that OK?






More information about the dev mailing list