Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 04 Jun 2012 14:07:34 +1000
From:      Lawrence Stewart <lstewart@freebsd.org>
To:        Colin Percival <cperciva@freebsd.org>
Cc:        freebsd-net@freebsd.org, Andre Oppermann <andre@freebsd.org>, Andrew Gallatin <gallatin@cs.duke.edu>
Subject:   Re: [please review] TSO mbuf chain length limiting patch
Message-ID:  <4FCC3486.3020104@freebsd.org>
In-Reply-To: <4FCC2C72.7000806@freebsd.org>
References:  <4FC635CC.5030608@freebsd.org> <4FC63D27.70807@cs.duke.edu> <4FCB95F6.30204@freebsd.org> <4FCC27FE.3030205@freebsd.org> <4FCC2C72.7000806@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FCC3486.3020104>