Hi Ilya, Please see my response inline. I will be sending new patch after incorporating some of your comments.
Regards, Nitin > -----Original Message----- > From: Ilya Maximets [mailto:i.maxim...@samsung.com] > Sent: Wednesday, August 14, 2019 5:45 PM > To: Nitin Katiyar <nitin.kati...@ericsson.com>; ovs-dev@openvswitch.org > Cc: Stokes, Ian <ian.sto...@intel.com>; Simon Horman > <simon.hor...@netronome.com>; Jan Scheurich > <jan.scheur...@ericsson.com>; Ben Pfaff <b...@ovn.org> > Subject: Re: [ovs-dev] [PATCH] Improve MPLSoGRE performance by reducing > EMC hash collisions. > > On 14.08.2019 11:33, Nitin Katiyar wrote: > > > > > >> -----Original Message----- > >> From: Ilya Maximets [mailto:i.maxim...@samsung.com] > >> Sent: Wednesday, August 14, 2019 1:42 PM > >> To: Nitin Katiyar <nitin.kati...@ericsson.com>; > >> ovs-dev@openvswitch.org > >> Cc: Stokes, Ian <ian.sto...@intel.com> > >> Subject: Re: [ovs-dev] [PATCH] Improve MPLSoGRE performance by > >> reducing EMC hash collisions. > >> > >> On 13.08.2019 20:49, Nitin Katiyar wrote: > >>> When a packet is received, the RSS hash is calculated if it is not > >>> already available. The Exact Match Cache (EMC) entry is then looked > >>> up using this RSS hash. > >>> > >>> When a MPLSoGRE encapsulated packet is received, the GRE header is > >>> popped, the RSS hash is invalidated and the packet is recirculated > >>> for further processing. When the recirculated packet is processed, > >>> the MPLS header is popped and the packet is recirculated again. > >>> Since the RSS hash has not been invalidated here, the EMC lookup > >>> will hit the same entry as that after the first recirculation. This > >>> degrades performance severely. > >> > > Hi Ilya, > >> Hmm. Function 'dpif_netdev_packet_get_rss_hash()' accounts > >> recirculation depth into the hash to avoid such cases. Why this doesn't > work for you? > > This will not very for multiple inner flows. If there are multiple flows > > are sent > across same tunnel (with same MPLS label) then they will all lead to same hash > as external tuple and recirculation id remains same for them. Let me know if I > am wrong. > > OK. I see. Collision is not with the previous pass of this packet but with > other > packets from the same MPLS tunnel. This needs to be made clear in the > commit message. > > And, IIUC, this issue is not related to MPLSoGRE specifically, it's just an > issue > with any MPLS. I think, you need to re-write the commit message to be more > general and, probably, rename the patch to something like: > "packets: Invalidate offloads after MPLS decapsulation." > or > "packets: Fix using outdated RSS hash after MPLS decapsulation." > etc. > Yes, this is for MPLS. Will update the patch title. > Another thing: > It looks like we need to do the same for NSH decap. What do you think? I will check this and take it separately. > > Note: > This change will force hash re-calculation in case of output to balanced > bonding. Yes, this should be okay as packets are recirculated after pop action and new hash should be calculated. Anyways hash is re-computated with new recirc-id. This will give better distribution over bond. > > Also, It's better to not mention EMC in a common 'lib/packets.c' file. > I think that it's enough to just say that we need to invalidate offloads since > they are not valid anymore after decapsulation. Okay. > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev