[ovs-dev] [PATCH] Improve MPLSoGRE performance by reducing EMC hash collisions.

[Nitin Katiyar]

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 at samsung.com]
> Sent: Wednesday, August 14, 2019 5:45 PM
> To: Nitin Katiyar <nitin.katiyar at ericsson.com>; ovs-dev at openvswitch.org
> Cc: Stokes, Ian <ian.stokes at intel.com>; Simon Horman
> <simon.horman at netronome.com>; Jan Scheurich
> <jan.scheurich at ericsson.com>; Ben Pfaff <blp at 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 at samsung.com]
> >> Sent: Wednesday, August 14, 2019 1:42 PM
> >> To: Nitin Katiyar <nitin.katiyar at ericsson.com>;
> >> ovs-dev at openvswitch.org
> >> Cc: Stokes, Ian <ian.stokes at 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.