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

[Ilya Maximets]

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 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?

> +
>          set_ethertype(packet, ethtype);
>          if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) {
>              dp_packet_set_l2_5(packet, NULL);
>