The following reply was made to PR kern/180430; it has been noted by GNATS.
From: Meny Yossefi <me...@mellanox.com> To: John Baldwin <j...@freebsd.org> Cc: "bug-follo...@freebsd.org" <bug-follo...@freebsd.org> Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets Date: Tue, 23 Jul 2013 14:01:23 +0000 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0D893MTLDAG01mtlcom_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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:j...@freebsd.org] Sent: Monday, July 22, 2013 7:04 PM To: Meny Yossefi Cc: bug-follo...@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:j...@freebsd.org] > Sent: Friday, July 19, 2013 6:29 PM > To: bug-follo...@freebsd.org<mailto:bug-follo...@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_F2E47A38E4D0B9499D76F2AB8901571A73A0D893MTLDAG01mtlcom_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable <html xmlns:v=3D"urn:schemas-microsoft-com:vml" xmlns:o=3D"urn:schemas-micr= osoft-com:office:office" xmlns:w=3D"urn:schemas-microsoft-com:office:word" = xmlns:m=3D"http://schemas.microsoft.com/office/2004/12/omml"; xmlns=3D"http:= //www.w3.org/TR/REC-html40"> <head> <meta http-equiv=3D"Content-Type" content=3D"text/html; charset=3Dus-ascii"= > <meta name=3D"Generator" content=3D"Microsoft Word 14 (filtered medium)"> <style><!-- /* Font Definitions */ @font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;} @font-face {font-family:Calibri; panose-1:2 15 5 2 2 2 4 3 2 4;} @font-face {font-family:Tahoma; panose-1:2 11 6 4 3 5 4 4 2 4;} /* Style Definitions */ p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0in; margin-bottom:.0001pt; font-size:11.0pt; font-family:"Calibri","sans-serif";} a:link, span.MsoHyperlink {mso-style-priority:99; color:blue; text-decoration:underline;} a:visited, span.MsoHyperlinkFollowed {mso-style-priority:99; color:purple; text-decoration:underline;} p.MsoPlainText, li.MsoPlainText, div.MsoPlainText {mso-style-priority:99; mso-style-link:"Plain Text Char"; margin:0in; margin-bottom:.0001pt; font-size:11.0pt; font-family:"Calibri","sans-serif";} p.MsoAcetate, li.MsoAcetate, div.MsoAcetate {mso-style-priority:99; mso-style-link:"Balloon Text Char"; margin:0in; margin-bottom:.0001pt; font-size:8.0pt; font-family:"Tahoma","sans-serif";} span.PlainTextChar {mso-style-name:"Plain Text Char"; mso-style-priority:99; mso-style-link:"Plain Text"; font-family:"Calibri","sans-serif";} span.BalloonTextChar {mso-style-name:"Balloon Text Char"; mso-style-priority:99; mso-style-link:"Balloon Text"; font-family:"Tahoma","sans-serif";} .MsoChpDefault {mso-style-type:export-only; font-family:"Calibri","sans-serif";} @page WordSection1 {size:8.5in 11.0in; margin:1.0in 1.0in 1.0in 1.0in;} div.WordSection1 {page:WordSection1;} --></style><!--[if gte mso 9]><xml> <o:shapedefaults v:ext=3D"edit" spidmax=3D"1026" /> </xml><![endif]--><!--[if gte mso 9]><xml> <o:shapelayout v:ext=3D"edit"> <o:idmap v:ext=3D"edit" data=3D"1" /> </o:shapelayout></xml><![endif]--> </head> <body lang=3D"EN-US" link=3D"blue" vlink=3D"purple"> <div class=3D"WordSection1"> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">Let's stick with th= e FreeBSD standards (without the likely/unlikely)<o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D"><o:p> </o:p></= span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">Let me just double = check the CSUM flags work as expected.<o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">I’ll get back= to you as soon as I’m done.<o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D"><o:p> </o:p></= span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">-Meny <o:p></o:p></= span></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">-----Original Message-----<br> From: John Baldwin [mailto:j...@freebsd.org] <br> Sent: Monday, July 22, 2013 7:04 PM<br> To: Meny Yossefi<br> Cc: bug-follo...@freebsd.org<br> Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed packets</p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">On Monday, July 22, 2013 10:11:51 am Meny Yossefi= wrote:<o:p></o:p></p> <p class=3D"MsoPlainText">> Hi John,<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> The problem is that the HW will only calcula= te csum for parts of the <o:p></o:p></p> <p class=3D"MsoPlainText">> payload, one fragment at a time,<o:p></o:p><= /p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> while the receiver side, in our case the tcp= /ip stack, will expect to validate the packet's payload as a whole.<o:p></o= :p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">Ok.<o:p></o:p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">> I agree with the change you offered, though = one thing bothers me.<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> This change will add two conditions to the s= end flow which will probably have an effect on performance.<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> Maybe 'likely' can be useful here ?<o:p></o:= p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">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.<o:p></o:p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">> 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></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> What do you think ?<o:p></o:p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">If the user uses ifconfig to disable checksum off= load and force software checksums the flag will not be set.<o:p></o:p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">> -Meny<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> -----Original Message-----<o:p></o:p></p> <p class=3D"MsoPlainText">> From: John Baldwin [<a href=3D"mailto:jhb@fr= eebsd.org"><span style=3D"color:windowtext;text-decoration:none">mailto:jhb= @freebsd.org</span></a>]<o:p></o:p></p> <p class=3D"MsoPlainText">> Sent: Friday, July 19, 2013 6:29 PM<o:p></o:= p></p> <p class=3D"MsoPlainText">> To: <a href=3D"mailto:bug-followup@freebsd.o= rg"><span style=3D"color:windowtext;text-decoration:none">bug-followup@free= bsd.org</span></a>; Meny Yossefi<o:p></o:p></p> <p class=3D"MsoPlainText">> Subject: Re: kern/180430: [ofed] [patch] Bad= UDP checksum calc for <o:p></o:p></p> <p class=3D"MsoPlainText">> fragmented packets<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> Oops, my previous reply didn't make it to th= e PR itself.<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> Is the problem that the hardware checksum ov= erwrites arbitrary data in the packet (at the location where the UDP header= would be)?<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> Also, I've looked at other drivers, and they= all look at the CSUM_* <o:p></o:p></p> <p class=3D"MsoPlainText">> flags to determine the right settings. = IP fragments will not have <o:p></o:p></p> <p class=3D"MsoPlainText">> CSUM_UDP or<o:p></o:p></p> <p class=3D"MsoPlainText">CSUM_TCP set, so I think the more correct fix is = this:<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> Index: en_tx.c<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> =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<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> --- en_tx.c &nb= sp; (revision 253470)<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> +++ en_tx.c &nb= sp; (working copy)<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> @@ -780,8 +780,12 @@ retry:<o:p></o:p></= p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> &nb= sp; tx_desc->ctrl.srcrb_flags = =3D <o:p></o:p></p> <p class=3D"MsoPlainText">> cpu_to_be32(MLX4_WQE_CTRL_CQ_UPDATE |<o:p></= o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> &nb= sp; = &nb= sp; = &nb= sp; = &nb= sp; <o:p></o:p></p> <p class=3D"MsoPlainText">> MLX4_WQE_CTRL_SOLICITED);<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> &nb= sp; if (mb->m_pkthdr.csum_flag= s & <o:p></o:p></p> <p class=3D"MsoPlainText">> (CSUM_IP|CSUM_TCP|CSUM_UDP)) {<o:p></o:p></p= > <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> - &= nbsp; &nbs= p; tx_desc->ctrl.s= rcrb_flags |=3D cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> - &= nbsp; &nbs= p; &= nbsp; &nbs= p; &= nbsp; &nbs= p; &= nbsp; &nbs= p; &= nbsp; MLX4_WQE_CTRL_TCP_UDP_CSUM);<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> + &n= bsp;  = ; if (mb->m_pkthdr.= csum_flags & CSUM_IP)<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> + &nb= sp; = &nb= sp; = <o:p></o:p></p> <p class=3D"MsoPlainText">> + tx_desc->ctrl.srcrb_flags |=3D<o:p>= </o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> + &nb= sp; = &nb= sp; = <o:p> </o:p></p> <p class=3D"MsoPlainText">> + cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);<o:= p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> + &nb= sp;  = ; if (mb->m_pkthdr.= csum_flags & <o:p></o:p></p> <p class=3D"MsoPlainText">> + (CSUM_TCP|CSUM_UDP))<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> + &nb= sp; = &nb= sp; = <o:p></o:p></p> <p class=3D"MsoPlainText">> + tx_desc->ctrl.srcrb_flags |=3D<o:p>= </o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> + &nb= sp; = &nb= sp; = <o:p> </o:p></p> <p class=3D"MsoPlainText">> + cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM= );<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> &nb= sp; = priv->= ;port_stats.tx_chksum_offload++;<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> &nb= sp; }<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> --<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText">> John Baldwin<o:p></o:p></p> <p class=3D"MsoPlainText">> <o:p></o:p></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText">--<o:p></o:p></p> <p class=3D"MsoPlainText">John Baldwin<o:p></o:p></p> </div> </body> </html> --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0D893MTLDAG01mtlcom_-- _______________________________________________ freebsd-net@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
