[ovs-dev] [PATCH] packets: Fix using outdated RSS hash after MPLS decapsulation.

Ilya Maximets i.maximets at samsung.com
Wed Aug 28 08:31:10 UTC 2019


On 28.08.2019 11:27, Nitin Katiyar wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>> Sent: Wednesday, August 28, 2019 12:44 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] packets: Fix using outdated RSS hash after
>> MPLS decapsulation.
>>
>> On 28.08.2019 10:03, Nitin Katiyar wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>>>> Sent: Tuesday, August 27, 2019 5:37 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] 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 <nitin.katiyar at ericsson.com>
>>>>> ---
>>>>>  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);
>>>>>


More information about the dev mailing list