tcp: provide tcp_rttupdated() and call it periodically
Needs ReviewPublic
Actions

Authored by glebius on Sat, Sep 5, 5:45 PM.

Details

Reviewers

rscheff

Group Reviewers

transport

Summary

On every 4-th rtt update (or session close) push connection data into the
TCP hostcache. The "every 4-th" is arbitrarily defined following previous
logic for a closed connection.

This change makes the TCP stack to add to the TCP hostcache during
a connection lifetime. Modern browsers usually open multiple connections
to a site and connections that were created later now can benefit from
host cache data that was gathered by earlier (but still open) connection.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76577
Build 73460: arc lint + arc unit

Event Timeline

glebius created this revision.Sat, Sep 5, 5:45 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptSat, Sep 5, 5:45 PM

Herald added subscribers: melifaro, imp. · View Herald Transcript

glebius requested review of this revision.Sat, Sep 5, 5:45 PM

Harbormaster completed remote builds in B76577: Diff 185943.Sat, Sep 5, 5:45 PM

glebius added a parent revision: D59453: tcp: use sparse initializer for host cache metrics.Sat, Sep 5, 5:45 PM

No local variable.

Harbormaster completed remote builds in B76584: Diff 185953.Sat, Sep 5, 7:08 PM

tuexen added a subscriber: tuexen.Sat, Sep 5, 10:49 PM

tuexen added inline comments.

sys/netinet/tcp_subr.c
2479	What about this comment? Don't you need to deal with different stacks measure the RTT in different units?
2588	As commented earlier, what about the time unit for `srtt` and `rttvar`? Adding entries during the lifetime of a connection changes the semantic. Wouldn't it make more sense to keep the old behavior, but store updated buffersize when they are changed? Wouldn't that give you what you want with less impact?

glebius added inline comments.Mon, Sep 7, 8:18 PM

sys/netinet/tcp_subr.c
2479	I think Randall was thinking of a potential thing rather than existing. Right now rack_fini() doesn't do any manipulations with `t_rttupdated`. Also, there is no signed contract, but I would dare to say that alternative stacks shall treat `struct tcpcb` members exactly the same way the default stack does. If a stack wants to do something special it has its own control block in `t_fb_ptr`.
2588	I really don't know and looking for your advice here. My naive logic is the following. Right now, before this change, if a connection A has went through 4 (or more) RTT updates and is closed, then a host cache entry is updated(or created) and a new connection B presumably would benefit from this. However, if connection A went through the same number of updates and stays idle, the new connection B will not benefit. If we presume that such host cache update is beneficial for all members of host cache structure, then let's update all of them. My personal testing was of course mostly focused on the socket buffer limits.