Page MenuHomeFreeBSD

Reduce size of t_rttupdated in tcpcb
AcceptedPublic

Authored by rscheff on Jul 30 2019, 5:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jan 13, 10:40 AM
Unknown Object (File)
Sun, Jan 8, 5:41 PM
Unknown Object (File)
Sat, Jan 7, 7:15 PM
Unknown Object (File)
Thu, Jan 5, 9:52 AM
Unknown Object (File)
Dec 29 2022, 11:35 AM
Unknown Object (File)
Dec 14 2022, 10:54 PM
Unknown Object (File)
Dec 12 2022, 12:03 PM
Unknown Object (File)
Dec 9 2022, 10:11 PM

Details

Summary
The TCPCB t_rttupdated is used initially to start using 
the rtt measurements only, once a number of valid data
points was collected.
However, using a unsigned long (64 bit) was overkill, when
the number of valid samples to have collected prior to
use of the timing information is well below 100.
Switching to a smaller type, and limit the increase to the
maximum allowed by that type.

Also, the general statistics around the number of rtt
samples is collected independently in tcps_rttupdated.
Test Plan
All packetdrill RTT related testing should pass identical
to prior of this change.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 49078
Build 45967: arc lint + arc unit

Event Timeline

  • only touch t_rttupdated and have portable macro to do post-increment with a ceiling

    That macro will used for other variables which really only track the initial state (e.g. in dctcp and cubic: num_cong_events, in cubic ticks_since_cong)
  • rename macro from INCMAX to CEILINC as we are (post)-incrementing to a ceiling depending on the type of the variable incremented

It might make sense to add gauge8_t, gauge16_t, gauge32_t and gauge64_t types and provide inline functions like

void inline
gauge8_incr(gauge8_t *gauge)
{
        if (*gauge < UINT8_MAX)
                *gauge++;
}

Using an (inline) function gives some sort of type safety... Not doing this in the TCP code would allow this to be used also in other code, like SCTP, where gauge16_t could be used for path error counters.

rgrimes added inline comments.
sys/netinet/tcp_var.h
140

Be aware this is a cache optimized data structure.

270

Changing the size of and location of this value is going to undo the comment about this structure being carefully cache aligned, though I do not know if that comment is still true.

717

Ah, now this explains why I could not find CEILINC on my 12.1 machine... This set of macros and usage should probably be discussed on the -arch mailling list. This is a general mechanism and should probably be considered on a grander scale and by a wider audiance than just netinet stuff

@tuexen:

I thought the set of macros provided can be evaluated at compile-time properly and in a portable way. However, they do break down for figuring out the signedness of char (int8_t / uint8_t) and short (int16_t / uint16_t).
A benefit of a macro would still be that it is a drop-in replacement for any post-increment operator. I think combining the macro with a new gauge type would make most sense.

An inline function approach would require the developer to adjust all these calls, when the width of a variable (type) is changed; also the function would need to read like

'''
gauge8_t inline
gauge8_incr(gauge8_t *gauge)
{

gauge8_t temp = *gauge;
if (*gauge < UINT8_MAX)
        (*gauge)++;
return(temp);

}
'''

In my testing, "*gauge++" was incrementing the pointer address after reading the value, not incrementing the value at the address. Also, at lower optimization levels, this inline function is not actually rolled up and made the simple 3 assembly operations (excluding mov's) as the Macro always is.

@rgrimes:

Yes, I'm well aware - rrs@ and other folks @NF have hand-optimized the first 3 or 4 cachelines of this struct, for the normal, in-sequence processing some time ago.

However, since that last optimization, the "/* cache line X */" comments haven't been kept up-to-date and currently don't correctly align any more.

Fields further down (cache lines 4+) are more or less fair game.

The one field I try to tackle here (around cacheline 5, maybe already 6 - or even split among both) really only needs to keep track of the first few (<10) rounds of RTT updates, and must never overflow (to bypass the processing, when that count is low again). As it's so far down in the structure, moving it into currently unused "alignment" space, and shrinking the size, the expectation is that this may actually help slightly, by shortening the TCPCB struct again.

As for

tcp_var.h:540
+ ((x) < UTYPE_MAX(x)) ? (x)++ : (x)
+
/*

Ah, now this explains why I could not find CEILINC on my 12.1 machine... This set of macros and usage should probably be discussed on the -arch mailling list. This is a general mechanism and should probably be considered on a grander scale and by a wider audiance than just netinet stuff

Perhaps create a Diff for gauge types and associated accessor / utility functions and macros would be a better plan, and make this a one-time (hardcoded) change until these functions become available - without MFC?

  • remove most macros and perform explicit if-then checks just for r_rttupdated
rscheff marked 2 inline comments as done.
  • make sure the macro also works for 64bit variables
guest-ccui added a subscriber: guest-ccui.

Looks a good idea. Thanks.

will use hardcode max_uint values in next rev.

  • rebase to main
  • use static maximum value for uint8_t
This revision is now accepted and ready to land.Thu, Jan 26, 3:34 PM