Date: Fri, 14 Jul 2023 07:20:57 -0600 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: tuexen@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: be78a31188c5 - main - tcp: fix build issue for some cc modules Message-ID: <CANCZdfrhoRZJDV74u-9ziLFLMZ90vu41fyhL5P7GsboS9UrotA@mail.gmail.com> In-Reply-To: <ZLE2oRW3hEM5R4fv@kib.kiev.ua> References: <202307132031.36DKVYIK019476@gitrepo.freebsd.org> <ZLEwLRprzR0gSBmH@kib.kiev.ua> <0C3C0CAD-523A-4A77-8942-4316DB49EF57@freebsd.org> <ZLE2oRW3hEM5R4fv@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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 [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 14, 2023 at 5:51 AM Konstantin Belousov <<a href="mailto:kostikbel@gmail.com">kostikbel@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Jul 14, 2023 at 01:28:54PM +0200, <a href="mailto:tuexen@freebsd.org" target="_blank">tuexen@freebsd.org</a> wrote:<br> > > On 14. Jul 2023, at 13:23, Konstantin Belousov <<a href="mailto:kostikbel@gmail.com" target="_blank">kostikbel@gmail.com</a>> wrote:<br> > > <br> > > On Thu, Jul 13, 2023 at 08:31:34PM +0000, Michael Tuexen wrote:<br> > >> The branch main has been updated by tuexen:<br> > >> <br> > >> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=be78a31188c530c93700396ecfdb5604a8f22fff" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=be78a31188c530c93700396ecfdb5604a8f22fff</a><br>; > >> <br> > >> commit be78a31188c530c93700396ecfdb5604a8f22fff<br> > >> Author: Michael Tuexen <tuexen@FreeBSD.org><br> > >> AuthorDate: 2023-07-13 16:56:25 +0000<br> > >> Commit: Michael Tuexen <tuexen@FreeBSD.org><br> > >> CommitDate: 2023-07-13 16:56:25 +0000<br> > >> <br> > >> tcp: fix build issue for some cc modules<br> > >> <br> > >> The TCP_HHOOK option was moved from opt_inet.h to opt_global.h in<br> > >> <a href="https://cgit.FreeBSD.org/src/commit/?id=e68b3792440cac248347afe08ba5881a00ba6523" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=e68b3792440cac248347afe08ba5881a00ba6523</a><br>; > >> The corresponding changes in two Makefiles were missed, which resulted<br> > >> in not building cc_cdg, cc_chd, cc_hd, and cc_vegas anymore.<br> > >> <br> > >> Reported by: <a href="mailto:void@f-m.fm" target="_blank">void@f-m.fm</a><br> > >> Reviewed by: rrs, rscheff<br> > >> Sponsored by: Netflix, Inc.<br> > >> Differential Revision: <a href="https://reviews.freebsd.org/D41010" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D41010</a><br>; > >> ---<br> > >> sys/modules/cc/Makefile | 6 +++---<br> > >> sys/modules/khelp/Makefile | 6 +++---<br> > >> 2 files changed, 6 insertions(+), 6 deletions(-)<br> > >> <br> > >> diff --git a/sys/modules/cc/Makefile b/sys/modules/cc/Makefile<br> > >> index 3f7110024722..b595cc204481 100644<br> > >> --- a/sys/modules/cc/Makefile<br> > >> +++ b/sys/modules/cc/Makefile<br> > >> @@ -8,9 +8,9 @@ SUBDIR= cc_newreno \<br> > >> <br> > >> # Do we have the TCP_HHOOK symbol defined? If not, there is no point in<br> > >> # building these modules by default.<br> > >> -# We will default to building these modules unless $OPT_INET is defined<br> > >> -# and does not contain the TCP_HHOOK option.<br> > >> -.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK} != ""<br> > >> +# We will default to building these modules if $OPT_GLOBAL does contain<br> > >> +# the TCP_HHOOK option.<br> > >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHOOK} != ""<br> > >> SUBDIR+= \<br> > >> cc_cdg \<br> > >> cc_chd \<br> > >> diff --git a/sys/modules/khelp/Makefile b/sys/modules/khelp/Makefile<br> > >> index 256d8838c573..c01d61541062 100644<br> > >> --- a/sys/modules/khelp/Makefile<br> > >> +++ b/sys/modules/khelp/Makefile<br> > >> @@ -4,9 +4,9 @@ SUBDIR=<br> > >> <br> > >> # Do we have the TCP_HHOOK symbol defined? If not, there is no point in<br> > >> # building this modules by default.<br> > >> -# We will default to building this module unless $OPT_INET is defined<br> > >> -# and does not contain the TCP_HHOOK option.<br> > >> -.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK} != ""<br> > >> +# We will default to building this module if $OPT_GLOBAL does contain<br> > >> +# the TCP_HHOOK option.<br> > >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHOOK} != ""<br></blockquote><div><br></div><div>I don't see how this could possibly work reliably. OPT_GLOBAL isn't set for all</div><div>builds, and I'm not sure it's set for this submake given how we set them up.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > >> SUBDIR+= h_ertt<br> > >> .endif<br> > >> <br> > > It seems modules are actually broken for some configurations.<br> > Some problems are known and being worked on.<br> > <br> > Could you share your kernel conf file and tell us, in which directory<br> > you are running the make command?<br> <br> My config is attached. I do 'make -C sys/amd64/compile/X' for this specific<br> run.<br></blockquote><div><br></div><div>I believe this is due to the interface between the kernel and the modules changing</div><div>based on kernel config, which is something we've traditionally avoided. Removing the</div><div>#ifdef for the 'osd' member in tcp_var.h</div><div><br></div><div>git diff<br>diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h<br>index 587998331fbf..f6d3e2b9fa85 100644<br>--- a/sys/netinet/tcp_var.h<br>+++ b/sys/netinet/tcp_var.h<br>@@ -482,9 +482,7 @@ struct tcpcb {<br> struct mbufq t_inpkts; /* List of saved input packets. */<br> struct mbufq t_outpkts; /* List of saved output packets. */<br> #endif<br>-#ifdef TCP_HHOOK<br> struct osd t_osd; /* storage for Khelp module data */<br>-#endif<br> uint8_t _t_logpoint; /* Used when a BB log points is enabled */<br> #ifdef TCP_REQUEST_TRK<br> /* Response tracking addons. */<br></div><div><br></div><div>fixes the problem, at least as far as building goes. The t_osd member</div><div>is trivial in size, and not worth the savings that having different KBIs</div><div>based on options causes (though there's other ifdefs that likely should</div><div>be rethought, and t_osd likely should be moved before all the optional</div><div>bits).</div><div><br></div><div>Warner<br></div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrhoRZJDV74u-9ziLFLMZ90vu41fyhL5P7GsboS9UrotA>