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() .... --HPS
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() .... >I think setting them before a call to ether_ifattach() should be required and any driver that doesn't do that needs to be fixed. Also, I notice that "32 * MCLBYTES - (ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN)" is getting written as "65536 - (ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN)" which obscures the reason it is the default. It probably isn't the correct default for any driver that sets if_hw_tsomaxsegcount, but is close to IP_MAXPACKET, so the breakage is mostly theoretical. rick> --HPS > > _______________________________________________ > freebsd-stable at freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-stable > To unsubscribe, send any mail to "freebsd-stable-unsubscribe at freebsd.org" >
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() .... >I don't really care where it gets subtracted, so long as it is subtracted at least by default, so all the drivers that don't subtract it get fixed. However, I might argue that tcp_output() is the correct place, since tcp_output() is where the tcp/ip header mbuf is prepended to the list. The subtraction is just taking into account the mbuf that tcp_output() will be adding to the head of the list and it should count that in the "while()" loop. rick> --HPS > > _______________________________________________ > freebsd-net at freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org" >
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> --HPS > > _______________________________________________ > freebsd-net at freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org" >