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>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] 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; > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 18, 2023 at 3:07 PM Mitchell Horne <<a href="mailto:mhorne@freebsd.org">mhorne@freebsd.org</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"><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="https://cgit.FreeBSD.org/src/commit/?id=b75062f23431fbabef1e7d665cae270b144f71b1" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=b75062f23431fbabef1e7d665cae270b144f71b1</a><br>; > <br> > commit b75062f23431fbabef1e7d665cae270b144f71b1<br> > Author: Brooks Davis <brooks@FreeBSD.org><br> > AuthorDate: 2023-01-17 16:36:15 +0000<br> > Commit: Brooks Davis <brooks@FreeBSD.org><br> > CommitDate: 2023-01-17 16:37:42 +0000<br> > <br> > riscv: Fix thread0.td_kstack_pages init<br> > <br> > Commit 0ef3ca7ae37c70e9dc83475dc2e68e98e1c2a418 initialized<br> > thread0.td_kstack_pages to KSTACK_PAGES. Due to the lack of an<br> > include of opt_kstack_pages.h it used the fallback value of 4 from<br> > 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 <br> 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 define _KERNEL becuse, well, they need to get to the kernel bits.... That will break...</div><div><br></div><div>I do agree, however, that the current interface is less than ideal...<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"> Of course, the problem at hand has been fixed and we want to keep direct <br> 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><br></div><div>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....<br></div><div><br></div><div>Warner<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"> Mitchell<br> <br> > This meant that increasing KSTACK_PAGES in the kernel<br> > config resulted in a panic in _epoch_enter_preempt as the following<br> > assertion was false during network stack setup:<br> > <br> > MPASS((vm_offset_t)et >= td->td_kstack &&<br> > (vm_offset_t)et + sizeof(struct epoch_tracker) <=<br> > td->td_kstack + td->td_kstack_pages * PAGE_SIZE);<br> > <br> > Switch to initializing with kstack_pages following other architectures.<br> > <br> > Reviewed by: imp, markj<br> > Sponsored by: DARPA, AFRL<br> > Differential Revision: <a href="https://reviews.freebsd.org/D38049" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D38049</a><br>; > ---<br> > sys/riscv/riscv/machdep.c | 2 +-<br> > 1 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> > <br> > proc_linkup0(&proc0, &thread0);<br> > thread0.td_kstack = kstack;<br> > - thread0.td_kstack_pages = KSTACK_PAGES;<br> > + thread0.td_kstack_pages = kstack_pages;<br> > thread0.td_pcb = (struct pcb *)(thread0.td_kstack +<br> > thread0.td_kstack_pages * PAGE_SIZE) - 1;<br> > thread0.td_pcb->pcb_fpflags = 0;<br> </blockquote></div></div>help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrMm0Kd46pfOYUaiAU%2BsMvYecU0whDH2jH0ck3C-Jjy0w>