| Message ID | CAPWQB7GUHsQheqyRfuPV=_2C5PJFXGBzqKbWuCcOUTXh6iUqxQ@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Thankyou Joe! Regards _Sugesh > -----Original Message----- > From: Joe Stringer [mailto:joe@ovn.org] > Sent: Wednesday, July 19, 2017 10:38 PM > To: Chandran, Sugesh <sugesh.chandran@intel.com> > Cc: ovs dev <dev@openvswitch.org>; Andy Zhou <azhou@ovn.org>; Zoltán > Balogh <zoltan.balogh@ericsson.com> > Subject: Re: [PATCH v4 0/3] tunneling : Improving tunneling performance by > avoiding dp recirc. > > On 19 July 2017 at 06:46, Sugesh Chandran <sugesh.chandran@intel.com> > wrote: > > 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. > > > > 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> > > > > v4 > > - Rebased on latest master. > > - Coding style fixes > > - Provide comment for tunnel-push without post actions. > > - Corrected new packet-aware test suites to pass userspace testsuites. > > - Changes on cache_steal function to use memcpy and ofpbuf_put_uninit > > functions. > > > > v3 > > - Rebased on latest master > > - Changed the xlate_cache copy function to avoid expensive ref update > operations. > > - Updated the new packet-aware test cases to handle the non-recirc > tunnel case. > > - Updated the commit message with performance results. > > > > v2 > > - Rebased on latest master. > > - Updated newely added packet-aware test case to honor tunnel combine > actions. > > - Folded related patches into single patch based on Joe's comments. > > - Do the translation only once for tunnel combine instead of two. > > > > > > Sugesh Chandran (3): > > xlate: Clear tunnel mask along with other fields while combine > > actions. > > tunneling: Calculate and update packet l4_offset in tunnel push. > > tunneling: Avoid datapath-recirc by combining recirc actions at xlate. > > > > lib/dpif-netdev.c | 18 +-- > > lib/netdev-native-tnl.c | 2 + > > ofproto/ofproto-dpif-xlate-cache.c | 32 +++- > > ofproto/ofproto-dpif-xlate-cache.h | 13 +- > > ofproto/ofproto-dpif-xlate.c | 238 > +++++++++++++++++++++++++++- > > ofproto/ofproto-dpif.c | 3 +- > > tests/packet-type-aware.at | 27 ++-- > > tests/system-userspace-packet-type-aware.at | 24 +-- > > 8 files changed, 296 insertions(+), 61 deletions(-) > > > > -- > > 2.7.4 > > > > Thanks Sugesh and Zoltán, I applied this series to master with the > following minor style incremental to the last patch: > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index 5dbf5833dfb8..ef579409b0d0 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -139,7 +139,7 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet > *packet, struct flow_tnl *tnl, > * > * This function sets the IP header's ip_tot_len field (which should be zeroed > * as part of 'header') and puts its value into '*ip_tot_size' as well. Also > - * updates IP header checksum. > + * updates IP header checksum, as well as the l3 and l4 offsets in 'packet'. > * > * Return pointer to the L4 header added to 'packet'. */ > void * > diff --git a/ofproto/ofproto-dpif-xlate-cache.c > b/ofproto/ofproto-dpif-xlate-cache.c > index 6947f2fd318d..d319d287eadb 100644 > --- a/ofproto/ofproto-dpif-xlate-cache.c > +++ b/ofproto/ofproto-dpif-xlate-cache.c > @@ -300,9 +300,10 @@ xlate_cache_steal_entries(struct xlate_cache > *dst, struct xlate_cache *src) > if (!dst || !src) { > return; > } > - void *p; > struct ofpbuf *src_entries = &src->entries; > struct ofpbuf *dst_entries = &dst->entries; > + void *p; > + > p = ofpbuf_put_uninit(dst_entries, src_entries->size); > memcpy(p, src_entries->data, src_entries->size); > ofpbuf_clear(src_entries); > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 0177f5c15b71..7f7adb280eaf 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3326,13 +3326,13 @@ validate_and_combine_post_tnl_actions(struct > xlate_ctx *ctx, > /* Update the CLONE action only when combined. */ > nl_msg_end_nested(ctx->odp_actions, clone_ofs); > } else { > + nl_msg_cancel_nested(ctx->odp_actions, clone_ofs); > /* XXX : There is no real use-case for a tunnel push without > * any post actions. However keeping it now > * as is to make the 'make check' happy. Should remove when all the > * make check tunnel test case does something meaningful on a > * tunnel encap packets. > */ > - nl_msg_cancel_nested(ctx->odp_actions, clone_ofs); > odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data); > }
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index 5dbf5833dfb8..ef579409b0d0 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -139,7 +139,7 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, * * This function sets the IP header's ip_tot_len field (which should be zeroed * as part of 'header') and puts its value into '*ip_tot_size' as well. Also - * updates IP header checksum. + * updates IP header checksum, as well as the l3 and l4 offsets in 'packet'. * * Return pointer to the L4 header added to 'packet'. */ void * diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c index 6947f2fd318d..d319d287eadb 100644 --- a/ofproto/ofproto-dpif-xlate-cache.c +++ b/ofproto/ofproto-dpif-xlate-cache.c @@ -300,9 +300,10 @@ xlate_cache_steal_entries(struct xlate_cache *dst, struct xlate_cache *src) if (!dst || !src) { return; } - void *p; struct ofpbuf *src_entries = &src->entries; struct ofpbuf *dst_entries = &dst->entries; + void *p; + p = ofpbuf_put_uninit(dst_entries, src_entries->size); memcpy(p, src_entries->data, src_entries->size); ofpbuf_clear(src_entries); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 0177f5c15b71..7f7adb280eaf 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3326,13 +3326,13 @@ validate_and_combine_post_tnl_actions(struct xlate_ctx *ctx, /* Update the CLONE action only when combined. */ nl_msg_end_nested(ctx->odp_actions, clone_ofs); } else { + nl_msg_cancel_nested(ctx->odp_actions, clone_ofs); /* XXX : There is no real use-case for a tunnel push without * any post actions. However keeping it now * as is to make the 'make check' happy. Should remove when all the * make check tunnel test case does something meaningful on a * tunnel encap packets. */ - nl_msg_cancel_nested(ctx->odp_actions, clone_ofs); odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data); }