Date: Sat, 22 Aug 2015 07:59:16 -0400 (EDT) From: Rick Macklem <rmacklem@uoguelph.ca> To: Daniel Braniss <danny@cs.huji.ac.il> Cc: pyunyh@gmail.com, Hans Petter Selasky <hps@selasky.org>, FreeBSD stable <freebsd-stable@freebsd.org>, FreeBSD Net <freebsd-net@freebsd.org>, Christopher Forgeron <csforgeron@gmail.com>, Gleb Smirnoff <glebius@FreeBSD.org>, Slawa Olhovchenkov <slw@zxy.spb.ru> Subject: Re: ix(intel) vs mlxen(mellanox) 10Gb performance Message-ID: <818666007.28930310.1440244756872.JavaMail.zimbra@uoguelph.ca> In-Reply-To: <15D19823-08F7-4E55-BBD0-CE230F67D26E@cs.huji.ac.il> References: <1D52028A-B39F-4F9B-BD38-CB1D73BF5D56@cs.huji.ac.il> <55D429A4.3010407@selasky.org> <20150819074212.GB964@michelle.fasterthan.com> <55D43615.1030401@selasky.org> <2013503980.25726607.1439989235806.JavaMail.zimbra@uoguelph.ca> <20150820023024.GB996@michelle.fasterthan.com> <1153838447.28656490.1440193567940.JavaMail.zimbra@uoguelph.ca> <15D19823-08F7-4E55-BBD0-CE230F67D26E@cs.huji.ac.il>
next in thread | previous in thread | raw e-mail | index | archive | help
------=_Part_28930308_990593625.1440244756870 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Daniel Braniss wrote: >=20 > > On Aug 22, 2015, at 12:46 AM, Rick Macklem <rmacklem@uoguelph.ca> wrote= : > >=20 > > Yonghyeon PYUN wrote: > >> On Wed, Aug 19, 2015 at 09:00:35AM -0400, Rick Macklem wrote: > >>> Hans Petter Selasky wrote: > >>>> On 08/19/15 09:42, Yonghyeon PYUN wrote: > >>>>> On Wed, Aug 19, 2015 at 09:00:52AM +0200, Hans Petter Selasky wrote= : > >>>>>> On 08/18/15 23:54, Rick Macklem wrote: > >>>>>>> Ouch! Yes, I now see that the code that counts the # of mbufs is > >>>>>>> before > >>>>>>> the > >>>>>>> code that adds the tcp/ip header mbuf. > >>>>>>>=20 > >>>>>>> In my opinion, this should be fixed by setting if_hw_tsomaxsegcou= nt > >>>>>>> to > >>>>>>> whatever > >>>>>>> the driver provides - 1. It is not the driver's responsibility to > >>>>>>> know if > >>>>>>> a tcp/ip > >>>>>>> header mbuf will be added and is a lot less confusing that expect= ing > >>>>>>> the > >>>>>>> driver > >>>>>>> author to know to subtract one. (I had mistakenly thought that > >>>>>>> tcp_output() had > >>>>>>> added the tc/ip header mbuf before the loop that counts mbufs in = the > >>>>>>> list. > >>>>>>> Btw, > >>>>>>> this tcp/ip header mbuf also has leading space for the MAC layer > >>>>>>> header.) > >>>>>>>=20 > >>>>>>=20 > >>>>>> Hi Rick, > >>>>>>=20 > >>>>>> Your question is good. With the Mellanox hardware we have separate > >>>>>> so-called inline data space for the TCP/IP headers, so if the TCP > >>>>>> stack > >>>>>> subtracts something, then we would need to add something to the li= mit, > >>>>>> because then the scatter gather list is only used for the data par= t. > >>>>>>=20 > >>>>>=20 > >>>>> I think all drivers in tree don't subtract 1 for > >>>>> if_hw_tsomaxsegcount. Probably touching Mellanox driver would be > >>>>> simpler than fixing all other drivers in tree. > >>>>>=20 > >>>>>> Maybe it can be controlled by some kind of flag, if all the three = TSO > >>>>>> limits should include the TCP/IP/ethernet headers too. I'm pretty = sure > >>>>>> we want both versions. > >>>>>>=20 > >>>>>=20 > >>>>> Hmm, I'm afraid it's already complex. Drivers have to tell almost > >>>>> the same information to both bus_dma(9) and network stack. > >>>>=20 > >>>> Don't forget that not all drivers in the tree set the TSO limits bef= ore > >>>> if_attach(), so possibly the subtraction of one TSO fragment needs t= o go > >>>> into ip_output() .... > >>>>=20 > >>> Ok, I realized that some drivers may not know the answers before > >>> ether_ifattach(), > >>> due to the way they are configured/written (I saw the use of > >>> if_hw_tsomax_update() > >>> in the patch). > >>=20 > >> I was not able to find an interface that configures TSO parameters > >> after if_t conversion. I'm under the impression > >> if_hw_tsomax_update() is not designed to use this way. Probably we > >> need a better one?(CCed to Gleb). > >>=20 > >>>=20 > >>> If it is subtracted as a part of the assignment to if_hw_tsomaxsegcou= nt > >>> in > >>> tcp_output() > >>> at line#791 in tcp_output() like the following, I don't think it shou= ld > >>> matter if the > >>> values are set before ether_ifattach()? > >>> =09=09=09/* > >>> =09=09=09 * Subtract 1 for the tcp/ip header mbuf that > >>> =09=09=09 * will be prepended to the mbuf chain in this > >>> =09=09=09 * function in the code below this block. > >>> =09=09=09 */ > >>> =09=09=09if_hw_tsomaxsegcount =3D tp->t_tsomaxsegcount - 1; > >>>=20 > >>> I don't have a good solution for the case where a driver doesn't plan= on > >>> using the > >>> tcp/ip header provided by tcp_output() except to say the driver can a= dd > >>> one > >>> to the > >>> setting to compensate for that (and if they fail to do so, it still > >>> works, > >>> although > >>> somewhat suboptimally). When I now read the comment in sys/net/if_var= .h > >>> it > >>> is clear > >>> what it means, but for some reason I didn't read it that way before? = (I > >>> think it was > >>> the part that said the driver didn't have to subtract for the headers > >>> that > >>> confused me?) > >>> In any case, we need to try and come up with a clear definition of wh= at > >>> they need to > >>> be set to. > >>>=20 > >>> I can now think of two ways to deal with this: > >>> 1 - Leave tcp_output() as is, but provide a macro for the device driv= er > >>> authors to use > >>> that sets if_hw_tsomaxsegcount with a flag for "driver uses tcp/ip > >>> header mbuf", > >>> documenting that this flag should normally be true. > >>> OR > >>> 2 - Change tcp_output() as above, noting that this is a workaround fo= r > >>> confusion w.r.t. > >>> whether or not if_hw_tsomaxsegcount should include the tcp/ip head= er > >>> mbuf and > >>> update the comment in if_var.h to reflect this. Then drivers that > >>> don't > >>> use the > >>> tcp/ip header mbuf can increase their value for if_hw_tsomaxsegcou= nt > >>> by > >>> 1. > >>> (The comment should also mention that a value of 35 or greater is = much > >>> preferred to > >>> 32 if the hardware will support that.) > >>>=20 > >>=20 > >> Both works for me. My preference is 2 just because it's very > >> common for most drivers that use tcp/ip header mbuf. > > Thanks for this comment. I tend to agree, both for the reason you state= and > > also > > because the patch is simple enough that it might qualify as an errata f= or > > 10.2. > >=20 > > I am hoping Daniel Braniss will be able to test the patch and let us kn= ow > > if it > > improves performance with TSO enabled? >=20 > send me the patch and I=E2=80=99ll test it ASAP. > =09danny >=20 Patch is attached. The one for head will also include an update to the comm= ent in sys/net/if_var.h, but that isn't needed for testing. Thanks for testing this, rick > >=20 > > rick > >=20 > >> _______________________________________________ > >> freebsd-stable@freebsd.org mailing list > >> https://lists.freebsd.org/mailman/listinfo/freebsd-stable > >> To unsubscribe, send any mail to "freebsd-stable-unsubscribe@freebsd.o= rg" > >>=20 >=20 > _______________________________________________ > freebsd-stable@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-stable > To unsubscribe, send any mail to "freebsd-stable-unsubscribe@freebsd.org" ------=_Part_28930308_990593625.1440244756870 Content-Type: text/x-patch; name=tsooutby1.patch Content-Disposition: attachment; filename=tsooutby1.patch Content-Transfer-Encoding: base64 LS0tIG5ldGluZXQvdGNwX291dHB1dC5jLnNhdgkyMDE1LTA4LTIyIDA3OjQ4OjEyLjAwMDAwMDAw MCAtMDQwMAorKysgbmV0aW5ldC90Y3Bfb3V0cHV0LmMJMjAxNS0wOC0yMiAwNzo1MDo1Mi4wMDAw MDAwMDAgLTA0MDAKQEAgLTc5NCw3ICs3OTQsMTMgQEAgc2VuZDoKIAogCQkJLyogZXh0cmFjdCBU U08gaW5mb3JtYXRpb24gKi8KIAkJCWlmX2h3X3Rzb21heCA9IHRwLT50X3Rzb21heDsKLQkJCWlm X2h3X3Rzb21heHNlZ2NvdW50ID0gdHAtPnRfdHNvbWF4c2VnY291bnQ7CisJCQkvKgorCQkJICog U3VidHJhY3QgMSBmb3IgdGhlIHRjcC9pcCBoZWFkZXIgbWJ1ZiB0aGF0CisJCQkgKiB3aWxsIGJl IHByZXBlbmRlZCB0byB0aGlzIG1idWYgY2hhaW4gYWZ0ZXIKKwkJCSAqIHRoZSBjb2RlIGluIHRo aXMgc2VjdGlvbiBsaW1pdHMgdGhlIG51bWJlciBvZgorCQkJICogbWJ1ZnMgaW4gdGhlIGNoYWlu IHRvIGlmX2h3X3Rzb21heHNlZ2NvdW50LgorCQkJICovCisJCQlpZl9od190c29tYXhzZWdjb3Vu dCA9IHRwLT50X3Rzb21heHNlZ2NvdW50IC0gMTsKIAkJCWlmX2h3X3Rzb21heHNlZ3NpemUgPSB0 cC0+dF90c29tYXhzZWdzaXplOwogCiAJCQkvKgo= ------=_Part_28930308_990593625.1440244756870--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?818666007.28930310.1440244756872.JavaMail.zimbra>