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>, "bug-follo...@freebsd.org" <bug-follo...@freebsd.org> Cc: Subject: RE: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragmented packets Date: Mon, 22 Jul 2013 14:11:51 +0000 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0C0DAMTLDAG01mtlcom_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi John, The problem is that the HW will only calculate csum for parts of the payloa= d, one fragment at a time, while the receiver side, in our case the tcp/ip stack, will expect to valid= ate the packet's payload as a whole. 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 ha= ve an effect on performance. Maybe 'likely' can be useful here ? 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 ? -Meny -----Original Message----- From: John Baldwin [mailto:j...@freebsd.org] Sent: Friday, July 19, 2013 6:29 PM To: bug-follo...@freebsd.org; Meny Yossefi Subject: Re: kern/180430: [ofed] [patch] Bad UDP checksum calc for fragment= ed 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 t= o determine the right settings. IP fragments will not have CSUM_UDP or CSU= M_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_U= PDATE | = 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(M= LX4_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 --_000_F2E47A38E4D0B9499D76F2AB8901571A73A0C0DAMTLDAG01mtlcom_ 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;} /* 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";} span.PlainTextChar {mso-style-name:"Plain Text Char"; mso-style-priority:99; mso-style-link:"Plain Text"; font-family:"Calibri","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">Hi John, <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">The problem is that= the HW will only calculate csum for parts of the payload, one fragment at = a time, <o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">while the receiver = side, in our case the tcp/ip stack, will expect to validate the packet̵= 7;s payload as a whole.<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">I agree with the ch= ange you offered, though one thing bothers me.<o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">This change will ad= d two conditions to the send flow which will probably have an effect on per= formance.<o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">Maybe ‘likely= ’ can be useful here ?<o:p></o:p></span></p> <p class=3D"MsoPlainText"><o:p> </o:p></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">BTW, I’m not = entirely sure, but I think the CSUM_IP flag is always set, so maybe the fir= st condition is not necessary. <o:p></o:p></span></p> <p class=3D"MsoPlainText"><span style=3D"color:#1F497D">What do you think ?= <o:p></o:p></span></p> <p class=3D"MsoPlainText"><o:p> </o:p></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: Friday, July 19, 2013 6:29 PM<br> To: bug-follo...@freebsd.org; Meny Yossefi<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">Oops, my previous reply didn't make it to the PR = itself.<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 overwri= tes arbitrary data in the packet (at the location where the UDP header woul= d be)?<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_* flags to determine the right settings. IP fragment= s will not have CSUM_UDP or CSUM_TCP set, so I think the more correct fix i= s this:<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">=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">--- en_tx.c &n= bsp; (revision 253470)<o:p></o:p></p> <p class=3D"MsoPlainText">+++ en_tx.c &n= bsp; (working copy)<o:p></o:p></p> <p class=3D"MsoPlainText">@@ -780,8 +780,12 @@ retry:<o:p></o:p></p> <p class=3D"MsoPlainText"> &= nbsp; tx_desc->ctrl.srcrb_flags =3D cpu_to= _be32(MLX4_WQE_CTRL_CQ_UPDATE |<o:p></o:p></p> <p class=3D"MsoPlainText"> &= nbsp; &nbs= p; &= nbsp; &nbs= p; &= nbsp; &nbs= p; &= nbsp; MLX4_WQE_= CTRL_SOLICITED);<o:p></o:p></p> <p class=3D"MsoPlainText"> &= nbsp; if (mb->m_pkthdr.csum_flags & (C= SUM_IP|CSUM_TCP|CSUM_UDP)) {<o:p></o:p></p> <p class=3D"MsoPlainText">- = &nb= sp; tx_desc->ctrl.srcrb_= flags |=3D cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |<o:p></o:p></p> <p class=3D"MsoPlainText">- = &nb= sp; = &nb= sp; = &nb= sp; = &nb= sp; = MLX4_WQE_CTRL_TCP_UDP_CSUM);<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">+ &n= bsp;  = ; &n= bsp; tx_d= esc->ctrl.srcrb_flags |=3D<o:p></o:p></p> <p class=3D"MsoPlainText">+ &n= bsp;  = ; &n= bsp; &nbs= p; cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM);<o:p></o:p></p> <p class=3D"MsoPlainText">+ &n= bsp;  = ; if (mb->m_pkthdr.csum_= flags & (CSUM_TCP|CSUM_UDP))<o:p></o:p></p> <p class=3D"MsoPlainText">+ &n= bsp;  = ; &n= bsp; tx_d= esc->ctrl.srcrb_flags |=3D<o:p></o:p></p> <p class=3D"MsoPlainText">+ &n= bsp;  = ; &n= bsp; &nbs= p; cpu_to_be32(MLX4_WQE_CTRL_TCP_UDP_CSUM);<o:p></o:p></p> <p class=3D"MsoPlainText"> &= nbsp; &nbs= p; priv->port_stat= s.tx_chksum_offload++;<o:p></o:p></p> <p class=3D"MsoPlainText"> &= nbsp; }<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_F2E47A38E4D0B9499D76F2AB8901571A73A0C0DAMTLDAG01mtlcom_-- _______________________________________________ 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"