Re: git: be78a31188c5 - main - tcp: fix build issue for some cc modules

On Fri, Jul 14, 2023 at 5:51 AM Konstantin Belousov <kostikbel@gmail.com>
wrote:

> On Fri, Jul 14, 2023 at 01:28:54PM +0200, tuexen@freebsd.org wrote:
> > > On 14. Jul 2023, at 13:23, Konstantin Belousov <kostikbel@gmail.com>
> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 08:31:34PM +0000, Michael Tuexen wrote:
> > >> The branch main has been updated by tuexen:
> > >>
> > >> URL:
> https://cgit.FreeBSD.org/src/commit/?id=be78a31188c530c93700396ecfdb5604a8f22fff
> > >>
> > >> commit be78a31188c530c93700396ecfdb5604a8f22fff
> > >> Author:     Michael Tuexen <tuexen@FreeBSD.org>
> > >> AuthorDate: 2023-07-13 16:56:25 +0000
> > >> Commit:     Michael Tuexen <tuexen@FreeBSD.org>
> > >> CommitDate: 2023-07-13 16:56:25 +0000
> > >>
> > >>   tcp: fix build issue for some cc modules
> > >>
> > >>   The TCP_HHOOK option was moved from opt_inet.h to opt_global.h in
> > >>
> https://cgit.FreeBSD.org/src/commit/?id=e68b3792440cac248347afe08ba5881a00ba6523
> > >>   The corresponding changes in two Makefiles were missed, which
> resulted
> > >>   in not building cc_cdg, cc_chd, cc_hd, and cc_vegas anymore.
> > >>
> > >>   Reported by:            void@f-m.fm
> > >>   Reviewed by:            rrs, rscheff
> > >>   Sponsored by:           Netflix, Inc.
> > >>   Differential Revision:  https://reviews.freebsd.org/D41010
> > >> ---
> > >> sys/modules/cc/Makefile    | 6 +++---
> > >> sys/modules/khelp/Makefile | 6 +++---
> > >> 2 files changed, 6 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/sys/modules/cc/Makefile b/sys/modules/cc/Makefile
> > >> index 3f7110024722..b595cc204481 100644
> > >> --- a/sys/modules/cc/Makefile
> > >> +++ b/sys/modules/cc/Makefile
> > >> @@ -8,9 +8,9 @@ SUBDIR= cc_newreno \
> > >>
> > >> # Do we have the TCP_HHOOK symbol defined? If not, there is no point
> in
> > >> # building these modules by default.
> > >> -# We will default to building these modules unless $OPT_INET is
> defined
> > >> -# and does not contain the TCP_HHOOK option.
> > >> -.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK} != ""
> > >> +# We will default to building these modules if $OPT_GLOBAL does
> contain
> > >> +# the TCP_HHOOK option.
> > >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHOOK} != ""
> > >> SUBDIR+= \
> > >> cc_cdg \
> > >> cc_chd \
> > >> diff --git a/sys/modules/khelp/Makefile b/sys/modules/khelp/Makefile
> > >> index 256d8838c573..c01d61541062 100644
> > >> --- a/sys/modules/khelp/Makefile
> > >> +++ b/sys/modules/khelp/Makefile
> > >> @@ -4,9 +4,9 @@ SUBDIR=
> > >>
> > >> # Do we have the TCP_HHOOK symbol defined? If not, there is no point
> in
> > >> # building this modules by default.
> > >> -# We will default to building this module unless $OPT_INET is defined
> > >> -# and does not contain the TCP_HHOOK option.
> > >> -.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK} != ""
> > >> +# We will default to building this module if $OPT_GLOBAL does contain
> > >> +# the TCP_HHOOK option.
> > >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHOOK} != ""
>

I don't see how this could possibly work reliably. OPT_GLOBAL isn't set for
all
builds, and I'm not sure it's set for this submake given how we set them up.


> > >> SUBDIR+= h_ertt
> > >> .endif
> > >>
> > > It seems modules are actually broken for some configurations.
> > Some problems are known and being worked on.
> >
> > Could you share your kernel conf file and tell us, in which directory
> > you are running the make command?
>
> My config is attached.  I do 'make -C sys/amd64/compile/X' for this
> specific
> run.
>

I believe this is due to the interface between the kernel and the modules
changing
based on kernel config, which is something we've traditionally avoided.
Removing the
#ifdef for the 'osd' member in tcp_var.h

git diff
diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h
index 587998331fbf..f6d3e2b9fa85 100644
--- a/sys/netinet/tcp_var.h
+++ b/sys/netinet/tcp_var.h
@@ -482,9 +482,7 @@ struct tcpcb {
        struct mbufq t_inpkts;          /* List of saved input packets. */
        struct mbufq t_outpkts;         /* List of saved output packets. */
 #endif
-#ifdef TCP_HHOOK
        struct osd      t_osd;          /* storage for Khelp module data */
-#endif
        uint8_t _t_logpoint;    /* Used when a BB log points is enabled */
 #ifdef TCP_REQUEST_TRK
        /* Response tracking addons. */

fixes the problem, at least as far as building goes. The t_osd member
is trivial in size, and not worth the savings that having different KBIs
based on options causes (though there's other ifdefs that likely should
be rethought, and t_osd likely should be moved before all the optional
bits).

Warner