From owner-freebsd-net@FreeBSD.ORG Mon Jul 22 16:10:01 2013 Return-Path: Delivered-To: freebsd-net@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 9365D164 for ; Mon, 22 Jul 2013 16:10:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 82CB3265C for ; Mon, 22 Jul 2013 16:10:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.7/8.14.7) with ESMTP id r6MGA1Ms017361 for ; Mon, 22 Jul 2013 16:10:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id r6MGA1Rr017360; Mon, 22 Jul 2013 16:10:01 GMT (envelope-from gnats) Date: Mon, 22 Jul 2013 16:10:01 GMT Message-Id: <201307221610.r6MGA1Rr017360@freefall.freebsd.org> To: freebsd-net@FreeBSD.org Cc: From: John Baldwin Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: John Baldwin List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Jul 2013 16:10:01 -0000 The following reply was made to PR kern/180430; it has been noted by GNATS. From: John Baldwin To: Meny Yossefi Cc: "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