Let me support Sugesh's points. The datapath performance penalty we pay today for VXLAN or GRE encapsulation due to recirculation is significant and this patch has proven to reduce that penalty almost by half by avoiding recirculation in Tx direction, giving a 20-30% performance boost in our L3-VPN benchmarks.
We have been working on this performance optimization patch upstream ever since the last OVS fall conference where we first presented the prototype results. So this is not a new thing. In fact it was already merged before (commit f1dac5128c). Admittedly, that commit was not complete because it was only tested in conjunction with a NORMAL/non-OF provider bridge, which is our main target scenario. But it was easy to adjust the flow to the encapsulated header. The patch had to be reverted temporarily due to more involved interaction issues with truncate action and statistics. As all this is fixed now, our wish would be that this be included in OSV 2.8. Yes, we have learned that the code dealing with continuation of translation across multiple bridges is complex with many possible interactions between various features. But that equally applies to patch ports, and we do not consider banning these, do we? Let's work together to simplify and strengthen this functionality in OVS for all users. I believe the generic Encap action (EXT-382) that we are upstreaming for 2.8 just now is likely to give new use cases that would profit from this. Thanks, Jan > -----Original Message----- > From: Chandran, Sugesh [mailto:[email protected]] > Sent: Friday, 14 July, 2017 11:28 > To: Joe Stringer <[email protected]> > Cc: [email protected]; Darrell Ball <[email protected]>; Zoltán Balogh > <[email protected]>; Jan Scheurich > <[email protected]> > Subject: RE: [PATCH v2 0/4]tunneling : Improving tunneling performance by > avoiding dp recirc. > > Hi Joe > Thank you for getting back on this. > Please find my comments inline below. > > Regards > _Sugesh > > > -----Original Message----- > > From: Joe Stringer [mailto:[email protected]] > > Sent: Tuesday, July 11, 2017 11:49 PM > > To: Chandran, Sugesh <[email protected]> > > Cc: [email protected]; Darrell Ball <[email protected]>; Zoltán Balogh > > <[email protected]>; Jan Scheurich > > <[email protected]> > > Subject: Re: [PATCH v2 0/4]tunneling : Improving tunneling performance by > > avoiding dp recirc. > > > > Hi Sugesh, > > > > I've been travelling so haven't had time to look closely at the patches > > yet. Is > > there currently a regression in performance from an earlier release? If so, > > then /perhaps/ this could be considered a bug fix. However, if it's just > > improving performance then I don't think it's fair to class this as a > > bugfix. > [Sugesh] What I meant by bug fix is, the proposal is being accepted before > and reverted due to some of the issues. Now we reworked to handle > all the reported issues. So it is more of a fix for a accepted approach. > > > Furthermore, if it's a bugfix then the v2.8 release timeline shouldn't > > really > > influence the decision because bugfixes should be backported to affected > > branches, it should just be a matter of backporting and waiting for a > > subsequent release. > > > > Since I haven't had a chance to look at v2, let me just highlight what I'm > > aware of with this proposal at this point: > > * The existing code is already complex, and this adds complexity > > * This area (including patch ports) often contains subtle bugs because the > > testsuite tends to put very simple flows on the second bridge, so we don't > > notice that execution on the second bridge affects the original bridge. > > This is > > a bit concerning, any mitigation of this concern would be well-received. > [Sugesh] What do you suggest here to handle this?. We had tested different > test scenarios > based on our discussion before. The performance improvement offered > by this patch is quite significant and definitely useful. > Should we hold back the patch due to the fact that the code is complex? and > our testsuites > are not sufficient enough to do all the tests? > Do you have any suggestions to simplify the complexity? Is there anything we > can do there? > > * Andy's previously-proposed series may have some impact on this series, I'd > > be interested to hear about the potential interactions. > [Sugesh] Yes, I feel the clone handling might be effected by Andy's proposal. > > > > > I think I've said before, if we can get considerable enough benefits then > > the > [Sugesh] As I mentioned above, we have very good performance improvement for > tunneling, which is going to useful for the customers anyway. > > series seems reasonable to me despite the complexity. We just have to be > > careful about not compromising correctness in the process. > [Sugesh] Can you suggest what needs to be done more to assure the correctness? > Do you have any specific case in mind that could be affected with this > solution? > > If we are holding back the patch due to the concern over impact on > second-bridge flow rules > , we cannot upstream this proposal any time. I assume everyone agree the > point that its very relevant > and quite useful. Please suggest an upstream plan , so that we can work on it. > > > > > Cheers, > > Joe > > > > > > On 7 July 2017 at 02:49, Chandran, Sugesh <[email protected]> > > wrote: > > > Hi Joe, > > > We would like to upstream this patch for v2.8, if possible. The reason > > > being > > pushing this for v2.8 release is that its more of a bug fix than a entirely > > new > > feature. > > > May I know what needs to be done for that? We are ok to set up a call to > > review the patch if needed. > > > Also please let us know, if you think we have to do any more > > testing/validations on this. > > > We had a discussion on this topic in OVS-DPDK sync call and the suggestion > > is to check with you, Andy to proceed. > > > > > > Thank you! > > > > > > > > > Regards > > > _Sugesh > > > > > >> -----Original Message----- > > >> From: Chandran, Sugesh > > >> Sent: Tuesday, July 4, 2017 10:47 AM > > >> To: [email protected]; [email protected]; [email protected] > > >> Cc: Chandran, Sugesh <[email protected]>; Zoltán Balogh > > >> <[email protected]> > > >> Subject: [PATCH v2 0/4]tunneling : Improving tunneling performance by > > >> avoiding dp recirc. > > >> > > >> 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 <[email protected]> > > >> Signed-off-by: Zoltán Balogh <[email protected]> > > >> Co-authored-by: Zoltán Balogh <[email protected]> > > >> > > >> 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 (4): > > >> xlate: Refactor translation of patch port output actions. > > >> 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 | 14 +- > > >> ofproto/ofproto-dpif-xlate- cache.h | 15 +- > > >> ofproto/ofproto-dpif-xlate.c | 568 > > ++++++++++++++++++++++++++++-- > > >> ------- > > >> ofproto/ofproto-dpif-xlate.h | 1 + > > >> ofproto/ofproto-dpif.c | 3 +- > > >> tests/packet-type-aware.at | 24 +- > > >> 8 files changed, 468 insertions(+), 177 deletions(-) > > >> > > >> -- > > >> 2.7.4 > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
