[please review] TSO mbuf chain length limiting patch

[Colin Percival-4]

Hi all,

The Xen virtual network interface has an issue (ok, really the issue is with
the linux back-end, but that's what most people are using) where it can't
handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
This currently bites us hard with TSO enabled, since it produces said long
mbuf chains.

I've attached a patch which:
(1) adds a IFCAP_MB_CHAIN_LIMIT "capability" and a if_mbuf_chain_limit field
to struct ifnet,
(2) sets these in netfront when the IFCAP_TSO4 flag is set,
(3) extends tcp_maxmtu to read the if_mbuf_chain_limit value (it doesn't
exactly fit here, but this is where we look up interface properties...),
(4) adds a tx_chain_limit field to struct tcpcb,
(5) makes tcp_mss_update set tx_chain_limit using tcp_maxmtu,
(6) adds a new m_copy_nbufs function which truncates the copied mbuf chain
after a specified number of mbufs, and returns the copied data length,
(7) uses these in tcp_output (rearranging some code so that if the output
length is modified, statistics are updated with the correct value).

I'm hoping to commit this and MFC it before 9.1-RELEASE; I believe I've
avoided breaking any ABIs.  In my testing on EC2 with 10 GbE and TSO turned
on, I get ~250-300 Mbps without this patch and 3-4 Gbps with this patch;
this replaces a patch I was using in my EC2 builds which did (6)&(7) above
with a hard-coded maximum mbuf chain length.

Please tell me all the things I did wrong. :-)

--
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Andrew Gallatin]

On 05/30/12 10:59, Colin Percival wrote:
> Hi all,
>
> The Xen virtual network interface has an issue (ok, really the issue is with
> the linux back-end, but that's what most people are using) where it can't
> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
> This currently bites us hard with TSO enabled, since it produces said long
> mbuf chains.

Colin,

Thanks for pointing me at this.  I've been talking about this
with bz@ a little.

I've never been clear about what the max TSO size supported by FreeBSD
is.  The NIC I maintain (mxge) is limited to 64K - epsilon for both
IPv4 *AND* IPv6.  Up until now, this has been enforced by the 16-bit
ip length limit of IPv4 and we have not had IPv6 TSO until this week.
With IPv6, I'm worried that FreeBSD may now send packets down larger
than I could handle.  In my case, however, the problem is not s/g list
length, but rather it is internal limits in the NIC which limit us to
64K - epsilon for IPv6 as well.  I think there may be other NICs in
the same boat for IPv6 (and maybe even some which cannot handle the
full 64K for IPv4).

Your approach would not work well for my size limit.  For
example, I'd have to set the limit to 4 mbufs to stay under 64KB.
This would be assuming the worst case of 16KB jumbo mbufs, so
that would limit me to ~8KB per TSO if 2KB mbufs were used.

I think a better approach would be to have a limit on the size of the
pre-segmented TCP payload size sent to the driver.  I tend to think
that this would be more generically useful, and it is a better match
for the NDIS APIs, where a driver must specify the max TSO size.  I
think the changes to the TCP stack might be simpler (eg, they
would seem to jive better with the existing "maxmtu" approach).

I think this could work for you as well.  You could set the Xen max
tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
2KB mbuf size, with some slack built in).  If the chain was too large,
you could m_defrag it down to size.



Drew


_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Jack Vogel]

On Wed, May 30, 2012 at 8:30 AM, Andrew Gallatin <[hidden email]>wrote:

> On 05/30/12 10:59, Colin Percival wrote:
>
>> Hi all,
>>
>> The Xen virtual network interface has an issue (ok, really the issue is
>> with
>> the linux back-end, but that's what most people are using) where it can't
>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>> This currently bites us hard with TSO enabled, since it produces said long
>> mbuf chains.
>>
>
> Colin,
>
> Thanks for pointing me at this.  I've been talking about this
> with bz@ a little.
>
> I've never been clear about what the max TSO size supported by FreeBSD
> is.  The NIC I maintain (mxge) is limited to 64K - epsilon for both
> IPv4 *AND* IPv6.  Up until now, this has been enforced by the 16-bit
> ip length limit of IPv4 and we have not had IPv6 TSO until this week.
> With IPv6, I'm worried that FreeBSD may now send packets down larger
> than I could handle.  In my case, however, the problem is not s/g list
> length, but rather it is internal limits in the NIC which limit us to
> 64K - epsilon for IPv6 as well.  I think there may be other NICs in
> the same boat for IPv6 (and maybe even some which cannot handle the
> full 64K for IPv4).
>
> Your approach would not work well for my size limit.  For
> example, I'd have to set the limit to 4 mbufs to stay under 64KB.
> This would be assuming the worst case of 16KB jumbo mbufs, so
> that would limit me to ~8KB per TSO if 2KB mbufs were used.
>
> I think a better approach would be to have a limit on the size of the
> pre-segmented TCP payload size sent to the driver.  I tend to think
> that this would be more generically useful, and it is a better match
> for the NDIS APIs, where a driver must specify the max TSO size.  I
> think the changes to the TCP stack might be simpler (eg, they
> would seem to jive better with the existing "maxmtu" approach).
>
> I think this could work for you as well.  You could set the Xen max
> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
> 2KB mbuf size, with some slack built in).  If the chain was too large,
> you could m_defrag it down to size.
>

Think I favor Drew's idea as well for what that's worth.

Jack
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Colin Percival-4]

On 05/30/12 08:30, Andrew Gallatin wrote:

> On 05/30/12 10:59, Colin Percival wrote:
>> The Xen virtual network interface has an issue (ok, really the issue is with
>> the linux back-end, but that's what most people are using) where it can't
>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>> This currently bites us hard with TSO enabled, since it produces said long
>> mbuf chains.
>
> I've never been clear about what the max TSO size supported by FreeBSD
> is.  The NIC I maintain (mxge) is limited to 64K - epsilon for both
> IPv4 *AND* IPv6.  Up until now, this has been enforced by the 16-bit
> ip length limit of IPv4 and we have not had IPv6 TSO until this week.
> With IPv6, I'm worried that FreeBSD may now send packets down larger
> than I could handle.  In my case, however, the problem is not s/g list
> length, but rather it is internal limits in the NIC which limit us to
> 64K - epsilon for IPv6 as well.  I think there may be other NICs in
> the same boat for IPv6 (and maybe even some which cannot handle the
> full 64K for IPv4).
>
> Your approach would not work well for my size limit.  For
> example, I'd have to set the limit to 4 mbufs to stay under 64KB.
> This would be assuming the worst case of 16KB jumbo mbufs, so
> that would limit me to ~8KB per TSO if 2KB mbufs were used.

Right, the problem you describe isn't the one I was trying to solve. :-)

> I think a better approach would be to have a limit on the size of the
> pre-segmented TCP payload size sent to the driver.  I tend to think
> that this would be more generically useful, and it is a better match
> for the NDIS APIs, where a driver must specify the max TSO size.  I
> think the changes to the TCP stack might be simpler (eg, they
> would seem to jive better with the existing "maxmtu" approach).
>
> I think this could work for you as well.  You could set the Xen max
> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
> 2KB mbuf size, with some slack built in).  If the chain was too large,
> you could m_defrag it down to size.

Sounds good -- I don't want to m_defrag too often, but I imagine in most
cases when TSO is being invoked most of the mbufs will have 2 kB each.
This should also make the patch simpler by avoiding the need to modify
uipc_mbuf.c; if we just limit the TSO payload size then the TCP stack can
figure things out by itself.

Are you working on a patch, or should I put one together?

--
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Andrew Gallatin]

On 05/30/12 18:35, Colin Percival wrote:

> On 05/30/12 08:30, Andrew Gallatin wrote:
>> On 05/30/12 10:59, Colin Percival wrote:
>>> The Xen virtual network interface has an issue (ok, really the issue is with
>>> the linux back-end, but that's what most people are using) where it can't
>>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>>> This currently bites us hard with TSO enabled, since it produces said long
>>> mbuf chains.
>>
>> I've never been clear about what the max TSO size supported by FreeBSD
>> is.  The NIC I maintain (mxge) is limited to 64K - epsilon for both
>> IPv4 *AND* IPv6.  Up until now, this has been enforced by the 16-bit
>> ip length limit of IPv4 and we have not had IPv6 TSO until this week.
>> With IPv6, I'm worried that FreeBSD may now send packets down larger
>> than I could handle.  In my case, however, the problem is not s/g list
>> length, but rather it is internal limits in the NIC which limit us to
>> 64K - epsilon for IPv6 as well.  I think there may be other NICs in
>> the same boat for IPv6 (and maybe even some which cannot handle the
>> full 64K for IPv4).
>>
>> Your approach would not work well for my size limit.  For
>> example, I'd have to set the limit to 4 mbufs to stay under 64KB.
>> This would be assuming the worst case of 16KB jumbo mbufs, so
>> that would limit me to ~8KB per TSO if 2KB mbufs were used.
>
> Right, the problem you describe isn't the one I was trying to solve. :-)
>
>> I think a better approach would be to have a limit on the size of the
>> pre-segmented TCP payload size sent to the driver.  I tend to think
>> that this would be more generically useful, and it is a better match
>> for the NDIS APIs, where a driver must specify the max TSO size.  I
>> think the changes to the TCP stack might be simpler (eg, they
>> would seem to jive better with the existing "maxmtu" approach).
>>
>> I think this could work for you as well.  You could set the Xen max
>> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
>> 2KB mbuf size, with some slack built in).  If the chain was too large,
>> you could m_defrag it down to size.
>
> Sounds good -- I don't want to m_defrag too often, but I imagine in most
> cases when TSO is being invoked most of the mbufs will have 2 kB each.
> This should also make the patch simpler by avoiding the need to modify
> uipc_mbuf.c; if we just limit the TSO payload size then the TCP stack can
> figure things out by itself.
>
> Are you working on a patch, or should I put one together?
>

No, I'd like to, but I'm afraid that I just don't have the time
right now.  I would very much appreciate it if you could put it
together.  I'd be happy to review it.

Thanks,

Drew
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Colin Percival-4]

On 05/30/12 08:30, Andrew Gallatin wrote:

> On 05/30/12 10:59, Colin Percival wrote:
>> The Xen virtual network interface has an issue (ok, really the issue is with
>> the linux back-end, but that's what most people are using) where it can't
>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>> This currently bites us hard with TSO enabled, since it produces said long
>> mbuf chains.
>
> I think a better approach would be to have a limit on the size of the
> pre-segmented TCP payload size sent to the driver.  I tend to think
> that this would be more generically useful, and it is a better match
> for the NDIS APIs, where a driver must specify the max TSO size.  I
> think the changes to the TCP stack might be simpler (eg, they
> would seem to jive better with the existing "maxmtu" approach).
>
> I think this could work for you as well.  You could set the Xen max
> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
> 2KB mbuf size, with some slack built in).  If the chain was too large,
> you could m_defrag it down to size.

I've attached a new patch which:
1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
2. sets these in netfront when the IFCAP_TSO4 flag is set,
3. extends tcp_maxmtu to read this value,
4. adds a tx_tso_mss field to struct tcpcb,
5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
6. limits TSO lengths to tx_tso_mss in tcp_output.

--
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Andrew Gallatin]

On 06/03/12 12:51, Colin Percival wrote:

> On 05/30/12 08:30, Andrew Gallatin wrote:
>> On 05/30/12 10:59, Colin Percival wrote:
>>> The Xen virtual network interface has an issue (ok, really the issue is with
>>> the linux back-end, but that's what most people are using) where it can't
>>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>>> This currently bites us hard with TSO enabled, since it produces said long
>>> mbuf chains.
>>
>> I think a better approach would be to have a limit on the size of the
>> pre-segmented TCP payload size sent to the driver.  I tend to think
>> that this would be more generically useful, and it is a better match
>> for the NDIS APIs, where a driver must specify the max TSO size.  I
>> think the changes to the TCP stack might be simpler (eg, they
>> would seem to jive better with the existing "maxmtu" approach).
>>
>> I think this could work for you as well.  You could set the Xen max
>> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
>> 2KB mbuf size, with some slack built in).  If the chain was too large,
>> you could m_defrag it down to size.
>
> I've attached a new patch which:
> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
> 3. extends tcp_maxmtu to read this value,
> 4. adds a tx_tso_mss field to struct tcpcb,
> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
> 6. limits TSO lengths to tx_tso_mss in tcp_output.
>

Looks good to me.  I don't pretend to understand the bowels of
the TCP stack, so I can't comment on the "sendalot" stuff to
force segmentation.  I assume it works as well as your
previous patch to solve the problems you were seeing with Xen?

One minor nit is that I envision this limit to always be
64K or less, so you could probably get away with stealing
16 bits from ifcspare.

The other trivial nit is that I would have made these new
if_tx_tso_mss and  t_tx_tso_mss fields unsigned.

Thanks so much for doing this!

Drew
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Colin Percival-4]

On 06/03/12 12:05, Andrew Gallatin wrote:

> On 06/03/12 12:51, Colin Percival wrote:
>> I've attached a new patch which:
>> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
>> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
>> 3. extends tcp_maxmtu to read this value,
>> 4. adds a tx_tso_mss field to struct tcpcb,
>> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
>> 6. limits TSO lengths to tx_tso_mss in tcp_output.
>
> Looks good to me.  I don't pretend to understand the bowels of
> the TCP stack, so I can't comment on the "sendalot" stuff to
> force segmentation.

The "sendalot" bit is rather confusing, and I'm not at all certain
that it's doing what it's supposed to be doing, but my reading of
the code is that it really means "there's more data buffered than
what we're sending right now so after we issue this write we need
to go back to the top and do another".

> I assume it works as well as your
> previous patch to solve the problems you were seeing with Xen?

Yes.

> One minor nit is that I envision this limit to always be
> 64K or less, so you could probably get away with stealing
> 16 bits from ifcspare.

With IPv6 I could imagine larger limits happening, so I figured
it was better to take one of the ints.

> The other trivial nit is that I would have made these new
> if_tx_tso_mss and  t_tx_tso_mss fields unsigned.

Oops, quite right.  I was trying to make sure I didn't break ABI,
but of course int and u_int should have the same size no matter
what platform we're on.

> Thanks so much for doing this!

Thanks for reviewing.  Updated patch attached with s/int/u_int/ and
some other minor cleanups.

Can anyone review this for ABI safety?

--
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Bjoern A. Zeeb]

On 3. Jun 2012, at 16:51 , Colin Percival wrote:

> On 05/30/12 08:30, Andrew Gallatin wrote:
>> On 05/30/12 10:59, Colin Percival wrote:
>>> The Xen virtual network interface has an issue (ok, really the issue is with
>>> the linux back-end, but that's what most people are using) where it can't
>>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>>> This currently bites us hard with TSO enabled, since it produces said long
>>> mbuf chains.
>>
>> I think a better approach would be to have a limit on the size of the
>> pre-segmented TCP payload size sent to the driver.  I tend to think
>> that this would be more generically useful, and it is a better match
>> for the NDIS APIs, where a driver must specify the max TSO size.  I
>> think the changes to the TCP stack might be simpler (eg, they
>> would seem to jive better with the existing "maxmtu" approach).
>>
>> I think this could work for you as well.  You could set the Xen max
>> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
>> 2KB mbuf size, with some slack built in).  If the chain was too large,
>> you could m_defrag it down to size.
>
> I've attached a new patch which:
> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
> 3. extends tcp_maxmtu to read this value,
> 4. adds a tx_tso_mss field to struct tcpcb,
> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
> 6. limits TSO lengths to tx_tso_mss in tcp_output.

general question - what happens in the IPv6 case?


--
Bjoern A. Zeeb                                 You have to have visions!
   It does not matter how good you are. It matters what good you do!

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Colin Percival-4]

On 06/03/12 15:56, Bjoern A. Zeeb wrote:

> On 3. Jun 2012, at 16:51 , Colin Percival wrote:
>> I've attached a new patch which:
>> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
>> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
>> 3. extends tcp_maxmtu to read this value,
>> 4. adds a tx_tso_mss field to struct tcpcb,
>> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
>> 6. limits TSO lengths to tx_tso_mss in tcp_output.
>
> general question - what happens in the IPv6 case?

It should work just fine; when I said "tcp_maxmtu" I really meant "tcp_maxmtu
and tcp_maxmtu6".

Of course, this all only affects TSO, and the netfront driver doesn't enable
TSO for IPv6 (yet), so in a sense the real answer is "nothing at all".  But
the infrastructure will be in place for when it's needed.

--
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Lawrence Stewart-3]

On 06/04/12 02:51, Colin Percival wrote:

> On 05/30/12 08:30, Andrew Gallatin wrote:
>> On 05/30/12 10:59, Colin Percival wrote:
>>> The Xen virtual network interface has an issue (ok, really the issue is with
>>> the linux back-end, but that's what most people are using) where it can't
>>> handle scatter-gather writes with lots of pieces, aka. long mbuf chains.
>>> This currently bites us hard with TSO enabled, since it produces said long
>>> mbuf chains.
>>
>> I think a better approach would be to have a limit on the size of the
>> pre-segmented TCP payload size sent to the driver.  I tend to think
>> that this would be more generically useful, and it is a better match
>> for the NDIS APIs, where a driver must specify the max TSO size.  I
>> think the changes to the TCP stack might be simpler (eg, they
>> would seem to jive better with the existing "maxmtu" approach).
>>
>> I think this could work for you as well.  You could set the Xen max
>> tso size to be 32K (derived from 18 pages/skb, multiplied by a typical
>> 2KB mbuf size, with some slack built in).  If the chain was too large,
>> you could m_defrag it down to size.
>
> I've attached a new patch which:
> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,

A minor thing, but I don't like the overloading of the term MSS. Perhaps
s/MSS/CHUNKSIZE or another suitably similar but different word/phrase?

> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
> 3. extends tcp_maxmtu to read this value,
> 4. adds a tx_tso_mss field to struct tcpcb,
> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
> 6. limits TSO lengths to tx_tso_mss in tcp_output.

Is there a reason to mix signed and unsigned types for the various
tx_tso_mss variables? If not, please pick a type and stick with it.

I've added andre@ to CC in the hope he will eyeball this too given that
he, if I recall correctly, was the original "Mr TSO".

Cheers,
Lawrence
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Colin Percival-4]

On 06/03/12 20:14, Lawrence Stewart wrote:
> On 06/04/12 02:51, Colin Percival wrote:
>> I've attached a new patch which:
>> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
>
> A minor thing, but I don't like the overloading of the term MSS. Perhaps
> s/MSS/CHUNKSIZE or another suitably similar but different word/phrase?

I don't see this as being overloading; rather, it's using the same term to mean
exactly the same thing: Maximum Segment Size, in this case of a TCP segment
which is going to be re-segmented in hardware.

>> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
>> 3. extends tcp_maxmtu to read this value,
>> 4. adds a tx_tso_mss field to struct tcpcb,
>> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
>> 6. limits TSO lengths to tx_tso_mss in tcp_output.
>
> Is there a reason to mix signed and unsigned types for the various tx_tso_mss
> variables? If not, please pick a type and stick with it.

It's all unsigned now (see updated patch I sent in reply to gallatin@).  The
use of u_int vs. uint32_t is because I wanted to have this fit into spares in
struct ifnet and struct tcpcb and I wasn't 100% certain that there weren't any
platforms with sizeof(int) != sizeof(uint32_t).

--
Colin Percival
Security Officer Emeritus, FreeBSD | The power to serve
Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"

[Lawrence Stewart-3]

On 06/04/12 13:33, Colin Percival wrote:

> On 06/03/12 20:14, Lawrence Stewart wrote:
>> On 06/04/12 02:51, Colin Percival wrote:
>>> I've attached a new patch which:
>>> 1. adds a IFCAP_TSO_MSS "capability" and a if_tx_tso_mss field to struct ifnet,
>>
>> A minor thing, but I don't like the overloading of the term MSS. Perhaps
>> s/MSS/CHUNKSIZE or another suitably similar but different word/phrase?
>
> I don't see this as being overloading; rather, it's using the same term to mean
> exactly the same thing: Maximum Segment Size, in this case of a TCP segment
> which is going to be re-segmented in hardware.

No, it's not the same thing. "Maximum segment size" and "segment" have
very specific meaning in RFCs and data networking parlance. To overload
the term in the way that you are is a bad idea IMO.

A "segment" is the transport layer's protocol data unit (PDU), with MSS
specifying the maximum size of the transport's PDU. I'm not very au fait
with the TSO implementation specifics, but my understanding is that we
do not "send" a 65k segment to the hardware as a fully specified TCP
segment and then have the the hardware also churn out fully specified
TCP segments, only smaller in size. We send the hardware a chunk of data
and partial header, which is then chopped up and sent as fully specified
segments.

Assuming my understanding is correct, to refer to the TSO chunk size as
MSS is therefore an inappropriate use of the term at best, potentially
confusing and misleading at worst.

>>> 2. sets these in netfront when the IFCAP_TSO4 flag is set,
>>> 3. extends tcp_maxmtu to read this value,
>>> 4. adds a tx_tso_mss field to struct tcpcb,
>>> 5. makes tcp_mss_update set tx_tso_mss using tcp_maxmtu, and
>>> 6. limits TSO lengths to tx_tso_mss in tcp_output.
>>
>> Is there a reason to mix signed and unsigned types for the various tx_tso_mss
>> variables? If not, please pick a type and stick with it.
>
> It's all unsigned now (see updated patch I sent in reply to gallatin@).  The

Sorry, missed the revised patch. Unsigned looks good to me.

> use of u_int vs. uint32_t is because I wanted to have this fit into spares in
> struct ifnet and struct tcpcb and I wasn't 100% certain that there weren't any
> platforms with sizeof(int) != sizeof(uint32_t).

I've got no issue with that.

Cheers,
Lawrence
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[hidden email]"