Date: Thu, 14 Mar 2024 10:23:12 -0700 From: John Baldwin <jhb@FreeBSD.org> To: Gleb Smirnoff <glebius@freebsd.org>, Konstantin Belousov <kib@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 220ee18f1964 - main - netinet/tcp_var.h: always define IS_FASTOPEN() for kernel compilation env Message-ID: <56f538f1-d406-4095-972a-2022d48ffc73@FreeBSD.org> In-Reply-To: <ZfJDsRPzASbZ_Cav@cell.glebi.us> References: <202403132321.42DNLNIX087785@gitrepo.freebsd.org> <ZfJDsRPzASbZ_Cav@cell.glebi.us>
next in thread | previous in thread | raw e-mail | index | archive | help
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 > 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 -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56f538f1-d406-4095-972a-2022d48ffc73>