tcp: allow a variable of type tt_which to be used as an index for tcptimers[]

[tuexen]

Summary

The sequence of timers in the definition of tt_which and in tcptimers is inconsistent. This patch fixes this.

[rscheff]

[glebius]

No objection with existing patch. I wonder if there is some way to prevent them ever getting different again.

[glebius]

Richard emailed me with a suggestion to improve this. Here is my interpretation:

In tcp_timer.h:

typedef enum {
        TT_REXMT = 0,
        TT_PERSIST,
        TT_KEEP,
        TT_2MSL,
        TT_DELACK,
        TT_N,
} tt_which;

#ifdef  TCPTIMERS
static const char *tcptimers[] =
{
        [TT_REXMT] = "REXMT",
        [TT_PERSIST] = "PERSIST",
        [TT_KEEP] = "KEEP",
        [TT_2MSL] = "2MSL",
        [TT_DELACK] = "DELACK",
};
_Static_assert(sizeof(tcptimers)/sizeof(tcptimers[0]) == TT_N)
#endif

[tuexen]

In D38547#878067, @glebius wrote:

Richard emailed me with a suggestion to improve this. Here is my interpretation:

In tcp_timer.h:

typedef enum {
        TT_REXMT = 0,
        TT_PERSIST,
        TT_KEEP,
        TT_2MSL,
        TT_DELACK,
        TT_N,
} tt_which;

#ifdef  TCPTIMERS
static const char *tcptimers[] =
{
        [TT_REXMT] = "REXMT",
        [TT_PERSIST] = "PERSIST",
        [TT_KEEP] = "KEEP",
        [TT_2MSL] = "2MSL",
        [TT_DELACK] = "DELACK",
};
_Static_assert(sizeof(tcptimers)/sizeof(tcptimers[0]) == TT_N)
#endif

Moving the definition of tt_which from tcp_var.h to tcp_timer.h breaks the compilation, because the declaration of tcp_timer_activate and tcp_timer_active in tcp_var.h uses tt_which. We can't move the definition of tcptimers around without breaking the compatibility. If we include tcp_timer.h in tcp_var.h, we require the #define TCPTIMERS to go before including tcp_var.h instead of tcp_timer.h. A solution would be to remove the #ifdef TCPTIMERS and include tcp_timer.h in tcp_var.h. Or keep the definition of tt_which in tcp_var.h and include tcp_var.h in tcp_timer.h, if TCPTIMERS is defined. Any opinions?

[rscheff]

Why would you need to remove the #ifdef TCPTIMERS when including tcp_var.h in tcp_timers.h?

Also, shouldn't it suffice to includ)e tcp_timers.h after tcp_var.h in the various .c files? (But including the tcp_var.h in tcp_timers.h should be safe with the _NETINET_TCP_VAR_H protection...

[tuexen]

In D38547#878511, @rscheff wrote:

Why would you need to remove the #ifdef x when including tcp_var.h in tcp_timers.h?

That does not solve the problem that some function definitions in tcp_var.h require the type definition in tcp_timers.h.
I was referring to include tcp_timers.h in tcp_var.h. If we keep the #ifdef TCPTIMERS, the following code would break:

#include <netinet/tcp_var.h>
#define TCPTIMERS
#include <netinet/tcp_timer.h

because tcp_timer.h would be included via tcp_var.h.

Another thing I just realized: the definition of tt_which is protected by #if defined(_KERNEL) || defined(_WANT_TCPCB), whereas the definition of tcptimers is not.
So do we want to expose tt_which to userland?

Also, shouldn't it suffice to include tcp_timers.h after tcp_var.h in the various .c files? (But including the tcp_var.h in tcp_timers.h should be safe with the _NETINET_TCP_VAR_H protection...

[tuexen]

Ensure the enum and the string keep in sync in the future based on a suggestion by rscheff@.

[cc]

Good to know this. Thanks.

[glebius]

What programs actually #define TCPTIMERS? Maybe patch all of them to include tcp_var.h instead of tcp_timer.h?

[tuexen]

In D38547#878664, @glebius wrote:

What programs actually #define TCPTIMERS? Maybe patch all of them to include tcp_var.h instead of tcp_timer.h?

I have no idea. None in the source tree. Don't know about the ports tree.

If we are willing to accept that some things break, why not remove tcptimers at all? Then nothing needs to be kept in sync.

However, I would prefer TT_REXMT to be zero. That way I can extend the BBLogging for TCP timers while being backwards compatible and use tt_which. That is what the initial version of the patch allows me to do. If that is not acceptable, I can use my own set of constants identifying the timers.

[cc]

In D38547#878664, @glebius wrote:

What programs actually #define TCPTIMERS? Maybe patch all of them to include tcp_var.h instead of tcp_timer.h?

Good question.
That is part of the earliest FreeBSD patch I can found: BSD 4.4 Lite Kernel Sources

It was defined in tcp_debug.c, and now it seems to be not used anywhere else. I think it is time to be removed as dead code.

[tuexen]

In D38547#878961, @guest-ccui wrote:

In D38547#878664, @glebius wrote:

What programs actually #define TCPTIMERS? Maybe patch all of them to include tcp_var.h instead of tcp_timer.h?

Good question.
That is part of the earliest FreeBSD patch I can found: BSD 4.4 Lite Kernel Sources

It was defined in tcp_debug.c, and now it seems to be not used anywhere else. I think it is time to be removed as dead code.

Just figured out that trpt also used it for printing. However, the code generating the entries never put the information in the record and therefore all timeouts are reported as retransmission timeouts. However, this tool is deprecated and has been removed from head.

So what about just removing tcptimers and changing tt_which as suggested. I would even be fine to keep the definition where it is limiting its visibility to the kernel.

[cc]

In D38547#879277, @tuexen wrote:

In D38547#878961, @guest-ccui wrote:

In D38547#878664, @glebius wrote:

What programs actually #define TCPTIMERS? Maybe patch all of them to include tcp_var.h instead of tcp_timer.h?

Good question.
That is part of the earliest FreeBSD patch I can found: BSD 4.4 Lite Kernel Sources

It was defined in tcp_debug.c, and now it seems to be not used anywhere else. I think it is time to be removed as dead code.

Just figured out that trpt also used it for printing. However, the code generating the entries never put the information in the record and therefore all timeouts are reported as retransmission timeouts. However, this tool is deprecated and has been removed from head.

So what about just removing tcptimers and changing tt_which as suggested. I would even be fine to keep the definition where it is limiting its visibility to the kernel.

+1 for "just removing tcptimers and changing tt_which as suggested"

[tuexen]

Next try: remove the definition of tcptimers, keep tt_which visible to the kernel only, but ensure that TT_REXMT is encoded as 0, since this allows using these constants in an upcoming BBLog patch while providing backwards compatibility.