From owner-freebsd-net@FreeBSD.ORG Sat Sep 6 00:06:25 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 64798D99; Sat, 6 Sep 2014 00:06:25 +0000 (UTC) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "funkthat.com", Issuer "funkthat.com" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 3E75D126A; Sat, 6 Sep 2014 00:06:24 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id s8606OhI026138 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 5 Sep 2014 17:06:24 -0700 (PDT) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id s8606OOY026137; Fri, 5 Sep 2014 17:06:24 -0700 (PDT) (envelope-from jmg) Date: Fri, 5 Sep 2014 17:06:23 -0700 From: John-Mark Gurney To: Hans Petter Selasky Subject: Re: [RFC] Patch to improve TSO limitation formula in general Message-ID: <20140906000623.GQ82175@funkthat.com> Mail-Followup-To: Hans Petter Selasky , FreeBSD Current , "freebsd-net@freebsd.org" , Scott Long References: <540A0301.9040701@selasky.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <540A0301.9040701@selasky.org> User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-TipJar: bitcoin:13Qmb6AeTgQecazTWph4XasEsP7nGRbAPE X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Fri, 05 Sep 2014 17:06:24 -0700 (PDT) Cc: "freebsd-net@freebsd.org" , FreeBSD Current , Scott Long X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Sep 2014 00:06:25 -0000 Hans Petter Selasky wrote this message on Fri, Sep 05, 2014 at 20:37 +0200: > I've tested the attached patch with success and would like to have some > feedback from other FreeBSD network developers. The problem is that the > current TSO limitation only limits the number of bytes that can be > transferred in a TSO packet and not the number of mbuf's. > > The current solution is to have a quick and dirty custom m_dup() in the > TX path to re-allocate the mbuf chains into 4K ones to make it simple. > All of this hack can be avoided if the definition of the TSO limit can > be changed a bit, like shown here: > > > /* > + * Structure defining hardware TSO limits. > + */ > +struct if_tso_limit { > + u_int raw_value[0]; /* access all fields as one */ > + u_char frag_count; /* maximum number of fragments: 1..255 */ > + u_char frag_size_log2; /* maximum fragment size: 2 ** (12..16) */ > + u_char hdr_size_log2; /* maximum header size: 2 ** (2..8) */ > + u_char reserved; /* zero */ > +}; Please make this a union if you really need to access the raw_value, or just drop it... Is this done to fit in the u_int t_tsomax that is in tcpcb? Also, I couldn't find code, but if the tcp connection needs to be sent out a different interface that has more restrictive tso requirements, do we properly handle this case? My quick reading of the code seems to imply that we only get the TSO requirements on connection and never update it... As these are per if, saving memory by packing them isn't really that effective these days... Per the later comments, yes, a shift MAY be faster than a full mul/div by a cycle or two, but this won't make that huge of a difference when dealing with it.. If the programmer has to use crazy macros or do the math EVERY time they use the fields, this will end up w/ less readable/maintainable code at the cost of improving performance by maybe .001%, so my vote is for u_int's instead, and convert to their sizes properly... Comments on the patch: You can drop the .reserve initalization... It is common C knowlege that unassigned members are assigned zero... The IF_TSO_LIMIT_CMP macros seems excesive... Do you ever see a need to use other operators? and if so, would they be useful? I'd just convert it to: #define IF_TSO_LIMIT_EQ(a, b) ((a)->raw_value[0] == (b)->raw_value[0]) I am a bit puzzled by this code: + /* Check if fragment limit will be exceeded */ + if (cur_frags >= rem_frags) { + max_len += min(cur_length, rem_frags << if_hw_tsomax.frag_size_log2); + break; + } specificly the max_len += line... The code seems to say if we would overrun the remaining frags (maybe you want a > instead of >=) we increase max_len... seems like if we include this frag that would put us over the limit that we should just skip it? (break w/o increasing max_len).. -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."