| Message ID | 1565718587-3178-1-git-send-email-nitin.katiyar@ericsson.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] Improve MPLSoGRE performance by reducing EMC hash collisions. | expand |
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. 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? Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Wednesday, August 14, 2019 1:42 PM > To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org > Cc: Stokes, Ian <ian.stokes@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. Regards, Nitin > > Best regards, Ilya Maximets.
On 14.08.2019 11:33, Nitin Katiyar wrote: > > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@samsung.com] >> Sent: Wednesday, August 14, 2019 1:42 PM >> To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@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. Another thing: It looks like we need to do the same for NSH decap. What do you think? Note: This change will force hash re-calculation in case of output to balanced bonding. 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. Best regards, Ilya Maximets.
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.maximets@samsung.com] > Sent: Wednesday, August 14, 2019 5:45 PM > To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org > Cc: Stokes, Ian <ian.stokes@intel.com>; Simon Horman > <simon.horman@netronome.com>; Jan Scheurich > <jan.scheurich@ericsson.com>; Ben Pfaff <blp@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.maximets@samsung.com] > >> Sent: Wednesday, August 14, 2019 1:42 PM > >> To: Nitin Katiyar <nitin.katiyar@ericsson.com>; > >> ovs-dev@openvswitch.org > >> Cc: Stokes, Ian <ian.stokes@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.
diff --git a/lib/packets.c b/lib/packets.c index ab0b1a3..35fa3c7 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -404,6 +404,11 @@ pop_mpls(struct dp_packet *packet, ovs_be16 ethtype) struct mpls_hdr *mh = dp_packet_l2_5(packet); size_t len = packet->l2_5_ofs; + /* Invalidate the hash so that it is recomputed using inner packet + * header after recirculation. Else it would cause EMC collision for + * packets recirculated after popping mpls header. */ + dp_packet_reset_offload(packet); + set_ethertype(packet, ethtype); if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) { dp_packet_set_l2_5(packet, NULL);
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. This patch invalides RSS hash after MPLS header is popped. Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com> --- lib/packets.c | 5 +++++ 1 file changed, 5 insertions(+)