On Mon, Dec 3, 2018 at 10:14 PM Cong Wang <xiyou.wangc...@gmail.com> wrote:
>
> When an ethernet frame is padded to meet the minimum ethernet frame
> size, the padding octets are not covered by the hardware checksum.
> Fortunately the padding octets are ususally zero's, which don't affect
> checksum. However, we have a switch which pads non-zero octets, this
> causes kernel hardware checksum fault repeatedly.
>
> Prior to commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE
> are friends"),
> skb checksum was forced to be CHECKSUM_NONE when padding is detected.
> After it, we need to keep skb->csum updated, like what we do for RXFCS.
> However, fixing up CHECKSUM_COMPLETE requires to verify and parse IP
> headers, it is not worthy the effort as the packets are so small that
> CHECKSUM_COMPLETE can't save anything.
>
> I tested this patch with RXFCS on and off, it works fine without any
> warning in both cases.
>
> Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are
> friends"),
> Cc: Saeed Mahameed <sae...@mellanox.com>
> Cc: Eric Dumazet <eduma...@google.com>
> Cc: Tariq Toukan <tar...@mellanox.com>
> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_rx.c | 22 ++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 624eed345b5d..1c153b8091da 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -732,6 +732,13 @@ static u8 get_ip_proto(struct sk_buff *skb, int
> network_depth, __be16 proto)
> ((struct ipv6hdr *)ip_p)->nexthdr;
> }
>
> +static bool is_short_frame(struct sk_buff *skb, bool has_fcs)
> +{
> + u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb->len;
> +
> + return frame_len <= ETH_ZLEN;
> +}
> +
Do we really need to handle FCS here ?
maybe increase the small packet size to always assume FCS is there:
return skb->len <= ETH_ZLEN + ETH_FCS_LEN;
to avoid conditional statements at Data path.
> static inline void mlx5e_handle_csum(struct net_device *netdev,
> struct mlx5_cqe64 *cqe,
> struct mlx5e_rq *rq,
> @@ -755,9 +762,22 @@ static inline void mlx5e_handle_csum(struct net_device
> *netdev,
> goto csum_unnecessary;
>
> if (likely(is_last_ethertype_ip(skb, &network_depth, &proto))) {
> + bool has_fcs = !!(netdev->features & NETIF_F_RXFCS);
> +
> if (unlikely(get_ip_proto(skb, network_depth, proto) ==
> IPPROTO_SCTP))
> goto csum_unnecessary;
>
> + /* CQE csum doesn't cover padding octets in short ethernet
> + * frames. And the pad field is appended prior to calculating
> + * and appending the FCS field.
> + *
> + * Detecting these padded frames requires to verify and parse
> + * IP headers, so we simply force all those small frames to be
> + * CHECKSUM_NONE even if they are not padded.
> + */
> + if (unlikely(is_short_frame(skb, has_fcs)))
> + goto csum_none;
> +
As Eric mentioned, goto csum_unnecessary; here, the code will handle the rest.
for l3/l4 packets the HW already verifies the checksum we must leverage that.
> skb->ip_summed = CHECKSUM_COMPLETE;
> skb->csum = csum_unfold((__force __sum16)cqe->check_sum);
> if (network_depth > ETH_HLEN)
> @@ -768,7 +788,7 @@ static inline void mlx5e_handle_csum(struct net_device
> *netdev,
> skb->csum = csum_partial(skb->data + ETH_HLEN,
> network_depth - ETH_HLEN,
> skb->csum);
> - if (unlikely(netdev->features & NETIF_F_RXFCS))
> + if (unlikely(has_fcs))
> skb->csum = csum_block_add(skb->csum,
> (__force
> __wsum)mlx5e_get_fcs(skb),
> skb->len - ETH_FCS_LEN);
> --
> 2.19.1
>
