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-...@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
>
_______________________________________________
freebsd-stable@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "freebsd-stable-unsubscr...@freebsd.org"
