On Thu, Jan 26, 2017 at 03:22:24PM +0000, Zoltán Balogh wrote: > From: Sugesh Chandran <sugesh.chand...@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.chand...@intel.com> > Signed-off-by: Zoltán Balogh <zoltan.bal...@ericsson.com> > Co-authored-by: Zoltán Balogh <zoltan.bal...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev