Page MenuHomeFreeBSD

Remove unnecessary ifdef soup from struct tcpcb
AbandonedPublic

Authored by rstone on Feb 6 2017, 11:36 PM.

Details

Reviewers
pkelsey
Group Reviewers
transport

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7240
Build 7410: arc lint + arc unit

Event Timeline

rstone updated this revision to Diff 24816.Feb 6 2017, 11:36 PM
rstone retitled this revision from to Remove unnecessary ifdef soup from struct tcpcb.
rstone edited the test plan for this revision. (Show Details)
jch added a subscriber: jch.Feb 7 2017, 7:49 AM
rwatson added a subscriber: rwatson.Feb 7 2017, 1:24 PM

I like the idea, and encourage you to proceed, but be aware that struct tcpcb is part of the user-visible ABI for monitoring tools (sigh). Someone should restock our supplies of padding someday.

pkelsey edited edge metadata.Feb 14 2017, 10:06 PM

I don't have any TFO-specific complaints with removing those #ifdefs. I put them there out of uncertainty about any existing policy on userland-ABI concerns and they made it through code review. In hindsight, the #else parts of the #ifdefs should have had two items - t_Xopaque[] and t_Xspare[], because I was never of the mind that the space used by TFO might ever be used instead by something else that would then could only be enabled mutually-exclusively with TFO.

Regarding rstone's related comment in an email on transport@:

Unfortunately the TCPPCAP stuff isn't as easy to excise because it's trying
to avoid exposing struct mbufq to userland.

I realize the issue there is exposure of a type definition to userland, but a related question is - what exactly is the userland-ABI property we are trying to maintain here? Are we just trying to keep this struct the same size and layout under all kernel configuration option selections and feature evolutions for a given machine type? Or are we also trying to keep it the same size and layout across different machine types (or at the very least a single pair of relevant machine types)? For example, is the #ifdef_LP64 really necessary in the TCPPCAP section?

I realize the issue there is exposure of a type definition to userland, but a related question is - what exactly is the userland-ABI property we are trying to maintain here? Are we just trying to keep this struct the same size and layout under all kernel configuration option selections and feature evolutions for a given machine type? Or are we also trying to keep it the same size and layout across different machine types (or at the very least a single pair of relevant machine types)? For example, is the #ifdef_LP64 really necessary in the TCPPCAP section?

We are trying to keep the same size and layout for a given machine type. The issue is that the TCPPCAP stuff used up uint64_t padding, and consumed different amounts of it on LP64 and LP32 systems, hence the #ifdef.

We are trying to keep the same size and layout for a given machine type. The issue is that the TCPPCAP stuff used up uint64_t padding, and consumed different amounts of it on LP64 and LP32 systems, hence the #ifdef.

Yep, think-o on my part. I was thinking of this in a way other than chewing up space in an existing array of uint64_t padding.

pkelsey accepted this revision.Feb 15 2017, 1:29 AM
pkelsey edited edge metadata.
This revision is now accepted and ready to land.Feb 15 2017, 1:29 AM
rstone abandoned this revision.Mar 22 2017, 6:00 PM

Obsoleted by gleb's User/Kernel KPI fixes