Hans Petter Selasky wrote:> On 08/18/15 14:53, Rick Macklem wrote:
> > 2572 ifp->if_hw_tsomax = 65518;
> >> >2573 ifp->if_hw_tsomaxsegcount =
IXGBE_82599_SCATTER;
> >> >2574 ifp->if_hw_tsomaxsegsize = 2048;
>
> Hi,
>
> If IXGBE_82599_SCATTER is the maximum scatter/gather entries the
> hardware can do, remember to subtract one fragment for the TCP/IP-header
> mbuf!
>
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.)
> I think there is an off-by-one here:
>
> ifp->if_hw_tsomax = 65518;
> ifp->if_hw_tsomaxsegcount = IXGBE_82599_SCATTER - 1;
> ifp->if_hw_tsomaxsegsize = 2048;
>
> Refer to:
>
> > *
> > * NOTE: The TSO limits only apply to the data payload part of
> > * a TCP/IP packet. That means there is no need to subtract
> > * space for ethernet-, vlan-, IP- or TCP- headers from the
> > * TSO limits unless the hardware driver in question requires
> > * so.
>
This comment suggests that the driver author doesn't need to do this.
However, unless this is fixed in tcp_output(), the above patch should be
applied to the driver.> In sys/net/if_var.h
>
> Thank you!
>
> --HPS
>
The problem I see is that, after doing the calculation of how many mbufs can
be in the TSO segment, the code in tcp_output() will have calculated a value
for "len" that will always be less that "tp->t_maxopd -
optlen" when the
if_hw_tsosegcount limit has been hit (see where it does a "break;" out
of
the while loop).
--> This does not imply "too many small fragments" for NFS, just
that the
driver's transmit segment limit has been reached, where most of them
are mbuf clusters, but not the first ones.
As such the code:
/*
* In case there are too many small fragments
* don't use TSO:
*/
if (len <= max_len) {
len = max_len;
sendalot = 1;
tso = 0;
}
Will always happen for this case and "tso" gets set to 0. Not what we
want to
happen, imho.
The above code block was what I suggested should be commented out or deleted
for the test.
It appears you should also add the "- 1" in the driver
sys/dev/ixgbe/if_ix.c.
rick
> _______________________________________________
> 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"
>