| Message ID | 1565981673-3563-1-git-send-email-nitin.katiyar@ericsson.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] packets: Fix using outdated RSS hash after MPLS decapsulation. | expand |
Hi, Gentle reminder. Please provide your feedback. Regards, Nitin > -----Original Message----- > From: Nitin Katiyar > Sent: Saturday, August 17, 2019 12:25 AM > To: ovs-dev@openvswitch.org > Cc: Nitin Katiyar <nitin.katiyar@ericsson.com> > Subject: [PATCH] packets: Fix using outdated RSS hash after MPLS > decapsulation. > > 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. > > This patch invalidates RSS hash (by resetting offload flags) after MPLS header > is popped. > > Signed-off-by: Nitin Katiyar <nitin.katiyar@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); > + > set_ethertype(packet, ethtype); > if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) { > dp_packet_set_l2_5(packet, NULL); > -- > 1.9.1
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.
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.
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.
>
> This patch invalidates RSS hash (by resetting offload flags) after MPLS
> header is popped.
>
> Signed-off-by: Nitin Katiyar <nitin.katiyar@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?
> +
> set_ethertype(packet, ethtype);
> if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) {
> dp_packet_set_l2_5(packet, NULL);
>
> -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Tuesday, August 27, 2019 5:37 PM > To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org > Cc: Stokes, Ian <ian.stokes@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. Thanks, Nitin > > > > > This patch invalidates RSS hash (by resetting offload flags) after > > MPLS header is popped. > > > > Signed-off-by: Nitin Katiyar <nitin.katiyar@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); > >
On 28.08.2019 10:03, Nitin Katiyar wrote: > > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@samsung.com] >> Sent: Tuesday, August 27, 2019 5:37 PM >> To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@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. 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@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); >>>
> -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@samsung.com] > Sent: Wednesday, August 28, 2019 12:44 PM > To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org > Cc: Stokes, Ian <ian.stokes@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@samsung.com] > >> Sent: Tuesday, August 27, 2019 5:37 PM > >> To: Nitin Katiyar <nitin.katiyar@ericsson.com>; > >> ovs-dev@openvswitch.org > >> Cc: Stokes, Ian <ian.stokes@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. 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@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); > >>>
On 28.08.2019 11:27, Nitin Katiyar wrote: > > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@samsung.com] >> Sent: Wednesday, August 28, 2019 12:44 PM >> To: Nitin Katiyar <nitin.katiyar@ericsson.com>; ovs-dev@openvswitch.org >> Cc: Stokes, Ian <ian.stokes@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@samsung.com] >>>> Sent: Tuesday, August 27, 2019 5:37 PM >>>> To: Nitin Katiyar <nitin.katiyar@ericsson.com>; >>>> ovs-dev@openvswitch.org >>>> Cc: Stokes, Ian <ian.stokes@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@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); >>>>>
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); + 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 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. This patch invalidates RSS hash (by resetting offload flags) after MPLS header is popped. Signed-off-by: Nitin Katiyar <nitin.katiyar@ericsson.com> --- lib/packets.c | 4 ++++ 1 file changed, 4 insertions(+)