On 28.08.2019 11:27, Nitin Katiyar wrote: > > >> -----Original Message----- >> From: Ilya Maximets [mailto:[email protected]] >> Sent: Wednesday, August 28, 2019 12:44 PM >> To: Nitin Katiyar <[email protected]>; [email protected] >> Cc: Stokes, Ian <[email protected]> >> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after >> MPLS decapsulation. >> >> On 28.08.2019 10:03, Nitin Katiyar wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ilya Maximets [mailto:[email protected]] >>>> Sent: Tuesday, August 27, 2019 5:37 PM >>>> To: Nitin Katiyar <[email protected]>; >>>> [email protected] >>>> Cc: Stokes, Ian <[email protected]> >>>> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash >>>> after MPLS decapsulation. >>>> >>>> Hi. >>>> >>>> Since this is not a first version of a patch, the subject prefix >>>> should be [PATCH v2]. For the next version, please, use [PATCH v3]. >>>> This way it's much easier to recognize/track patches in a list or mailbox. >>> Hi, >>> Since the title got of the patch got changed so I didn't make it V2. I >>> wasn't >> sure to use V2 as with new title it becomes new thread. Anyway, I will be >> sending the new patch with V3. >>> >>>> >>>> Hint: It's generally speeds up review process if relevant people are >>>> in CC list while sending the patch. Not everyone subscribed for >>>> all the messages in a list. >>> Thanks for suggestion. I will follow it in future. >>> >>>> >>>> The code looks good in general. >>>> Few comments inline. >>>> >>>> Best regards, Ilya Maximets. >>>> >>>> On 16.08.2019 21:54, 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 MPLS encapsulated packet is received, the MPLS header is >>>>> popped and the packet is recirculated. Since the RSS hash has not >>>>> been invalidated here, the EMC lookup will hit the same entry as >>>>> that before recirculation. This degrades performance severely. >>>> >>>> Above sentences are not correct. It's stated that the collision >>>> happens with the previous pass of the same packet, but it happens >>>> with different packets from the same MPLS tunnel. This should be fixed. >>> It will actually collide with the entry of packet before de-capsulation. I >>> have >> rephrased it as follows: >>> >>> When a MPLS encapsulated packet is received, the MPLS header is >> popped >>> and the packet is recirculated. Since the RSS hash has not been >>> invalidated here, the EMC lookup for decapsulated packets will hit the >>> same entry as that before recirculation (i.e. entry of the packet with >>> MPLS encapsulation). This degrades performance severely as different >>> inner packets from the same MPLS tunnel would hit the same EMC entry. >>> >>> Let me know if it is more clear now. >> >> But dpif_netdev_packet_get_rss_hash() mixes the recirculation depth into >> RSS hash on each packet pass through the datapath. So, after recirculation >> RSS >> hash should be different from the hash of encapsulated packet. and where >> should be no collisions. >> I already wrote about this in my review to v1 and you said: "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." That makes sense, but it's opposite to what you're saying now. >> I'm >> confused. > Ah okay, I misinterpret your previous comment. You are correct. I have > modified it as below: > When a MPLS encapsulated packet is received, the MPLS header is popped > and the packet is recirculated. Since the RSS hash has not been > invalidated here, the EMC lookup for all decapsulated packets will hit > the same entry even though these packets will have different tuple > values. This degrades performance severely as different inner packets > from the same MPLS tunnel would hit the same EMC entry.
This version looks good to me. > > Thanks for correcting this. > > Regards, > Nitin >> >> Best regards, Ilya Maximets. >> >>> >>> Thanks, >>> Nitin >>>> >>>>> >>>>> This patch invalidates RSS hash (by resetting offload flags) after >>>>> MPLS header is popped. >>>>> >>>>> Signed-off-by: Nitin Katiyar <[email protected]> >>>>> --- >>>>> lib/packets.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/lib/packets.c b/lib/packets.c index ab0b1a3..db3f6d0 >>>>> 100644 >>>>> --- a/lib/packets.c >>>>> +++ b/lib/packets.c >>>>> @@ -404,6 +404,10 @@ 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 offload flags as they are not valid after >>>>> + * decapsulation of MPLS header. */ >>>>> + dp_packet_reset_offload(packet); >>>> >>>> Maybe it's better to move this code down to 'dp_packet_resize_l2_5' >>>> at the end of this function to have similar changes closer? >>>> >>> Sure, I will move it to end of function. >>>>> + >>>>> set_ethertype(packet, ethtype); >>>>> if (get_16aligned_be32(&mh->mpls_lse) & >> htonl(MPLS_BOS_MASK)) { >>>>> dp_packet_set_l2_5(packet, NULL); >>>>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
