From nobody Wed Jan 18 23:53:51 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 4Ny2dx252bz2v6WP for ; Wed, 18 Jan 2023 23:53:41 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) (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 4Ny2dx0Wcmz3Clc for ; Wed, 18 Jan 2023 23:53:41 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ej1-x631.google.com with SMTP id mp20so1417760ejc.7 for ; Wed, 18 Jan 2023 15:53:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=J1NDRvETvwLioi3nQN1oKSQuoOaUSYkmvq1RsNR8k+A=; b=zIyH+WzKoS8il5G8qyGnXIxfog+jKYnHgYxefwiWTEPS4pjkX2Ccn33hjkEuiNSmI3 zq9l+HeEkOUVT9gbK6r0jmvkqU7dHw44QoYG/b+ufXqDuJNic27FQz6P9xuKD4JnsluY TQY1F8o7l5iBV4UVlyx7NYjaeylvC7VPV1VXSlj5IJd8HgGgLl4rgS5vi6uP8yFdtItV +77Ri3MJmtBhE+5IKdM/gb2q7t8Vm4dzy9RDeceDE7oHprDWeouoVq/wAOPyxmUN11nK diw5z7rBxHWpEnRXgG9CEjwZxvDUIZxbLtTQKwEt7UNhPxybLX1iwJ5TC85MMMdzBV7X +quw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=J1NDRvETvwLioi3nQN1oKSQuoOaUSYkmvq1RsNR8k+A=; b=4PtkofFGC39XHSxMmdrNbWsuuCm/5SUTRzqlFF6sdHgrEl3CWbpbuwlg6iP347qJhP /StF99fLvbdET1F6XjM2zib6IQeKDL2p2ZvCL3AM96i0/DXS4JzbtS71xcpR7iu1K3lo MhPd8Cil7p7QU+yNgGQFpvQZCv+cUJTktXcJCwpyeesu66UtQ+pWFyfa4X0v5iCyozbj +CNW77lMkprSFYNHc7CREyFBaAI1qW/+dYLydwTJRmhgd7Sayv94PHKIW3Ememo+mqGN AIQ4/QoeiALmJqaU/tMqiFmR9Bb43IVtLFhABw16DXy25xo0fNS4LyFUEK1y9JmO/gGP I7hw== X-Gm-Message-State: AFqh2kq9Y/0WbhirzSjjghSH3lmKCIB24U1is1SFXVD0gOuI3SZJQamt IDCC9wbJQMGjVynTeHAyVXg4SDI2fHoTqryY2EArzg== X-Google-Smtp-Source: AMrXdXtn/Gmgg/qEecyfteKzkSzUDnE4xd8HV+SnnT/dEls+roXOGRe9yBS6fT1U4651a+Tg5KJOmX1OcYCzNY71sXA= X-Received: by 2002:a17:906:a3c3:b0:870:7c88:466e with SMTP id ca3-20020a170906a3c300b008707c88466emr600413ejb.140.1674086019751; Wed, 18 Jan 2023 15:53:39 -0800 (PST) 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: <202301171638.30HGcP3C091184@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Wed, 18 Jan 2023 16:53:51 -0700 Message-ID: Subject: Re: git: b75062f23431 - main - riscv: Fix thread0.td_kstack_pages init To: Mitchell Horne Cc: Brooks Davis , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="00000000000087f81c05f292864d" X-Rspamd-Queue-Id: 4Ny2dx0Wcmz3Clc 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 --00000000000087f81c05f292864d Content-Type: text/plain; charset="UTF-8" On Wed, Jan 18, 2023 at 3:07 PM Mitchell Horne wrote: > > > On 1/17/23 12:38, Brooks Davis wrote: > > The branch main has been updated by brooks: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=b75062f23431fbabef1e7d665cae270b144f71b1 > > > > commit b75062f23431fbabef1e7d665cae270b144f71b1 > > Author: Brooks Davis > > AuthorDate: 2023-01-17 16:36:15 +0000 > > Commit: Brooks Davis > > CommitDate: 2023-01-17 16:37:42 +0000 > > > > riscv: Fix thread0.td_kstack_pages init > > > > Commit 0ef3ca7ae37c70e9dc83475dc2e68e98e1c2a418 initialized > > thread0.td_kstack_pages to KSTACK_PAGES. Due to the lack of an > > include of opt_kstack_pages.h it used the fallback value of 4 from > > machine/param.h. > > Does this mean that we could/should include opt_kstack_pages.h within > machine/param.h (under #ifdef _KERNEL)? This header is both a consumer > and provider of the KSTACK_PAGES definition, by virtue of the #ifndef. I > think the hidden dependency should be avoided, if possible. > No. Including opt_XXXX.h is never OK in our .h files. They are used in too many places, some of which "cheat" and define _KERNEL becuse, well, they need to get to the kernel bits.... That will break... I do agree, however, that the current interface is less than ideal... > Of course, the problem at hand has been fixed and we want to keep direct > consumers of KSTACK_PAGES to a minimum, but I think the point still stands. > I think it's a good point, but the current way is likely the least-bad way to accomplish things. It would be much better if we could remove it from machine/param.h and opt_XXX.h always defines it, even the default value when it's not otherwise specified. However, we don't (currently) have a way to set default values in config(8). We could add it, since the efforts at config++ have thus far fallen flat.... Warner > Mitchell > > > This meant that increasing KSTACK_PAGES in the kernel > > config resulted in a panic in _epoch_enter_preempt as the following > > assertion was false during network stack setup: > > > > MPASS((vm_offset_t)et >= td->td_kstack && > > (vm_offset_t)et + sizeof(struct epoch_tracker) <= > > td->td_kstack + td->td_kstack_pages * PAGE_SIZE); > > > > Switch to initializing with kstack_pages following other > architectures. > > > > Reviewed by: imp, markj > > Sponsored by: DARPA, AFRL > > Differential Revision: https://reviews.freebsd.org/D38049 > > --- > > sys/riscv/riscv/machdep.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c > > index b03d45b018ec..0821a29d11c1 100644 > > --- a/sys/riscv/riscv/machdep.c > > +++ b/sys/riscv/riscv/machdep.c > > @@ -291,7 +291,7 @@ init_proc0(vm_offset_t kstack) > > > > proc_linkup0(&proc0, &thread0); > > thread0.td_kstack = kstack; > > - thread0.td_kstack_pages = KSTACK_PAGES; > > + thread0.td_kstack_pages = kstack_pages; > > thread0.td_pcb = (struct pcb *)(thread0.td_kstack + > > thread0.td_kstack_pages * PAGE_SIZE) - 1; > > thread0.td_pcb->pcb_fpflags = 0; > --00000000000087f81c05f292864d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Jan 18, 2023 at 3:07 PM Mitch= ell Horne <mhorne@freebsd.org&= gt; wrote:


On 1/17/23 12:38, Brooks Davis wrote:
> The branch main has been updated by brooks:
>
> URL: https://= cgit.FreeBSD.org/src/commit/?id=3Db75062f23431fbabef1e7d665cae270b144f71b1<= /a>
>
> commit b75062f23431fbabef1e7d665cae270b144f71b1
> Author:=C2=A0 =C2=A0 =C2=A0Brooks Davis <brooks@FreeBSD.org>
> AuthorDate: 2023-01-17 16:36:15 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Brooks Davis <brooks@FreeBSD.org>
> CommitDate: 2023-01-17 16:37:42 +0000
>
>=C2=A0 =C2=A0 =C2=A0 riscv: Fix thread0.td_kstack_pages init
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 Commit 0ef3ca7ae37c70e9dc83475dc2e68e98e1c2a418 in= itialized
>=C2=A0 =C2=A0 =C2=A0 thread0.td_kstack_pages to KSTACK_PAGES.=C2=A0 Due= to the lack of an
>=C2=A0 =C2=A0 =C2=A0 include of opt_kstack_pages.h it used the fallback= value of 4 from
>=C2=A0 =C2=A0 =C2=A0 machine/param.h.

Does this mean that we could/should include opt_kstack_pages.h within
machine/param.h (under #ifdef _KERNEL)? This header is both a consumer
and provider of the KSTACK_PAGES definition, by virtue of the #ifndef. I think the hidden dependency should be avoided, if possible.

No. Including opt_XXXX.h is never OK in our .h files. = They are used in too many places, some of which "cheat" and defin= e _KERNEL becuse, well, they need to get to the kernel bits....=C2=A0 That = will break...

I do agree, however, that the curren= t interface is less than ideal...
=C2=A0
Of course, the problem at hand has been fixed and we want to keep direct consumers of KSTACK_PAGES to a minimum, but I think the point still stands.=

I think it's a good point, but the= current way is likely the least-bad way to accomplish things.
It would be much better if we could remove it from machine/par= am.h and opt_XXX.h always defines it, even the default value when it's = not otherwise specified. However, we don't (currently) have a way to se= t default values in config(8). We could add it, since the efforts at config= ++ have thus far fallen flat....

Warner
=C2=A0
Mitchell

>=C2=A0 =C2=A0 =C2=A0 This meant that increasing KSTACK_PAGES in the ker= nel
>=C2=A0 =C2=A0 =C2=A0 config resulted in a panic in _epoch_enter_preempt= as the following
>=C2=A0 =C2=A0 =C2=A0 assertion was false during network stack setup: >=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MPASS((vm_offset_t)et = >=3D td->td_kstack &&
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (vm_offs= et_t)et + sizeof(struct epoch_tracker) <=3D
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 td->t= d_kstack + td->td_kstack_pages * PAGE_SIZE);
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 Switch to initializing with kstack_pages following= other architectures.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 Reviewed by:=C2=A0 =C2=A0 imp, markj
>=C2=A0 =C2=A0 =C2=A0 Sponsored by:=C2=A0 =C2=A0DARPA, AFRL
>=C2=A0 =C2=A0 =C2=A0 Differential Revision:=C2=A0
https://revi= ews.freebsd.org/D38049
> ---
>=C2=A0 =C2=A0sys/riscv/riscv/machdep.c | 2 +-
>=C2=A0 =C2=A01 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c
> index b03d45b018ec..0821a29d11c1 100644
> --- a/sys/riscv/riscv/machdep.c
> +++ b/sys/riscv/riscv/machdep.c
> @@ -291,7 +291,7 @@ init_proc0(vm_offset_t kstack)
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0proc_linkup0(&proc0, &thread0);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_kstack =3D kstack;
> -=C2=A0 =C2=A0 =C2=A0thread0.td_kstack_pages =3D KSTACK_PAGES;
> +=C2=A0 =C2=A0 =C2=A0thread0.td_kstack_pages =3D kstack_pages;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_pcb =3D (struct pcb *)(thread0.td= _kstack +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_kstack_pages * PAGE= _SIZE) - 1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_pcb->pcb_fpflags =3D 0;
--00000000000087f81c05f292864d--