Re: [RFC] Patch to improve TSO limitation formula in general

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."