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.
Yonghyeon,
On Thu, Aug 20, 2015 at 11:30:24AM +0900, Yonghyeon PYUN wrote:
Y> > > >> Maybe it can be controlled by some kind of flag, if all
the three TSO
Y> > > >> limits should include the TCP/IP/ethernet headers too.
I'm pretty sure
Y> > > >> we want both versions.
Y> > > >>
Y> > > >
Y> > > > Hmm, I'm afraid it's already complex. Drivers have
to tell almost
Y> > > > the same information to both bus_dma(9) and network stack.
Y> > >
Y> > > Don't forget that not all drivers in the tree set the TSO
limits before
Y> > > if_attach(), so possibly the subtraction of one TSO fragment
needs to go
Y> > > into ip_output() ....
Y> > >
Y> > Ok, I realized that some drivers may not know the answers before
ether_ifattach(),
Y> > due to the way they are configured/written (I saw the use of
if_hw_tsomax_update()
Y> > in the patch).
Y>
Y> I was not able to find an interface that configures TSO parameters
Y> after if_t conversion. I'm under the impression
Y> if_hw_tsomax_update() is not designed to use this way. Probably we
Y> need a better one?(CCed to Gleb).
Yes. In the projects/ifnet all the TSO stuff is configured differently.
I'd really appreciate if other developers look there and review it,
try it, give some input.
Here is a snippet from net/if.h in projects/ifnet:
/*
* Structure describing TSO properties of an interface. Known both to ifnet
* layer and TCP. Most interfaces point to a static tsomax in ifdriver
* definition. However, vlan(4) and lagg(4) require a dynamic tsomax.
*/
struct iftsomax {
uint32_t tsomax_bytes; /* TSO total burst length limit in bytes */
uint32_t tsomax_segcount; /* TSO maximum segment count */
uint32_t tsomax_segsize; /* TSO maximum segment size in bytes */
};
Now closer to your original question. I haven't yet converted lagg(4), so
haven't yet worked on if_hw_tsomax_update(). I am convinced that it
shouldn't
be needed for a regular driver (save lagg(4).
A proper driver should first study its hardware and only then call if_attach().
Correct me if am wrong, please.
Also, I suppose, that a piece of hardware can't change its TSO maximums at
runtime, so I don't see reason for changing them at runtime (save lagg(4)).
--
Totus tuus, Glebius.
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. > > > >>> > > > >>> 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.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. I am hoping Daniel Braniss will be able to test the patch and let us know if it improves performance with TSO enabled? rick> _______________________________________________ > 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" >