hello Tariq,
On Sun, 2017-06-18 at 14:10 +0300, Tariq Toukan wrote:
> > @@ -624,12 +632,13 @@ static int check_csum(struct mlx4_cqe *cqe, struct
> > sk_buff *skb, void *va,
> > hdr += sizeof(struct vlan_hdr);
> > }
> >
> > - if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
> > - get_fixed_ipv4_csum(hw_checksum, skb, hdr);
> > + if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4)) &&
> > + (unlikely(get_fixed_ipv4_csum(hw_checksum, skb, hdr))))
>
> No! The lazy evaluation trick is wrong here.
> This way you'll end up going almost always to the else (ipv6) for the
> wrong reason.
you are right! thanks for spotting this.
> > + return -1;
> > #if IS_ENABLED(CONFIG_IPV6)
> > - else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
> > - if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
> > - return -1;
> > + else if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6)) &&
> > + (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr))))
> > + return -1;
>
> Let's not change this, might cause future bugs, similarly to the one above.
> > #endif
> > return 0;
> > }
maybe we can avoid adding braces, remove that 'else' keyword and the nested
'if',
thus saving one line, given that check_csum() returns the same set of values as
get_fixed_ipv{4,6}_checksum(), with the same meaning (-1 => go with
CHECKSUM_NONE,
0 => go with CHECKSUM_COMPLETE).
---- >8 ----
@@ -625,11 +633,10 @@ static int check_csum(struct mlx4_cqe *cqe, struct
sk_buff *skb, void *va,
}
if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
- get_fixed_ipv4_csum(hw_checksum, skb, hdr);
+ return get_fixed_ipv4_csum(hw_checksum, skb, hdr);
#if IS_ENABLED(CONFIG_IPV6)
- else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
- if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
- return -1;
+ if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
+ return get_fixed_ipv6_csum(hw_checksum, skb, hdr);
#endif
return 0;
}
---- 8< ----
I will test and repost a v2 with this modification, unless you have any
objections. Thank you in advance!
regards
--
davide
