[ovs-dev] [PATCH v3 1/2] tunneling: Avoid recirculation on datapath by computing the recirculate actions at translate time.

Ben Pfaff blp at ovn.org
Wed Feb 1 00:56:53 UTC 2017


On Thu, Jan 26, 2017 at 03:22:24PM +0000, Zoltán Balogh wrote:
> From: Sugesh Chandran <sugesh.chandran at intel.com>
> 
> Openvswitch datapath recirculates packets for tunneling, i.e.
> the incoming packets are encapsulated at first pass. Further actions are
> applied on encapsulated packets on the second pass after recirculating.
> The proposed patch compute and append the post tunnel actions at the time of
> translation itself instead of recirculating at datapath. These actions are solely
> depends on tunnel attributes so there is no need of datapath recirculation.
> By avoiding the recirculation at datapath, the patch offers upto 30%
> performance improvement for VxLAN tunneling in our testing.
> The action execution logic is also extended with new CLONE action to define
> the packet cloning when the actions are combined. The lenght in the CLONE
> action specifies the size of nested action set.
> 
> Rebased onto commit: 535e3acfa70b1c1a44daf91de3a63b80d673dc33
>                      dpif-netdev: Add clone action
> 
> 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>

Thanks for working on this!  This seems like a really nice idea, and the
implementation is pretty simple

Even with patch 2 applied, I still get a failure in test 767:

    ../../tests/tunnel-push-pop.at:137: tail -1 stdout
    --- -   2017-01-31 16:37:07.328760120 -0800
    +++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/767/stdout        2017-01-31 16:37:07.326337064 -0800
    @@ -1,2 +1,2 @@
    -Datapath actions: set(skb_mark(0x4d2)),tnl_push(tnl_port(6081),header(size=50,type=5,eth(dst=f8:bc:12:44:34:b7,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.93,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=6081,csum=0x0),geneve(vni=0xea)),out_port(100))
    +Datapath actions: set(skb_mark(0x4d2)),clone{tnl_push(tnl_port(6081),header(size=50,type=5,eth(dst=f8:bc:12:44:34:b7,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.93,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=6081,csum=0x0),geneve(vni=0xea)),out_port(100)),1}

I'm not sure why this changes the formatting for the ODP clone action
from clone(...) to clone{...}.  The latter syntax isn't used anywhere
else in ODP.

This includes a whitespace only change to format_odp_tnl_push_header().

It's really not clear to me why this replaces a simple loop in
format_odp_actions() by a new recursive function print_odp_actions().

There's funny indentation in the print_odp_actions() prototype, here's a
fix:

@@ -890,8 +890,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
 
 static void
 print_odp_actions(struct ds *ds, bool is_first_action,
-                                const struct nlattr **action_,
-                                size_t *action_len)
+                  const struct nlattr **action_, size_t *action_len)
 
 {
     const struct nlattr *action = *action_;

Why is group_actions() named group_actions()?  OpenFlow has a concept
named a "group", but this doesn't seem to have anything to do with it.
Instead, it seems to be about outputting to a patch port, or maybe just
a port on a different bridge.

group_actions() should have a comment explaining its purpose.

I think that build_tunnel_send() should probably just use the nl_msg
functions for nested attributes, something like this:

    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);
    group_actions(ctx, xport, out_dev);
    nl_msg_end_nested(ctx->odp_actions, clone_ofs);

If there's a real possibility that group_actions() won't actually add
anything, then probably the whole set of actions should just get
dropped, with "ctx->odp_actions->size = clone_ofs;", since there's no
value in cloning and pushing tunnel data just to drop the packet.

Thanks,

Ben.


More information about the dev mailing list