Date: Mon, 22 Jul 2013 16:10:01 GMT From: John Baldwin <jhb@freebsd.org> To: freebsd-net@FreeBSD.org Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets Message-ID: <201307221610.r6MGA1Rr017360@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/180430; it has been noted by GNATS.
From: John Baldwin <jhb@freebsd.org>
To: Meny Yossefi <menyy@mellanox.com>
Cc: "bug-followup@freebsd.org" <bug-followup@freebsd.org>
Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets
Date: Mon, 22 Jul 2013 11:40:08 -0400
On Monday, July 22, 2013 10:11:51 am Meny Yossefi wrote:
> Hi John,
>
>
>
> The problem is that the HW will only calculate csum for parts of the payload, one fragment at a time,
>
> while the receiver side, in our case the tcp/ip stack, will expect to validate the packet's payload as a whole.
Ok.
> I agree with the change you offered, though one thing bothers me.
>
> This change will add two conditions to the send flow which will probably have an effect on performance.
>
> Maybe 'likely' can be useful here ?
FreeBSD tends to not use likely/unlikely unless there is a demonstrable gain
via benchmark measurements. However, if the OFED code regularly uses it and
you feel this is a case where you would normally use it, it can be added.
> BTW, I'm not entirely sure, but I think the CSUM_IP flag is always set, so maybe the first condition is not necessary.
>
> What do you think ?
If the user uses ifconfig to disable checksum offload and force software
checksums the flag will not be set.
> -Meny
>
>
>
>
>
> -----Original Message-----
> From: John Baldwin [mailto:jhb@freebsd.org]
> Sent: Friday, July 19, 2013 6:29 PM
> To: bug-followup@freebsd.org; Meny Yossefi
> Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets
>
>
>
> Oops, my previous reply didn't make it to the PR itself.
>
>
>
> Is the problem that the hardware checksum overwrites arbitrary data in the packet (at the location where the UDP header would be)?
>
>
>
> Also, I've looked at other drivers, and they all look at the CSUM_* flags to determine the right settings. IP fragments will not have CSUM_UDP or
CSUM_TCP set, so I think the more correct fix is this:
>
>
>
> Index: en_tx.c
>
> ===================================================================
>
> --- en_tx.c (revision 253470)
>
> +++ en_tx.c (working copy)
>
> @@ -780,8 +780,12 @@ retry:
>
> tx_desc->ctrl.srcrb_flags = cpu_to_be32(MLX4_WQE_CTRL_CQ_UPDATE |
>
> MLX4_WQE_CTRL_SOLICITED);
>
> if (mb->m_pkthdr.csum_flags & (CSUM_IP|CSUM_TCP|CSUM_UDP)) {
>
> - tx_desc->ctrl.srcrb_flags |= cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |
>
> - MLX4_WQE_CTRL_TCP_UDP_CSUM);
>
> + if (mb->m_pkthdr.csum_flags & CSUM_IP)
>
> + tx_desc->ctrl.srcrb_flags |=
>
> + cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);
>
> + if (mb->m_pkthdr.csum_flags & (CSUM_TCP|CSUM_UDP))
>
> + tx_desc->ctrl.srcrb_flags |=
>
> + cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM);
>
> priv->port_stats.tx_chksum_offload++;
>
> }
>
>
>
> --
>
> John Baldwin
>
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201307221610.r6MGA1Rr017360>