From owner-freebsd-net@FreeBSD.ORG Mon Jun 4 04:07:36 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1D29A1065670; Mon, 4 Jun 2012 04:07:36 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (lauren.room52.net [210.50.193.198]) by mx1.freebsd.org (Postfix) with ESMTP id CEC2A8FC1F; Mon, 4 Jun 2012 04:07:35 +0000 (UTC) Received: from lstewart.caia.swin.edu.au (lstewart.caia.swin.edu.au [136.186.229.95]) by lauren.room52.net (Postfix) with ESMTPSA id A72DE7E891; Mon, 4 Jun 2012 14:07:34 +1000 (EST) Message-ID: <4FCC3486.3020104@freebsd.org> Date: Mon, 04 Jun 2012 14:07:34 +1000 From: Lawrence Stewart User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:10.0.2) Gecko/20120311 Thunderbird/10.0.2 MIME-Version: 1.0 To: Colin Percival References: <4FC635CC.5030608@freebsd.org> <4FC63D27.70807@cs.duke.edu> <4FCB95F6.30204@freebsd.org> <4FCC27FE.3030205@freebsd.org> <4FCC2C72.7000806@freebsd.org> In-Reply-To: <4FCC2C72.7000806@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY autolearn=unavailable version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on lauren.room52.net Cc: freebsd-net@freebsd.org, Andre Oppermann , Andrew Gallatin Subject: Re: [please review] TSO mbuf chain length limiting patch X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jun 2012 04:07:36 -0000 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