Re: git: 220ee18f1964 - main - netinet/tcp_var.h: always define IS_FASTOPEN() for kernel compilation env

On Thu, Mar 14, 2024 at 11:23 AM John Baldwin <jhb@freebsd.org> wrote:

> On 3/13/24 5:24 PM, Gleb Smirnoff wrote:
> > On Wed, Mar 13, 2024 at 11:21:23PM +0000, Konstantin Belousov wrote:
> > K> diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h
> > K> index 6f7f7115c2f4..7b5c57d39213 100644
> > K> --- a/sys/netinet/tcp_var.h
> > K> +++ b/sys/netinet/tcp_var.h
> > K> @@ -812,11 +812,13 @@ tcp_packets_this_ack(struct tcpcb *tp, tcp_seq
> ack)
> > K>  #define   ENTER_RECOVERY(t_flags) t_flags |= (TF_CONGRECOVERY |
> TF_FASTRECOVERY)
> > K>  #define   EXIT_RECOVERY(t_flags) t_flags &= ~(TF_CONGRECOVERY |
> TF_FASTRECOVERY)
> > K>
> > K> -#if defined(_KERNEL) && !defined(TCP_RFC7413)
> > K> +#if defined(_KERNEL)
> > K> +#if !defined(TCP_RFC7413)
> > K>  #define   IS_FASTOPEN(t_flags)            (false)
> > K>  #else
> > K>  #define   IS_FASTOPEN(t_flags)            (t_flags & TF_FASTOPEN)
> > K>  #endif
> > K> +#endif
> > K>
> > K>  #define   BYTES_THIS_ACK(tp, th)  (th->th_ack - tp->snd_una)
> >
> > I know Konstantin in doing that to clear path for IPSEC changes, and the
> patch
> > does improve code.  So the message isn't addressed to him, rather it is
> for
> > other src-committers.
> >
> > Using ifdefs that come from the kernel config inside include files that
> are not
> > ephemeral opt_foo.h is laying out a minefield for the future. Best case
> - for
> > yourself, worst case - for somebody else.
>
> +100
>

+100 as well. I can't tell you how many times I've been burned by this, had
to mop
up for it after the fact, etc over the years.


> > In the best case this ends in cryptic kernel failure builds, where your
> custom
> > kernel doesn't build, but GENERIC builds and it is not clear why. In the
> worst
> > case this creates runtime failures even more cryptic in their nature.
> >
> > In this particular case TCP_RFC7413 comes via opt_inet.h.  Thus, if you
> got a
> > userland tool, e.g. netstat(1) using IS_FASTOPEN() it will always be
> false.
> > Again, netstat would be a best case.  A worst case would be netinet6
> kernel
> > compilation unit that does not include opt_inet.h, but uses
> IS_FASTOPEN().
> >
> > Other than that, we got 32 flags for t_flags and only one is obfuscated
> via a
> > macro.  I'd really like to remove the macro and test the flag directly.
> Any
> > objections?
>
> +1
>

Yea, this macro isn't that special, and  there's no good reason from
looking at
the code it should get such special treatment.

Warner