Daniel Braniss wrote:>
> > On Aug 22, 2015, at 12:46 AM, Rick Macklem <rmacklem at
uoguelph.ca> wrote:
> >
> > 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?
>
> send me the patch and I?ll test it ASAP.
> danny
>
Patch is attached. The one for head will also include an update to the comment
in sys/net/if_var.h, but that isn't needed for testing.
Thanks for testing this, rick
> >
> > 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"
> >>
>
> _______________________________________________
> 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"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tsooutby1.patch
Type: text/x-patch
Size: 599 bytes
Desc: not available
URL:
<http://lists.freebsd.org/pipermail/freebsd-stable/attachments/20150822/69700572/attachment.bin>