tcp: remove the `goto drop` label by reusing equivalences in tcp_do_segment().

[cc]

Summary

One patch at a time, this is one of the few steps to re-factor these super
long TCP functions.

The eventual goal is to extract the mass of tcp loss recovery logic out of
tcp_do_segment().

Test Plan

no functional change

[rscheff]

While I agree that "goto" statements are not nice, and hard to follow - there is something to be said about the overhead of a function preamble, stack use and cleanup. I would suggest to make this an "__inline" function.

sys/netinet/tcp_input.c

1523

static __inline void
should prevent the superfluous preamble/return code of a stand-alone function (each instance should effectively be placed into the other code block without extra preamble/return block)

2036

according to style(9) - unary operators example - a construct (!(<bool | int>)) is valid; For me, "not ack, or fin, or rst" reads better than masking this to get an int which then should be zero. (I like the style(9) better than the == 0).

Although style also has

Do not use ! for tests unless it is a boolean, e.g., use:
     if (*p == '\0')
not:
     if (!*p)

(But in this example it's obviously not a flag operation/construct which arguably represent booleans.

[cc]

Add the __inline keyword to avoid overhead when possible.

[cc]

sys/netinet/tcp_input.c

2036

Do not use ! for tests unless it is a boolean

For example, I didn't find the result of (flags & MASK) is a boolean. Here is an example from ChatGPT on the topic of what is the best C coding style for "if" condition of binary operation?

/* 
 * When performing bitwise operations, it's a good practice to explicitly check for zero or non-zero results to make the logic clearer, even though if implicitly treats non-zero as true.
 */
if ((flags & MASK) == 0) {  // Positive condition is easier to understand
    // Do something
}

[rscheff]

sys/netinet/tcp_input.c

1523

as discussed, just "inline" would do.

[cc]

update code based on discussion

[glebius]

We discussed this change (and supposedly following up changes) on the TCP call 2024-10-17. @cc , @rscheff , @tuexen , @thj and Peter Lei participated.

First, more context needs to pulled for those who did not participate in the call. There is overall dissatisfaction with how long tcp_do_segment() is, and especially with level of indentation in some of its parts. There is also a concern with use of goto. Cheng plans to improve the function and this review is just the first step.

Second, let's categorize problems with tcp_do_segment() into roughly three parts:

1. Deep level of indentation in certain places.
2. 4 gotos in the middle of the function, especially one that jumps backwards.
3. 4 gotos to different function epilogues.

This review addresses one of the gotos in #3.

I don't agree that use of goto should be an absolute taboo in the computer programming. The fear & hate towards goto is based on use of goto inside or outside a cycle and on a goto that jumps backwards. Using a goto to a cleanup epilogue is a standard solution used everywhere in the FreeBSD kernel, userland and endless list of other projects with overall good code quality.

Now I would argue that isolating function cleanup epilogue into a separate function doesn't improve readability. A reader still needs scrolling to check where resources are freed, just needs to scroll in a different direction. The new function is supposed to be called only by tcp_do_segment() and must be followed by return (or a tail call may be used). But this is not enforced by the compiler in any way! This leads to at least two problems with future code development and maintenance:

1. A new developer may find a function useful and call it from other function than tcp_do_segment().
2. A new resource is allocated in tcp_do_segment() and a cleanup needs to be added. How many epilogue sub-functions need to be edited and not missed? In general, if a resource can be allocated and freed in the same function, that should be preferred to an option where allocation and free are parted.