> On 19 Aug 2015, at 16:00, Rick Macklem <rmack...@uoguelph.ca> 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. >>>>> >>>>> 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.) >>>>> >>>> >>>> Hi Rick, >>>> >>>> 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. >>>> >>> >>> 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. >>> >>>> 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. >>>> >>> >>> Hmm, I'm afraid it's already complex. Drivers have to tell almost >>> the same information to both bus_dma(9) and network stack. >> >> 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() .... >> > 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). > > 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 = tp->t_tsomaxsegcount - 1; > > 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. > > 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.) > > Also, I'd like to apologize for some of my emails getting a little "blunt". I > just find > it flustrating that this problem is still showing up and is even in 10.2. > This is partly > my fault for not making it clearer to driver authors what > if_hw_tsomaxsegcount should be > set to, because I had it incorrect. > > Hopefully we can come up with a solution that everyone is comfortable with, > rick
ok guys, when you have some code for me to try just let me know. danny _______________________________________________ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
