From owner-freebsd-net@FreeBSD.ORG Wed Jul 24 13:20: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 5EEF6624 for ; Wed, 24 Jul 2013 13:20: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 3F2712F6E for ; Wed, 24 Jul 2013 13:20: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 r6ODK0HO013593 for ; Wed, 24 Jul 2013 13:20:00 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id r6ODK0mf013592; Wed, 24 Jul 2013 13:20:00 GMT (envelope-from gnats) Date: Wed, 24 Jul 2013 13:20:00 GMT Message-Id: <201307241320.r6ODK0mf013592@freefall.freebsd.org> To: freebsd-net@FreeBSD.org Cc: From: Meny Yossefi 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: Meny Yossefi List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jul 2013 13:20:01 -0000 The following reply was made to PR kern/180430; it has been noted by GNATS. From: Meny Yossefi To: John Baldwin Cc: "bug-followup@freebsd.org" Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets Date: Wed, 24 Jul 2013 13:14:53 +0000 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0EC13MTLDAG01mtlcom_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi John, Looks good. -Meny From: Meny Yossefi Sent: Tuesday, July 23, 2013 5:01 PM To: 'John Baldwin' Cc: 'bug-followup@freebsd.org' Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed packets Let's stick with the FreeBSD standards (without the likely/unlikely) Let me just double check the CSUM flags work as expected. I'll get back to you as soon as I'm done. -Meny -----Original Message----- From: John Baldwin [mailto:jhb@freebsd.org] Sent: Monday, July 22, 2013 7:04 PM To: Meny Yossefi Cc: bug-followup@freebsd.org Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed packets 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 val= idate 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 gai= n 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 adde= d. > BTW, I'm not entirely sure, but I think the CSUM_IP flag is always set, s= o maybe the first condition is not necessary. > > What do you think ? If the user uses ifconfig to disable checksum offload and force software ch= ecksums 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 Yosse= fi > 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 th= e 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 > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > --- en_tx.c (revision 253470) > > +++ en_tx.c (working copy) > > @@ -780,8 +780,12 @@ retry: > > tx_desc->ctrl.srcrb_flags =3D > 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 |=3D 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 |=3D > > + > + cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM); > > + if (mb->m_pkthdr.csum_flags & > + (CSUM_TCP|CSUM_UDP)) > > + > + tx_desc->ctrl.srcrb_flags |=3D > > + > + cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM); > > priv->port_stats.tx_chksum_offload++; > > } > > > > -- > > John Baldwin > -- John Baldwin --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0EC13MTLDAG01mtlcom_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi John,

 

Looks good.

 

-Meny

 

From: Meny Yos= sefi
Sent: Tuesday, July 23, 2013 5:01 PM
To: 'John Baldwin'
Cc: 'bug-followup@freebsd.org'
Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for f= ragmented packets

 

Let's stick with th= e FreeBSD standards (without the likely/unlikely)

 

Let me just double = check the CSUM flags work as expected.

I’ll get back= to you as soon as I’m done.

 

-Meny

 

 

-----Original Message-----
From: John Baldwin [mailto:jhb@freebsd.o= rg]
Sent: Monday, July 22, 2013 7:04 PM
To: Meny Yossefi
Cc: bug-followup@freebsd.org
Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed packets

 

On Monday, July 22, 2013 10:11:51 am Meny Yossefi= wrote:

> Hi John,

>

>

>

> The problem is that the HW will only calcula= te csum for parts of the

> payload, one fragment at a time,<= /p>

>

> 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 s= end flow which will probably have an effect on performance.

>

> Maybe 'likely' can be useful here ?

 

FreeBSD tends to not use likely/unlikely unless t= here is a demonstrable gain via benchmark measurements.  However, if t= he 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.<= o:p>

>

> What do you think ?

 

If the user uses ifconfig to disable checksum off= load 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@free= bsd.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 th= e PR itself.

>

>

>

> Is the problem that the hardware checksum ov= erwrites 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

>

> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D

>

> --- en_tx.c     &nb= sp;     (revision 253470)

>

> +++ en_tx.c   &nb= sp;    (working copy)

>

> @@ -780,8 +780,12 @@ retry:

>

>       &nb= sp;        tx_desc->ctrl.srcrb_flags = =3D

> cpu_to_be32(MLX4_WQE_CTRL_CQ_UPDATE |

>

>       &nb= sp;            =             &nb= sp;            =             &nb= sp;            =             &nb= sp;            

> MLX4_WQE_CTRL_SOLICITED);

>

>       &nb= sp;        if (mb->m_pkthdr.csum_flag= s &

> (CSUM_IP|CSUM_TCP|CSUM_UDP)) {

>

> -       &= nbsp;           &nbs= p;          tx_desc->ctrl.s= rcrb_flags |=3D cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |

>

> -       &= nbsp;           &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;  MLX4_WQE_CTRL_TCP_UDP_CSUM);

>

> +       &n= bsp;            = ;         if (mb->m_pkthdr.= csum_flags & CSUM_IP)

>

> +      &nb= sp;            =             &nb= sp;            =

> + tx_desc->ctrl.srcrb_flags |=3D=

>

> +      &nb= sp;            =             &nb= sp;            =     

> + cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);

>

> +      &nb= sp;             = ;         if (mb->m_pkthdr.= csum_flags &

> + (CSUM_TCP|CSUM_UDP))

>

> +      &nb= sp;            =             &nb= sp;            =

> + tx_desc->ctrl.srcrb_flags |=3D=

>

> +      &nb= sp;            =             &nb= sp;            =     

> + cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM= );

>

>       &nb= sp;            =             priv->= ;port_stats.tx_chksum_offload++;

>

>       &nb= sp;        }

>

>

>

> --

>

> John Baldwin

>

 

--

John Baldwin

--_000_F2E47A38E4D0B9499D76F2AB8901571A73A0EC13MTLDAG01mtlcom_--