From nobody Fri Jul 14 13:20:57 2023 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4R2XDD1qLcz4n7lW for ; Fri, 14 Jul 2023 13:21:00 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4R2XDC6Ghrz48kt for ; Fri, 14 Jul 2023 13:20:59 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-x52e.google.com with SMTP id 4fb4d7f45d1cf-51e2a6a3768so2333321a12.0 for ; Fri, 14 Jul 2023 06:20:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20221208.gappssmtp.com; s=20221208; t=1689340858; x=1691932858; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=zAtqmz43Z1rIelD4tKpmKNUZaHQUnPtlLpaiGPJQk60=; b=HpPqV7ZHoPK/cp39JbcXljhxHiZrXN5GbKl/wpddWU8fQKsyhpTK2UaT7WAOBvUFMO LJ2HJHpUB2DEe97ArNyrgwVtM3ERouVkVncTBK7BYGcaChpeYCKzEwFKI4K52hTu5O62 sMVpUIKlaYAZ+YnDJGs8+43Heyx/O/zGA40gmNhqN/U0L96bVpbqhgEJc/7Majg+qWQ1 jX4kxIMHeJPJCxz1OXjYmw66/o8xZDn8X7LxC0juokNIrhd+SJfmFeE4K37yPftxfzIH zRO/duDDSrZWaIdhu9MyyNnywMgNz5U9ULuidphJpI6YRoHok8jh2XkpXpCMJUi0jjOd L5vA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689340858; x=1691932858; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=zAtqmz43Z1rIelD4tKpmKNUZaHQUnPtlLpaiGPJQk60=; b=laTnd38Y79uWNhnaTKrh5mG7LFaf7Xk7iOt4SHCzToSAk+rRUe2QedRGTM13ik70j7 uiNExFl/GhwRbLziBuUUdzw4zGHuSaG19eFGvSH/9LHEsqO8M6rSTtQX69c9WOeJrnpK D238K19k4UpVUerv47GCaMiEblo5w21GTaYHYQT/2QtDKLuJNeVyFwHyqJa097CbcpMW mYbS0UWEGRHBDg4RVTM76+TU942r+xN2oeETBlMkJWQdUAzMozuDMv5CbGUS1r+xd0/0 MPZ9dL30WuNlC48sM2xXm9WhE3f+i7aaE/a7xwBAzC8FeweHic8bkJusxKP2HX4pg4gq Kqtg== X-Gm-Message-State: ABy/qLaI9b3lLEypWDZvJrCiIyaXFVF4hkgKdd27pMGmNlNrMcuKV5aG PfVtOm/jotcpL8sXMlXEC3JbgDEq9ZmkkneK2/zQ3A== X-Google-Smtp-Source: APBJJlGOLNHh0v1qkckd8Gey0p4sY6dGxTNAz+mpn11GaucCSRtDB6KOhxP26KqOyn6mYqBDAMi44JvSh5sTY4DJrIA= X-Received: by 2002:aa7:c50a:0:b0:51d:d227:15d0 with SMTP id o10-20020aa7c50a000000b0051dd22715d0mr4303030edq.0.1689340858383; Fri, 14 Jul 2023 06:20:58 -0700 (PDT) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 References: <202307132031.36DKVYIK019476@gitrepo.freebsd.org> <0C3C0CAD-523A-4A77-8942-4316DB49EF57@freebsd.org> In-Reply-To: From: Warner Losh Date: Fri, 14 Jul 2023 07:20:57 -0600 Message-ID: Subject: Re: git: be78a31188c5 - main - tcp: fix build issue for some cc modules To: Konstantin Belousov Cc: tuexen@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="000000000000c4d4d00600725192" X-Rspamd-Queue-Id: 4R2XDC6Ghrz48kt X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --000000000000c4d4d00600725192 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Jul 14, 2023 at 5:51=E2=80=AFAM Konstantin Belousov wrote: > On Fri, Jul 14, 2023 at 01:28:54PM +0200, tuexen@freebsd.org wrote: > > > On 14. Jul 2023, at 13:23, Konstantin Belousov > 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=3Dbe78a31188c530c93700396ecfdb560= 4a8f22fff > > >> > > >> commit be78a31188c530c93700396ecfdb5604a8f22fff > > >> Author: Michael Tuexen > > >> AuthorDate: 2023-07-13 16:56:25 +0000 > > >> Commit: Michael Tuexen > > >> 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=3De68b3792440cac248347afe08ba5881= a00ba6523 > > >> 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=3D 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} !=3D = "" > > >> +# 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} != =3D "" > > >> SUBDIR+=3D \ > > >> 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=3D > > >> > > >> # 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 defin= ed > > >> -# and does not contain the TCP_HHOOK option. > > >> -.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK} !=3D = "" > > >> +# We will default to building this module if $OPT_GLOBAL does conta= in > > >> +# the TCP_HHOOK option. > > >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHOOK} != =3D "" > 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+=3D 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 --000000000000c4d4d00600725192 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Jul 14, 2023 at 5:51=E2=80=AF= 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> wrot= e:
> >
> > 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=3Dbe78a31188c530c93700396ecfdb5604= a8f22fff
> >>
> >> commit be78a31188c530c93700396ecfdb5604a8f22fff
> >> Author:=C2=A0 =C2=A0 =C2=A0Michael Tuexen <tuexen@FreeBSD.= org>
> >> AuthorDate: 2023-07-13 16:56:25 +0000
> >> Commit:=C2=A0 =C2=A0 =C2=A0Michael Tuexen <tuexen@FreeBSD.= org>
> >> CommitDate: 2023-07-13 16:56:25 +0000
> >>
> >>=C2=A0 =C2=A0tcp: fix build issue for some cc modules
> >>
> >>=C2=A0 =C2=A0The TCP_HHOOK option was moved from opt_inet.h to= opt_global.h in
> >>=C2=A0 =C2=A0https://cgit.FreeBSD.org/src/commit/?id=3De68b3792440cac248347afe08= ba5881a00ba6523
> >>=C2=A0 =C2=A0The corresponding changes in two Makefiles were m= issed, which resulted
> >>=C2=A0 =C2=A0in not building cc_cdg, cc_chd, cc_hd, and cc_veg= as anymore.
> >>
> >>=C2=A0 =C2=A0Reported by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 void@f-m.fm
> >>=C2=A0 =C2=A0Reviewed by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 rrs, rscheff
> >>=C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0Netflix, Inc.
> >>=C2=A0 =C2=A0Differential Revision:=C2=A0 https://rev= iews.freebsd.org/D41010
> >> ---
> >> sys/modules/cc/Makefile=C2=A0 =C2=A0 | 6 +++---
> >> sys/modules/khelp/Makefile | 6 +++---
> >> 2 files changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/sys/modules/cc/Makefile b/sys/modules/cc/Makefil= e
> >> index 3f7110024722..b595cc204481 100644
> >> --- a/sys/modules/cc/Makefile
> >> +++ b/sys/modules/cc/Makefile
> >> @@ -8,9 +8,9 @@ SUBDIR=3D cc_newreno \
> >>
> >> # Do we have the TCP_HHOOK symbol defined? If not, there is n= o 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= } !=3D ""
> >> +# We will default to building these modules if $OPT_GLOBAL d= oes contain
> >> +# the TCP_HHOOK option.
> >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHO= OK} !=3D ""
> >> SUBDIR+=3D \
> >> cc_cdg \
> >> cc_chd \
> >> diff --git a/sys/modules/khelp/Makefile b/sys/modules/khelp/M= akefile
> >> index 256d8838c573..c01d61541062 100644
> >> --- a/sys/modules/khelp/Makefile
> >> +++ b/sys/modules/khelp/Makefile
> >> @@ -4,9 +4,9 @@ SUBDIR=3D
> >>
> >> # Do we have the TCP_HHOOK symbol defined? If not, there is n= o point in
> >> # building this modules by default.
> >> -# We will default to building this module unless $OPT_INET i= s defined
> >> -# and does not contain the TCP_HHOOK option.
> >> -.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK= } !=3D ""
> >> +# We will default to building this module if $OPT_GLOBAL doe= s contain
> >> +# the TCP_HHOOK option.
> >> +.if defined(ALL_MODULES) || ${OPT_GLOBAL:UTCP_HHOOK:MTCP_HHO= OK} !=3D ""

I don't see h= ow 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.
=C2=A0
> >> SUBDIR+=3D 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<= br> > you are running the make command?

My config is attached.=C2=A0 I do 'make -C sys/amd64/compile/X' for= this specific
run.

I believe this is due to the inter= face 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 {
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct mbufq t_inpkts; =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/* List of saved input packets. */
=C2=A0 =C2=A0 =C2=A0 =C2=A0= struct mbufq t_outpkts; =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* List of saved outpu= t packets. */
=C2=A0#endif
-#ifdef TCP_HHOOK
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 struct osd =C2=A0 =C2=A0 =C2=A0t_osd; =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* storage for Khelp module data */
-#endif
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 uint8_t _t_logpoint; =C2=A0 =C2=A0/* Used when a BB log points i= s enabled */
=C2=A0#ifdef TCP_REQUEST_TRK
=C2=A0 =C2=A0 =C2=A0 =C2=A0= /* Response tracking addons. */

fixes the pro= blem, at least as far as building goes. The t_osd member
is trivi= al 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 op= tional
bits).

Warner
--000000000000c4d4d00600725192--