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" >
> On 19 Aug 2015, at 16:00, Rick Macklem <rmacklem at 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, rickok guys, when you have some code for me to try just let me know. danny
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. > > >>> > > >>> 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).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).> > 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.) >Both works for me. My preference is 2 just because it's very common for most drivers that use tcp/ip header mbuf.