Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Mon, Aug 21, 2023 at 10:34:53PM -0500, Scott Cheloha wrote: > > On Tue, Aug 22, 2023 at 11:28:25AM +0800, Kevin Lo wrote: > > On Mon, Aug 21, 2023 at 09:53:53PM -0500, Scott Cheloha wrote: > > > On Tue, Aug 22, 2023 at 02:36:31AM +, Mike Larkin wrote: > > > > On Mon, Aug 21, 2023 at 09:26:00PM -0500, Scott Cheloha wrote: > > > > > On Mon, Aug 21, 2023 at 10:10:58PM +, Mike Larkin wrote: > > > > > > > - alpha > > > > > > > - hppa > > > > > > > - m88k/luna88k > > > > > > > > > > > > if you are really interested in doing this [...] > > > > > > > > > > "really interested" is a bit strong. As always, my primary goal is > > > > > not to break anything when I make a commit. > > > > > > > > > > The luna88k patch looks pretty straightfoward, but it's hard to be > > > > > completely sure I didn't screw something up. > > > > > > > > > > > [...] you could run this in nono since you're just looking for > > > > > > a compile/boot test. > > > > > > > > > > Apparently the license forbids redistribution. Super annoying. > > > > > > > > > > > > > so? install it, boot a luna88k "vm", test your diff, then you have your > > > > question answered. you aren't redistributing anything. > > > > > > No, I mean that there is no binary for pkg_add, so I have to build it > > > by hand. Unless I'm missing something? > > > > Hi Scott, > > > > You could install emulators/nono, which is luna88k emulator. > > ??? > > I just tried that, it says there is no such thing. > > $ doas pkg_add nono > quirks-6.138 signed on 2023-08-20T20:51:44Z > Can't find nono > $ doas pkg_add emulators/nono > quirks-6.138 signed on 2023-08-20T20:51:44Z > file:emulators/: empty > Can't find nono > > What am I missing here? I'm really sorry, I'm stumped. Because of https://github.com/openbsd/ports/blob/master/emulators/nono/Makefile#L14, you need to install nono from the ports tree, thanks.
Re: link mbufs/inpcbs to pf_states, not pf_state_keys
Hello,
On Tue, Aug 22, 2023 at 12:21:59AM +0200, Alexander Bluhm wrote:
> On Thu, Aug 17, 2023 at 12:02:35PM +1000, David Gwynne wrote:
> > there are links between the pcb/socket layer and pf as an optimisation,
> > and links on mbufs between both sides of a forwarded connection.
> > these links let pf skip an rb tree lookup for outgoing packets.
> >
> > right now these links are between pf_state_key structs, which are the
> > things that contain the actual addresses used by the connection, but you
> > then have to iterate over a list in pf_state_keys to get to the pf_state
> > structures.
> >
> > i dont understand why we dont just link the actual pf_state structs.
> > my best guess is there wasnt enough machinery (ie, refcnts and
> > mtxes) on a pf_state struct to make it safe, so the compromise was
> > the pf_state keys. it still got to avoid the tree lookup.
>
> This linkage is much older than MP in pf. There was no refcount
> and mutex when added by henning@.
>
> > i wanted this to make it easier to look up information on pf states from
> > the socket layer, but sashan@ said i should send it out. i do think it
> > makes things a bit easier to understand.
> >
> > the most worrying bit is the change to pf_state_find().
> >
> > thoughts? ok?
>
> It took me years to get the logic correct for all corner cases.
> When using pf-divert with UDP strange things start to happen. Most
> things are covered by regress/sys/net/pf_divert . You have to setup
> two machines to run it.
I'm running the diff on my home router which is doing simple IPv4 NAT,
nothing fancy. I have not noticed any problem so far. Uptime is ~3 days.
I have not used pf_divert tests.
>
> I don't know why it was written that way, but I know it works for
> me. What do you want to fix?
>
>
I've seen the diff off-list as a part of another change. I did ask
David to post this part (the diff) to tech@ separately because I like it.
In my opinion it makes things bit more straightforward. Things
just look more pretty with this diff in. Currently we have
something like this:
{ mbuf, pcb } <-> state key <-> { state, state ... }
with this diff we get to:
{ mbuf, pcb } <-> state <-> state key
Basically when we do process packet we are interested in state
not state key itself. The state key is just a mean to reach the state,
so why not to reach/keep state directly. I like the simplicity
introduced by proposed diff.
I support this change. I also prefer to reach wide consent on this
before the diff will arrive to tree.
thanks and
regards
sashan
Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Tue, Aug 22, 2023 at 11:28:25AM +0800, Kevin Lo wrote: > On Mon, Aug 21, 2023 at 09:53:53PM -0500, Scott Cheloha wrote: > > On Tue, Aug 22, 2023 at 02:36:31AM +, Mike Larkin wrote: > > > On Mon, Aug 21, 2023 at 09:26:00PM -0500, Scott Cheloha wrote: > > > > On Mon, Aug 21, 2023 at 10:10:58PM +, Mike Larkin wrote: > > > > > > - alpha > > > > > > - hppa > > > > > > - m88k/luna88k > > > > > > > > > > if you are really interested in doing this [...] > > > > > > > > "really interested" is a bit strong. As always, my primary goal is > > > > not to break anything when I make a commit. > > > > > > > > The luna88k patch looks pretty straightfoward, but it's hard to be > > > > completely sure I didn't screw something up. > > > > > > > > > [...] you could run this in nono since you're just looking for > > > > > a compile/boot test. > > > > > > > > Apparently the license forbids redistribution. Super annoying. > > > > > > > > > > so? install it, boot a luna88k "vm", test your diff, then you have your > > > question answered. you aren't redistributing anything. > > > > No, I mean that there is no binary for pkg_add, so I have to build it > > by hand. Unless I'm missing something? > > Hi Scott, > > You could install emulators/nono, which is luna88k emulator. ??? I just tried that, it says there is no such thing. $ doas pkg_add nono quirks-6.138 signed on 2023-08-20T20:51:44Z Can't find nono $ doas pkg_add emulators/nono quirks-6.138 signed on 2023-08-20T20:51:44Z file:emulators/: empty Can't find nono What am I missing here? I'm really sorry, I'm stumped.
Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Tue, Aug 22, 2023 at 02:36:31AM +, Mike Larkin wrote: > On Mon, Aug 21, 2023 at 09:26:00PM -0500, Scott Cheloha wrote: > > On Mon, Aug 21, 2023 at 10:10:58PM +, Mike Larkin wrote: > > > On Sat, Aug 19, 2023 at 01:44:47PM -0500, Scott Cheloha wrote: > > > > On Sun, Aug 13, 2023 at 01:48:21PM -0500, Scott Cheloha wrote: > > > > > This is the next patch in the clock interrupt reorganization series. > > > > > > > > > > Before we continue breaking up the hardclock(9) we need to detour into > > > > > the MD code. > > > > > > > > > > This patch divides the "initialization" parts of cpu_initclocks() from > > > > > the "start the clock interrupt" parts. Seprating the two parts leaves > > > > > initclocks() an opportunity to prepare the primary CPU for clock > > > > > interrupt dispatch in a machine-independent manner before actually > > > > > pulling the trigger. It's nearly impossible to do any MI setup during > > > > > initclocks() because cpu_initclocks() does everything in one go: both > > > > > initialization and kickoff are done when cpu_initclocks() returns. > > > > > > > > > > Many platforms have a "cpu_startclock()" function, so this patch takes > > > > > that de facto standard and makes it a rule: cpu_startclock() is now > > > > > required. It is prototyped in sys/systm.h and every platform must > > > > > implement it. > > > > > > > > > > The revised initclocks() sequence is then: > > > > > > > > > > 1. Call cpu_initclocks(). At minimum, cpu_initclocks() ensures > > > > >hz, stathz, and profhz are initialized. All the machine > > > > >independent setup in step (2) (currently) depends upon > > > > >these machine-dependent values. > > > > > > > > > > 2. Compute intervals using hz, stathz, and profhz. > > > > > > > > > >In a later step I will move the full contents of clockintr_init() > > > > >up into initclocks() and get rid of clockintr_init() entirely. > > > > > > > > > > 3. Call cpu_startclock(). At minimum, cpu_startclock() starts the > > > > >clock interrupt dispatch cycle on the primary CPU. > > > > > > > > > > I have compiled/booted this patch on amd64 (lapic path), arm64, i386 > > > > > (lapic path), macppc, octeon, and sparc64 (sun4v). > > > > > > > > > > I am looking for compile/boot tests on alpha, armv7, hppa, landisk, > > > > > luna88k, powerpc64, and riscv64. I think armv7 is the tricky one > > > > > here. Everything else is relatively straightforward, though I may > > > > > have missed a few stray variables here or there. > > > > > > > > > > Test results? Ok? > > > > > > > > Here is an updated patch that removes several MD prototypes for > > > > cpu_startclock() that I missed the first time through. > > > > > > > > I went back and tested these again: > > > > > > > > - amd64 (lapic) > > > > - arm64 > > > > - i386 (lapic) > > > > - powerpc/macppc > > > > - mips64/octeon (loongson should be fine) > > > > - sparc64 (sys_tick; tick/stick should be fine) > > > > > > > > arm/armv7 and riscv64 were tested under the previous version, but I > > > > would appreciate a second compile-test to make sure the header changes > > > > in the updated patch did not break the build (CC phessler@, jsg@). > > > > > > > > I am still seeking compile/boot-tests for the following: > > > > > > > > - alpha > > > > - hppa > > > > - m88k/luna88k > > > > > > if you are really interested in doing this [...] > > > > "really interested" is a bit strong. As always, my primary goal is > > not to break anything when I make a commit. > > > > The luna88k patch looks pretty straightfoward, but it's hard to be > > completely sure I didn't screw something up. > > > > > [...] you could run this in nono since you're just looking for > > > a compile/boot test. > > > > Apparently the license forbids redistribution. Super annoying. > > so? install it, boot a luna88k "vm", test your diff, then you have your > question answered. you aren't redistributing anything. FWIW, I think vmctl/vmd have a nicer user interface. I feel like I'm... boxing... with nono, not using it.
Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Tue, Aug 22, 2023 at 02:36:31AM +, Mike Larkin wrote: > On Mon, Aug 21, 2023 at 09:26:00PM -0500, Scott Cheloha wrote: > > On Mon, Aug 21, 2023 at 10:10:58PM +, Mike Larkin wrote: > > > On Sat, Aug 19, 2023 at 01:44:47PM -0500, Scott Cheloha wrote: > > > > On Sun, Aug 13, 2023 at 01:48:21PM -0500, Scott Cheloha wrote: > > > > > This is the next patch in the clock interrupt reorganization series. > > > > > > > > > > Before we continue breaking up the hardclock(9) we need to detour into > > > > > the MD code. > > > > > > > > > > This patch divides the "initialization" parts of cpu_initclocks() from > > > > > the "start the clock interrupt" parts. Seprating the two parts leaves > > > > > initclocks() an opportunity to prepare the primary CPU for clock > > > > > interrupt dispatch in a machine-independent manner before actually > > > > > pulling the trigger. It's nearly impossible to do any MI setup during > > > > > initclocks() because cpu_initclocks() does everything in one go: both > > > > > initialization and kickoff are done when cpu_initclocks() returns. > > > > > > > > > > Many platforms have a "cpu_startclock()" function, so this patch takes > > > > > that de facto standard and makes it a rule: cpu_startclock() is now > > > > > required. It is prototyped in sys/systm.h and every platform must > > > > > implement it. > > > > > > > > > > The revised initclocks() sequence is then: > > > > > > > > > > 1. Call cpu_initclocks(). At minimum, cpu_initclocks() ensures > > > > >hz, stathz, and profhz are initialized. All the machine > > > > >independent setup in step (2) (currently) depends upon > > > > >these machine-dependent values. > > > > > > > > > > 2. Compute intervals using hz, stathz, and profhz. > > > > > > > > > >In a later step I will move the full contents of clockintr_init() > > > > >up into initclocks() and get rid of clockintr_init() entirely. > > > > > > > > > > 3. Call cpu_startclock(). At minimum, cpu_startclock() starts the > > > > >clock interrupt dispatch cycle on the primary CPU. > > > > > > > > > > I have compiled/booted this patch on amd64 (lapic path), arm64, i386 > > > > > (lapic path), macppc, octeon, and sparc64 (sun4v). > > > > > > > > > > I am looking for compile/boot tests on alpha, armv7, hppa, landisk, > > > > > luna88k, powerpc64, and riscv64. I think armv7 is the tricky one > > > > > here. Everything else is relatively straightforward, though I may > > > > > have missed a few stray variables here or there. > > > > > > > > > > Test results? Ok? > > > > > > > > Here is an updated patch that removes several MD prototypes for > > > > cpu_startclock() that I missed the first time through. > > > > > > > > I went back and tested these again: > > > > > > > > - amd64 (lapic) > > > > - arm64 > > > > - i386 (lapic) > > > > - powerpc/macppc > > > > - mips64/octeon (loongson should be fine) > > > > - sparc64 (sys_tick; tick/stick should be fine) > > > > > > > > arm/armv7 and riscv64 were tested under the previous version, but I > > > > would appreciate a second compile-test to make sure the header changes > > > > in the updated patch did not break the build (CC phessler@, jsg@). > > > > > > > > I am still seeking compile/boot-tests for the following: > > > > > > > > - alpha > > > > - hppa > > > > - m88k/luna88k > > > > > > if you are really interested in doing this [...] > > > > "really interested" is a bit strong. As always, my primary goal is > > not to break anything when I make a commit. > > > > The luna88k patch looks pretty straightfoward, but it's hard to be > > completely sure I didn't screw something up. > > > > > [...] you could run this in nono since you're just looking for > > > a compile/boot test. > > > > Apparently the license forbids redistribution. Super annoying. > > > > so? install it, boot a luna88k "vm", test your diff, then you have your > question answered. you aren't redistributing anything. No, I mean that there is no binary for pkg_add, so I have to build it by hand. Unless I'm missing something?
Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Mon, Aug 21, 2023 at 09:26:00PM -0500, Scott Cheloha wrote: > On Mon, Aug 21, 2023 at 10:10:58PM +, Mike Larkin wrote: > > On Sat, Aug 19, 2023 at 01:44:47PM -0500, Scott Cheloha wrote: > > > On Sun, Aug 13, 2023 at 01:48:21PM -0500, Scott Cheloha wrote: > > > > This is the next patch in the clock interrupt reorganization series. > > > > > > > > Before we continue breaking up the hardclock(9) we need to detour into > > > > the MD code. > > > > > > > > This patch divides the "initialization" parts of cpu_initclocks() from > > > > the "start the clock interrupt" parts. Seprating the two parts leaves > > > > initclocks() an opportunity to prepare the primary CPU for clock > > > > interrupt dispatch in a machine-independent manner before actually > > > > pulling the trigger. It's nearly impossible to do any MI setup during > > > > initclocks() because cpu_initclocks() does everything in one go: both > > > > initialization and kickoff are done when cpu_initclocks() returns. > > > > > > > > Many platforms have a "cpu_startclock()" function, so this patch takes > > > > that de facto standard and makes it a rule: cpu_startclock() is now > > > > required. It is prototyped in sys/systm.h and every platform must > > > > implement it. > > > > > > > > The revised initclocks() sequence is then: > > > > > > > > 1. Call cpu_initclocks(). At minimum, cpu_initclocks() ensures > > > >hz, stathz, and profhz are initialized. All the machine > > > >independent setup in step (2) (currently) depends upon > > > >these machine-dependent values. > > > > > > > > 2. Compute intervals using hz, stathz, and profhz. > > > > > > > >In a later step I will move the full contents of clockintr_init() > > > >up into initclocks() and get rid of clockintr_init() entirely. > > > > > > > > 3. Call cpu_startclock(). At minimum, cpu_startclock() starts the > > > >clock interrupt dispatch cycle on the primary CPU. > > > > > > > > I have compiled/booted this patch on amd64 (lapic path), arm64, i386 > > > > (lapic path), macppc, octeon, and sparc64 (sun4v). > > > > > > > > I am looking for compile/boot tests on alpha, armv7, hppa, landisk, > > > > luna88k, powerpc64, and riscv64. I think armv7 is the tricky one > > > > here. Everything else is relatively straightforward, though I may > > > > have missed a few stray variables here or there. > > > > > > > > Test results? Ok? > > > > > > Here is an updated patch that removes several MD prototypes for > > > cpu_startclock() that I missed the first time through. > > > > > > I went back and tested these again: > > > > > > - amd64 (lapic) > > > - arm64 > > > - i386 (lapic) > > > - powerpc/macppc > > > - mips64/octeon (loongson should be fine) > > > - sparc64 (sys_tick; tick/stick should be fine) > > > > > > arm/armv7 and riscv64 were tested under the previous version, but I > > > would appreciate a second compile-test to make sure the header changes > > > in the updated patch did not break the build (CC phessler@, jsg@). > > > > > > I am still seeking compile/boot-tests for the following: > > > > > > - alpha > > > - hppa > > > - m88k/luna88k > > > > if you are really interested in doing this [...] > > "really interested" is a bit strong. As always, my primary goal is > not to break anything when I make a commit. > > The luna88k patch looks pretty straightfoward, but it's hard to be > completely sure I didn't screw something up. > > > [...] you could run this in nono since you're just looking for > > a compile/boot test. > > Apparently the license forbids redistribution. Super annoying. > so? install it, boot a luna88k "vm", test your diff, then you have your question answered. you aren't redistributing anything. > > > - powerpc64 > > > > builds and boots on powerpc64 > > Noted. Thank you! >
Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Mon, Aug 21, 2023 at 10:10:58PM +, Mike Larkin wrote: > On Sat, Aug 19, 2023 at 01:44:47PM -0500, Scott Cheloha wrote: > > On Sun, Aug 13, 2023 at 01:48:21PM -0500, Scott Cheloha wrote: > > > This is the next patch in the clock interrupt reorganization series. > > > > > > Before we continue breaking up the hardclock(9) we need to detour into > > > the MD code. > > > > > > This patch divides the "initialization" parts of cpu_initclocks() from > > > the "start the clock interrupt" parts. Seprating the two parts leaves > > > initclocks() an opportunity to prepare the primary CPU for clock > > > interrupt dispatch in a machine-independent manner before actually > > > pulling the trigger. It's nearly impossible to do any MI setup during > > > initclocks() because cpu_initclocks() does everything in one go: both > > > initialization and kickoff are done when cpu_initclocks() returns. > > > > > > Many platforms have a "cpu_startclock()" function, so this patch takes > > > that de facto standard and makes it a rule: cpu_startclock() is now > > > required. It is prototyped in sys/systm.h and every platform must > > > implement it. > > > > > > The revised initclocks() sequence is then: > > > > > > 1. Call cpu_initclocks(). At minimum, cpu_initclocks() ensures > > >hz, stathz, and profhz are initialized. All the machine > > >independent setup in step (2) (currently) depends upon > > >these machine-dependent values. > > > > > > 2. Compute intervals using hz, stathz, and profhz. > > > > > >In a later step I will move the full contents of clockintr_init() > > >up into initclocks() and get rid of clockintr_init() entirely. > > > > > > 3. Call cpu_startclock(). At minimum, cpu_startclock() starts the > > >clock interrupt dispatch cycle on the primary CPU. > > > > > > I have compiled/booted this patch on amd64 (lapic path), arm64, i386 > > > (lapic path), macppc, octeon, and sparc64 (sun4v). > > > > > > I am looking for compile/boot tests on alpha, armv7, hppa, landisk, > > > luna88k, powerpc64, and riscv64. I think armv7 is the tricky one > > > here. Everything else is relatively straightforward, though I may > > > have missed a few stray variables here or there. > > > > > > Test results? Ok? > > > > Here is an updated patch that removes several MD prototypes for > > cpu_startclock() that I missed the first time through. > > > > I went back and tested these again: > > > > - amd64 (lapic) > > - arm64 > > - i386 (lapic) > > - powerpc/macppc > > - mips64/octeon (loongson should be fine) > > - sparc64 (sys_tick; tick/stick should be fine) > > > > arm/armv7 and riscv64 were tested under the previous version, but I > > would appreciate a second compile-test to make sure the header changes > > in the updated patch did not break the build (CC phessler@, jsg@). > > > > I am still seeking compile/boot-tests for the following: > > > > - alpha > > - hppa > > - m88k/luna88k > > if you are really interested in doing this [...] "really interested" is a bit strong. As always, my primary goal is not to break anything when I make a commit. The luna88k patch looks pretty straightfoward, but it's hard to be completely sure I didn't screw something up. > [...] you could run this in nono since you're just looking for > a compile/boot test. Apparently the license forbids redistribution. Super annoying. > > - powerpc64 > > builds and boots on powerpc64 Noted. Thank you!
Re: link mbufs/inpcbs to pf_states, not pf_state_keys
On Thu, Aug 17, 2023 at 12:02:35PM +1000, David Gwynne wrote:
> there are links between the pcb/socket layer and pf as an optimisation,
> and links on mbufs between both sides of a forwarded connection.
> these links let pf skip an rb tree lookup for outgoing packets.
>
> right now these links are between pf_state_key structs, which are the
> things that contain the actual addresses used by the connection, but you
> then have to iterate over a list in pf_state_keys to get to the pf_state
> structures.
>
> i dont understand why we dont just link the actual pf_state structs.
> my best guess is there wasnt enough machinery (ie, refcnts and
> mtxes) on a pf_state struct to make it safe, so the compromise was
> the pf_state keys. it still got to avoid the tree lookup.
This linkage is much older than MP in pf. There was no refcount
and mutex when added by henning@.
> i wanted this to make it easier to look up information on pf states from
> the socket layer, but sashan@ said i should send it out. i do think it
> makes things a bit easier to understand.
>
> the most worrying bit is the change to pf_state_find().
>
> thoughts? ok?
It took me years to get the logic correct for all corner cases.
When using pf-divert with UDP strange things start to happen. Most
things are covered by regress/sys/net/pf_divert . You have to setup
two machines to run it.
I don't know why it was written that way, but I know it works for
me. What do you want to fix?
bluhm
> Index: kern/uipc_mbuf.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.287
> diff -u -p -r1.287 uipc_mbuf.c
> --- kern/uipc_mbuf.c 23 Jun 2023 04:36:49 - 1.287
> +++ kern/uipc_mbuf.c 17 Aug 2023 01:31:04 -
> @@ -308,7 +308,7 @@ m_clearhdr(struct mbuf *m)
> /* delete all mbuf tags to reset the state */
> m_tag_delete_chain(m);
> #if NPF > 0
> - pf_mbuf_unlink_state_key(m);
> + pf_mbuf_unlink_state(m);
> pf_mbuf_unlink_inpcb(m);
> #endif /* NPF > 0 */
>
> @@ -440,7 +440,7 @@ m_free(struct mbuf *m)
> if (m->m_flags & M_PKTHDR) {
> m_tag_delete_chain(m);
> #if NPF > 0
> - pf_mbuf_unlink_state_key(m);
> + pf_mbuf_unlink_state(m);
> pf_mbuf_unlink_inpcb(m);
> #endif /* NPF > 0 */
> }
> @@ -1398,8 +1398,8 @@ m_dup_pkthdr(struct mbuf *to, struct mbu
> to->m_pkthdr = from->m_pkthdr;
>
> #if NPF > 0
> - to->m_pkthdr.pf.statekey = NULL;
> - pf_mbuf_link_state_key(to, from->m_pkthdr.pf.statekey);
> + to->m_pkthdr.pf.st = NULL;
> + pf_mbuf_link_state(to, from->m_pkthdr.pf.st);
> to->m_pkthdr.pf.inp = NULL;
> pf_mbuf_link_inpcb(to, from->m_pkthdr.pf.inp);
> #endif /* NPF > 0 */
> @@ -1526,8 +1526,8 @@ m_print(void *v,
> m->m_pkthdr.csum_flags, MCS_BITS);
> (*pr)("m_pkthdr.ether_vtag: %u\tm_ptkhdr.ph_rtableid: %u\n",
> m->m_pkthdr.ether_vtag, m->m_pkthdr.ph_rtableid);
> - (*pr)("m_pkthdr.pf.statekey: %p\tm_pkthdr.pf.inp %p\n",
> - m->m_pkthdr.pf.statekey, m->m_pkthdr.pf.inp);
> + (*pr)("m_pkthdr.pf.st: %p\tm_pkthdr.pf.inp %p\n",
> + m->m_pkthdr.pf.st, m->m_pkthdr.pf.inp);
> (*pr)("m_pkthdr.pf.qid: %u\tm_pkthdr.pf.tag: %u\n",
> m->m_pkthdr.pf.qid, m->m_pkthdr.pf.tag);
> (*pr)("m_pkthdr.pf.flags: %b\n",
> Index: net/if_mpw.c
> ===
> RCS file: /cvs/src/sys/net/if_mpw.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 if_mpw.c
> --- net/if_mpw.c 29 Aug 2022 07:51:45 - 1.63
> +++ net/if_mpw.c 17 Aug 2023 01:31:04 -
> @@ -620,7 +620,7 @@ mpw_input(struct mpw_softc *sc, struct m
> m->m_pkthdr.ph_rtableid = ifp->if_rdomain;
>
> /* packet has not been processed by PF yet. */
> - KASSERT(m->m_pkthdr.pf.statekey == NULL);
> + KASSERT(m->m_pkthdr.pf.st == NULL);
>
> if_vinput(ifp, m);
> return;
> Index: net/if_tpmr.c
> ===
> RCS file: /cvs/src/sys/net/if_tpmr.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 if_tpmr.c
> --- net/if_tpmr.c 16 May 2023 14:32:54 - 1.33
> +++ net/if_tpmr.c 17 Aug 2023 01:31:04 -
> @@ -303,7 +303,7 @@ tpmr_pf(struct ifnet *ifp0, int dir, str
> return (NULL);
>
> if (dir == PF_IN && ISSET(m->m_pkthdr.pf.flags, PF_TAG_DIVERTED)) {
> - pf_mbuf_unlink_state_key(m);
> + pf_mbuf_unlink_state(m);
> pf_mbuf_unlink_inpcb(m);
> (*fam->ip_input)(ifp0, m);
> return (NULL);
> Index: net/if_veb.c
> ===
> RCS file: /cvs/src/sys/net/if_veb.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 if_veb.c
> ---
Re: all platforms: separate cpu_initclocks() from cpu_startclock()
On Sat, Aug 19, 2023 at 01:44:47PM -0500, Scott Cheloha wrote:
> On Sun, Aug 13, 2023 at 01:48:21PM -0500, Scott Cheloha wrote:
> > This is the next patch in the clock interrupt reorganization series.
> >
> > Before we continue breaking up the hardclock(9) we need to detour into
> > the MD code.
> >
> > This patch divides the "initialization" parts of cpu_initclocks() from
> > the "start the clock interrupt" parts. Seprating the two parts leaves
> > initclocks() an opportunity to prepare the primary CPU for clock
> > interrupt dispatch in a machine-independent manner before actually
> > pulling the trigger. It's nearly impossible to do any MI setup during
> > initclocks() because cpu_initclocks() does everything in one go: both
> > initialization and kickoff are done when cpu_initclocks() returns.
> >
> > Many platforms have a "cpu_startclock()" function, so this patch takes
> > that de facto standard and makes it a rule: cpu_startclock() is now
> > required. It is prototyped in sys/systm.h and every platform must
> > implement it.
> >
> > The revised initclocks() sequence is then:
> >
> > 1. Call cpu_initclocks(). At minimum, cpu_initclocks() ensures
> >hz, stathz, and profhz are initialized. All the machine
> >independent setup in step (2) (currently) depends upon
> >these machine-dependent values.
> >
> > 2. Compute intervals using hz, stathz, and profhz.
> >
> >In a later step I will move the full contents of clockintr_init()
> >up into initclocks() and get rid of clockintr_init() entirely.
> >
> > 3. Call cpu_startclock(). At minimum, cpu_startclock() starts the
> >clock interrupt dispatch cycle on the primary CPU.
> >
> > I have compiled/booted this patch on amd64 (lapic path), arm64, i386
> > (lapic path), macppc, octeon, and sparc64 (sun4v).
> >
> > I am looking for compile/boot tests on alpha, armv7, hppa, landisk,
> > luna88k, powerpc64, and riscv64. I think armv7 is the tricky one
> > here. Everything else is relatively straightforward, though I may
> > have missed a few stray variables here or there.
> >
> > Test results? Ok?
>
> Here is an updated patch that removes several MD prototypes for
> cpu_startclock() that I missed the first time through.
>
> I went back and tested these again:
>
> - amd64 (lapic)
> - arm64
> - i386 (lapic)
> - powerpc/macppc
> - mips64/octeon (loongson should be fine)
> - sparc64 (sys_tick; tick/stick should be fine)
>
> arm/armv7 and riscv64 were tested under the previous version, but I
> would appreciate a second compile-test to make sure the header changes
> in the updated patch did not break the build (CC phessler@, jsg@).
>
> I am still seeking compile/boot-tests for the following:
>
> - alpha
> - hppa
> - m88k/luna88k
if you are really interested in doing this you could run this in nono since
you're just looking for a compile/boot test.
> - powerpc64
builds and boots on powerpc64
> - sh/landisk
>
> Test results? Ok?
>
> Index: kern/kern_clock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_clock.c,v
> retrieving revision 1.113
> diff -u -p -r1.113 kern_clock.c
> --- kern/kern_clock.c 12 Aug 2023 13:19:28 - 1.113
> +++ kern/kern_clock.c 19 Aug 2023 18:16:16 -
> @@ -103,6 +103,9 @@ initclocks(void)
> profclock_period = 10 / profhz;
>
> inittimecounter();
> +
> + /* Start dispatching clock interrupts on the primary CPU. */
> + cpu_startclock();
> }
>
> /*
> Index: sys/systm.h
> ===
> RCS file: /cvs/src/sys/sys/systm.h,v
> retrieving revision 1.164
> diff -u -p -r1.164 systm.h
> --- sys/systm.h 5 Aug 2023 20:07:56 - 1.164
> +++ sys/systm.h 19 Aug 2023 18:16:17 -
> @@ -243,6 +243,7 @@ void initclocks(void);
> void inittodr(time_t);
> void resettodr(void);
> void cpu_initclocks(void);
> +void cpu_startclock(void);
>
> void startprofclock(struct process *);
> void stopprofclock(struct process *);
> Index: arch/alpha/alpha/clock.c
> ===
> RCS file: /cvs/src/sys/arch/alpha/alpha/clock.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 clock.c
> --- arch/alpha/alpha/clock.c 25 Jul 2023 18:16:19 - 1.28
> +++ arch/alpha/alpha/clock.c 19 Aug 2023 18:16:17 -
> @@ -193,7 +193,11 @@ cpu_initclocks(void)
> stathz = hz;
> profhz = stathz;
> clockintr_init(0);
> +}
>
> +void
> +cpu_startclock(void)
> +{
> clockintr_cpu_init(NULL);
>
> /*
> Index: arch/amd64/amd64/machdep.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.286
> diff -u -p -r1.286 machdep.c
> --- arch/amd64/amd64/machdep.c27 Jul 2023 00:28:25 - 1.286
> +++ arch/amd64/amd64/machdep.c19 Aug 2023 18:16:18 -
> @@ -227,6 +227,7 @@ paddr_t avail_end;
>
Re: Virtio fix for testing
On Sun, Aug 20, 2023 at 12:23:49PM +0200, Stefan Fritsch wrote:
> Am 13.08.23 um 17:38 schrieb Tobias Heider:
> > On Sun, Aug 13, 2023 at 08:33:54AM -0400, Andrew Cagney wrote:
> > > > Hi Andrew,
> > > >
> > > > can you share the qemu cmd you are using in your tests?
> > > > I'd like to see if I can reproduce this.
> > >
> > > Here's pretty much everything. Thanks for looking at it.
> >
> > Thank you, I managed to reproduce your crash.
> > I am not yet sure what the exact problem is but you could try using
> > install73.img > instead of install73.iso. It looks like only the iso
> > triggers the bug
> for me.
>
> -cdrom makes qemu add an ATA cdrom drive. This issue has nothing to do with
> the virtio scsi issue / fix from May.
>
> The "wdc_atapi_start" here
>
> >> --:-- ETAwdc_atapi_start: not ready, st = 50
>
> also points to the problem being related to ATA.
>
That matches what I'm seeing. I can reliably reproduce the crash here.
My debug prints show that xfer->chp seems to be garbage:
wdcstart: xfer: 0xfd8016abeea8 xfer->chp: 0x8007f710
wdc_free_xfer: TAILQ_REMOVE(0xfd8016abeea8)
wdcstart: xfer: 0xfd8016abeea8 xfer->chp: 0x8007f710
wdc_free_xfer: TAILQ_REMOVE(0xfd8016abeea8)
wdcstart: xfer: 0xfd8016abeea8 xfer->chp: 0x75d4af0594eaf807
in:
887 /* adjust chp, in case we have a shared queue */
888 chp = xfer->chp;
889
890 if ((chp->ch_flags & WDCF_ACTIVE) != 0 ) {
I haven't had time yet to bisect if and find out when and why that happens.
trace:
wdcstart(8007f710) at wdcstart+0x38 [/usr/src/sys/dev/ic/wdc .c:890]
wdc_atapi_the_machine(8007f710,fd8016abeea8,2) at wdc_atapi_the_mac
hine+0x14a [/usr/src/sys/dev/atapiscsi/atapiscsi.c:640]
wdc_atapi_intr(8007f710,fd8016abeea8,1) at wdc_atapi_intr+0x47
[/usr/src/sys/dev/atapiscsi/atapiscsi.c:550]
wdcintr(8007f710) at wdcintr+0xae [/usr/g/src/sys/dev/ic/wdc.c :969]
intr_handler(8aface68,8006a100) at intr_handler+0x26
[/usr/src/sys/arch/amd64/amd64/intr.c:537]
Xintr_ioapic_edge15_untramp() at Xintr_ioapic_edge15_untramp+0x18f Xspllower()
at Xspllower+0x10
uvm_fault_upper(8afad0d8,8afad110,8afacfd0,0) at uvm_fa
ult_upper+0x1b6 [/usr/src/sys/uvm/uvm_fault.c:1102]
uvm_fault(fd801785ee60,29fd78000,0,2) at uvm_fault+0xb4
[/usr/src/sys/uvm/uvm_fault.c:0]
upageflttrap(8afad230,29fd78da8) at upageflttrap+0x4d
[/usr/src/sys/arch/amd64/amd64/trap.c:189]
usertrap(8afad230) at usertrap+0xbd
[/usr/src/sys/arch/amd64/amd64/trap.c:436]
Re: i386: i8254_initclocks: set IPL_MPSAFE for clock, rtc IRQs
On Sun, Aug 20, 2023 at 10:39:46PM -0500, Scott Cheloha wrote:
> pOn amd64 we lie about the interrupts established during
> i8254_initclocks(). We claim they are MP-safe in order to mollify a
> KASSERT in intr_establish() and continue booting.
>
> See amd64/isa/clock.c:
>279 void
>280 i8254_initclocks(void)
>281 {
>282 i8254_inittimecounter();/* hook the interrupt-based
> i8254 tc */
>283
>284 stathz = 128;
>285 profhz = 1024; /* XXX does not divide into 1 billion
> */
>286 clockintr_init(0);
>287
>288 clockintr_cpu_init(NULL);
>289
>290 /*
>291 * While the clock interrupt handler isn't really MPSAFE, the
>292 * i8254 can't really be used as a clock on a true MP system.
>293 */
>294 isa_intr_establish(NULL, 0, IST_PULSE, IPL_CLOCK | IPL_MPSAFE,
>295 clockintr, 0, "clock");
>296 isa_intr_establish(NULL, 8, IST_PULSE, IPL_STATCLOCK |
> IPL_MPSAFE,
>297 rtcintr, 0, "rtc");
>
> and amd64/amd64/intr.c:
>
>332 void *
>333 intr_establish(int legacy_irq, struct pic *pic, int pin, int type,
> int level,
>334 struct cpu_info *ci, int (*handler)(void *), void *arg, const
> char *what)
>335 {
>336 struct intrhand **p, *q, *ih;
>337 int slot, error, idt_vec;
>338 struct intrsource *source;
>339 struct intrstub *stubp;
>340 int flags;
>341
>342 #ifdef DIAGNOSTIC
>343 if (legacy_irq != -1 && (legacy_irq < 0 || legacy_irq > 15))
>344 panic("intr_establish: bad legacy IRQ value");
>345
>346 if (legacy_irq == -1 && pic == _pic)
>347 panic("intr_establish: non-legacy IRQ on i8259");
>348 #endif
>349
>350 flags = level & IPL_MPSAFE;
>351 level &= ~IPL_MPSAFE;
>352
>353 KASSERT(level <= IPL_TTY || level >= IPL_CLOCK || flags &
> IPL_MPSAFE);
>
> Can we do the same on i386? I'm trying to test the i8254 path on
> modern hardware and I'm tripping the equivalent KASSERT in
> apic_intr_establish().
>
> See i386/i386/ioapic.c:
>
>661 void *
>662 apic_intr_establish(int irq, int type, int level, int (*ih_fun)(void
> *),
>663 void *ih_arg, const char *ih_what)
>664 {
>665 unsigned int ioapic = APIC_IRQ_APIC(irq);
>666 unsigned int intr = APIC_IRQ_PIN(irq);
>667 struct ioapic_softc *sc = ioapic_find(ioapic);
>668 struct ioapic_pin *pin;
>669 struct intrhand **p, *q, *ih;
>670 extern int cold;
>671 int minlevel, maxlevel;
>672 extern void intr_calculatemasks(void); /* XXX */
>673 int flags;
>674
>675 flags = level & IPL_MPSAFE;
>676 level &= ~IPL_MPSAFE;
>677
>678 KASSERT(level <= IPL_TTY || flags & IPL_MPSAFE);
>
> The patch below lets me test the i8254 clockintr path on modern
> hardware in 32-bit mode without needing to rototill the GENERIC
> config to delete all the things that implicitly depend upon the
> ioapic.
>
> I don't think lying in this case is harmful. We can only get to
> i8254_initclocks() if we have no local APIC, or if
> lapic_calibrate_timer() fails.
>
> ok?
>
> Index: clock.c
> ===
> RCS file: /cvs/src/sys/arch/i386/isa/clock.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 clock.c
> --- clock.c 25 Jul 2023 18:16:20 - 1.65
> +++ clock.c 21 Aug 2023 03:26:39 -
> @@ -431,9 +431,9 @@ i8254_initclocks(void)
> clockintr_cpu_init(NULL);
>
> /* When using i8254 for clock, we also use the rtc for profclock */
> - (void)isa_intr_establish(NULL, 0, IST_PULSE, IPL_CLOCK,
> + (void)isa_intr_establish(NULL, 0, IST_PULSE, IPL_CLOCK | IPL_MPSAFE,
> clockintr, 0, "clock");
> - (void)isa_intr_establish(NULL, 8, IST_PULSE, IPL_STATCLOCK,
> + (void)isa_intr_establish(NULL, 8, IST_PULSE, IPL_STATCLOCK | IPL_MPSAFE,
> rtcintr, 0, "rtc");
>
> rtcstart(); /* start the mc146818 clock */
I think this is fine. I tried to come up with a scenario where you'd be doing
smp i386 without a local apic and even the ancient 82489 (for 80486 systems)
acted as a lapic. And since we don't run on real 80386 anymore, I think we can
ignore someone trying to do smp there.
ok mlarkin if it makes your work easier.
Re: Virtio fix for testing
On Sun, 20 Aug 2023 at 06:23, Stefan Fritsch wrote: > > Am 13.08.23 um 17:38 schrieb Tobias Heider: > > On Sun, Aug 13, 2023 at 08:33:54AM -0400, Andrew Cagney wrote: > >>> Hi Andrew, > >>> > >>> can you share the qemu cmd you are using in your tests? > >>> I'd like to see if I can reproduce this. > >> > >> Here's pretty much everything. Thanks for looking at it. > > > > Thank you, I managed to reproduce your crash. > > I am not yet sure what the exact problem is but you could try using > > install73.img > instead of install73.iso. It looks like only the iso > > triggers the bug > for me. > > -cdrom makes qemu add an ATA cdrom drive. This issue has nothing to do > with the virtio scsi issue / fix from May. > > The "wdc_atapi_start" here > > >> --:-- ETAwdc_atapi_start: not ready, st = 50 > > also points to the problem being related to ATA. > > You could try something like > > -device virtio-scsi-pci,id=scsi > -drive file=install73.iso,format=raw,id=cdinst,if=none > -device scsi-cd,drive=cdinst good idea (just need to translate it to virt-install speak) > That depends on your seabios having support for virtio cdroms. Not sure > if that is the default by now. > > Or maybe try a SATA cdrom, but you would need to figure out the qemu > options for that yourself. > > Oh, and the exact qemu version you used may be interesting for people > trying to debug this. virt-install-4.1.0-2.fc38.noarch qemu-7.2.4-2.fc38.x86_64
ober_scanf_elements() and empty sequences
Hi Martijn,
last November you fixed ber.c so that sequences won't generate
an uninitialized subelement.
This revealed another bug in ober_scanf_elements(): it couldn't
process sequences with an empty list of subelements. The following
code failed in ober_scanf_elements():
struct ber_element *root;
struct ber_element *sub;
if ((root = ober_add_sequence(NULL)) == NULL)
err(1, "ober_add_sequence() failed");
errno = 0;
if (ober_scanf_elements(root, "{e", ))
err(1, "ober_scanf_elements() failed");
printf("sub = %p\n", sub);
The patch below fixes that.
Gerhard
Index: lib/libutil/ber.c
===
RCS file: /cvs/src/lib/libutil/ber.c,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 ber.c
--- lib/libutil/ber.c 3 Nov 2022 17:58:10 - 1.24
+++ lib/libutil/ber.c 21 Aug 2023 07:24:21 -
@@ -700,7 +700,8 @@ ober_scanf_elements(struct ber_element *
va_start(ap, fmt);
while (*fmt) {
- if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')')
+ if (ber == NULL && *fmt != '$' && *fmt != '}' && *fmt != ')' &&
+ *fmt != 'e')
goto fail;
switch (*fmt++) {
case '$':
@@ -797,7 +798,7 @@ ober_scanf_elements(struct ber_element *
if (ber->be_encoding != BER_TYPE_SEQUENCE &&
ber->be_encoding != BER_TYPE_SET)
goto fail;
- if (ber->be_sub == NULL || level >= _MAX_SEQ-1)
+ if (level >= _MAX_SEQ-1)
goto fail;
parent[++level] = ber;
ber = ber->be_sub;
smime.p7s
Description: S/MIME cryptographic signature
