Date: Sat, 22 Aug 2015 10:28:03 +0300 From: Daniel Braniss <danny@cs.huji.ac.il> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: pyunyh@gmail.com, Hans Petter Selasky <hps@selasky.org>, FreeBSD stable <freebsd-stable@freebsd.org>, FreeBSD Net <freebsd-net@freebsd.org>, Slawa Olhovchenkov <slw@zxy.spb.ru>, Gleb Smirnoff <glebius@FreeBSD.org>, Christopher Forgeron <csforgeron@gmail.com> Subject: Re: ix(intel) vs mlxen(mellanox) 10Gb performance Message-ID: <15D19823-08F7-4E55-BBD0-CE230F67D26E@cs.huji.ac.il> In-Reply-To: <1153838447.28656490.1440193567940.JavaMail.zimbra@uoguelph.ca> References: <1D52028A-B39F-4F9B-BD38-CB1D73BF5D56@cs.huji.ac.il> <55D333D6.5040102@selasky.org> <1325951625.25292515.1439934848268.JavaMail.zimbra@uoguelph.ca> <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>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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_tsomaxsegcount >>>>>>> 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 = expecting >>>>>>> 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 = limit, >>>>>> because then the scatter gather list is only used for the data = part. >>>>>>=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 = before >>>> if_attach(), so possibly the subtraction of one TSO fragment needs = to 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_tsomaxsegcount in >>> tcp_output() >>> at line#791 in tcp_output() like the following, I don't think it = should >>> matter if the >>> values are set before ether_ifattach()? >>> /* >>> * Subtract 1 for the tcp/ip header mbuf that >>> * will be prepended to the mbuf chain in this >>> * function in the code below this block. >>> */ >>> if_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 = add 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 = what >>> 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 = driver >>> 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 = for >>> confusion w.r.t. >>> whether or not if_hw_tsomaxsegcount should include the tcp/ip = header >>> 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_tsomaxsegcount 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 = for 10.2. >=20 > I am hoping Daniel Braniss will be able to test the patch and let us = know if it > improves performance with TSO enabled? send me the patch and I=E2=80=99ll test it ASAP. danny >=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.org" >>=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?15D19823-08F7-4E55-BBD0-CE230F67D26E>