Date: Wed, 18 Jan 2023 16:53:51 -0700 From: Warner Losh <imp@bsdimp.com> To: Mitchell Horne <mhorne@freebsd.org> Cc: Brooks Davis <brooks@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: b75062f23431 - main - riscv: Fix thread0.td_kstack_pages init Message-ID: <CANCZdfrMm0Kd46pfOYUaiAU%2BsMvYecU0whDH2jH0ck3C-Jjy0w@mail.gmail.com> In-Reply-To: <b2a60e9a-c918-5a88-5eb9-5d8322ca2326@freebsd.org> References: <202301171638.30HGcP3C091184@gitrepo.freebsd.org> <b2a60e9a-c918-5a88-5eb9-5d8322ca2326@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--00000000000087f81c05f292864d Content-Type: text/plain; charset="UTF-8" On Wed, Jan 18, 2023 at 3:07 PM Mitchell Horne <mhorne@freebsd.org> 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 <brooks@FreeBSD.org> > > AuthorDate: 2023-01-17 16:36:15 +0000 > > Commit: Brooks Davis <brooks@FreeBSD.org> > > 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 <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Wed, Jan 18, 2023 at 3:07 PM Mitch= ell Horne <<a href=3D"mailto:mhorne@freebsd.org">mhorne@freebsd.org</a>&= gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0= px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> <br> On 1/17/23 12:38, Brooks Davis wrote:<br> > The branch main has been updated by brooks:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Db75062f23431= fbabef1e7d665cae270b144f71b1" rel=3D"noreferrer" target=3D"_blank">https://= cgit.FreeBSD.org/src/commit/?id=3Db75062f23431fbabef1e7d665cae270b144f71b1<= /a><br> > <br> > commit b75062f23431fbabef1e7d665cae270b144f71b1<br> > Author:=C2=A0 =C2=A0 =C2=A0Brooks Davis <brooks@FreeBSD.org><br> > AuthorDate: 2023-01-17 16:36:15 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Brooks Davis <brooks@FreeBSD.org><br> > CommitDate: 2023-01-17 16:37:42 +0000<br> > <br> >=C2=A0 =C2=A0 =C2=A0 riscv: Fix thread0.td_kstack_pages init<br> >=C2=A0 =C2=A0 =C2=A0 <br> >=C2=A0 =C2=A0 =C2=A0 Commit 0ef3ca7ae37c70e9dc83475dc2e68e98e1c2a418 in= itialized<br> >=C2=A0 =C2=A0 =C2=A0 thread0.td_kstack_pages to KSTACK_PAGES.=C2=A0 Due= to the lack of an<br> >=C2=A0 =C2=A0 =C2=A0 include of opt_kstack_pages.h it used the fallback= value of 4 from<br> >=C2=A0 =C2=A0 =C2=A0 machine/param.h. <br> <br> Does this mean that we could/should include opt_kstack_pages.h within <br> machine/param.h (under #ifdef _KERNEL)? This header is both a consumer <br> and provider of the KSTACK_PAGES definition, by virtue of the #ifndef. I <b= r> think the hidden dependency should be avoided, if possible.<br></blockquote= ><div><br></div><div>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...</div><div><br></div><div>I do agree, however, that the curren= t interface is less than ideal...<br></div><div>=C2=A0</div><blockquote cla= ss=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid = rgb(204,204,204);padding-left:1ex"> Of course, the problem at hand has been fixed and we want to keep direct <b= r> consumers of KSTACK_PAGES to a minimum, but I think the point still stands.= <br></blockquote><div><br></div><div>I think it's a good point, but the= current way is likely the least-bad way to accomplish things.</div><div><b= r></div><div>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....<br></div><div><br></div><div>Warner<br></d= iv><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0= px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Mitchell<br> <br> >=C2=A0 =C2=A0 =C2=A0 This meant that increasing KSTACK_PAGES in the ker= nel<br> >=C2=A0 =C2=A0 =C2=A0 config resulted in a panic in _epoch_enter_preempt= as the following<br> >=C2=A0 =C2=A0 =C2=A0 assertion was false during network stack setup:<br= > >=C2=A0 =C2=A0 =C2=A0 <br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MPASS((vm_offset_t)et = >=3D td->td_kstack &&<br> >=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<br> >=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);<br> >=C2=A0 =C2=A0 =C2=A0 <br> >=C2=A0 =C2=A0 =C2=A0 Switch to initializing with kstack_pages following= other architectures.<br> >=C2=A0 =C2=A0 =C2=A0 <br> >=C2=A0 =C2=A0 =C2=A0 Reviewed by:=C2=A0 =C2=A0 imp, markj<br> >=C2=A0 =C2=A0 =C2=A0 Sponsored by:=C2=A0 =C2=A0DARPA, AFRL<br> >=C2=A0 =C2=A0 =C2=A0 Differential Revision:=C2=A0 <a href=3D"https://re= views.freebsd.org/D38049" rel=3D"noreferrer" target=3D"_blank">https://revi= ews.freebsd.org/D38049</a><br> > ---<br> >=C2=A0 =C2=A0sys/riscv/riscv/machdep.c | 2 +-<br> >=C2=A0 =C2=A01 file changed, 1 insertion(+), 1 deletion(-)<br> > <br> > diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c<br> > index b03d45b018ec..0821a29d11c1 100644<br> > --- a/sys/riscv/riscv/machdep.c<br> > +++ b/sys/riscv/riscv/machdep.c<br> > @@ -291,7 +291,7 @@ init_proc0(vm_offset_t kstack)<br> >=C2=A0 =C2=A0<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0proc_linkup0(&proc0, &thread0);<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_kstack =3D kstack;<br> > -=C2=A0 =C2=A0 =C2=A0thread0.td_kstack_pages =3D KSTACK_PAGES;<br> > +=C2=A0 =C2=A0 =C2=A0thread0.td_kstack_pages =3D kstack_pages;<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_pcb =3D (struct pcb *)(thread0.td= _kstack +<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_kstack_pages * PAGE= _SIZE) - 1;<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0thread0.td_pcb->pcb_fpflags =3D 0;<br> </blockquote></div></div> --00000000000087f81c05f292864d--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrMm0Kd46pfOYUaiAU%2BsMvYecU0whDH2jH0ck3C-Jjy0w>