On Wed, Jun 23, 2021 at 05:39:05PM +0200, Mark Kettenis wrote: > > Date: Wed, 23 Jun 2021 15:32:03 +0000 > > From: Visa Hankala <v...@hankala.org> > > > > On Wed, Jun 23, 2021 at 05:15:05PM +0200, Mark Kettenis wrote: > > > > Date: Wed, 23 Jun 2021 14:56:45 +0000 > > > > From: Visa Hankala <v...@hankala.org> > > > > > > > > On Tue, Jun 22, 2021 at 09:46:22AM -0500, Scott Cheloha wrote: > > > > > On Mon, Jun 21, 2021 at 02:04:30PM +0000, Visa Hankala wrote: > > > > > > On Thu, May 27, 2021 at 07:40:26PM -0500, Scott Cheloha wrote: > > > > > > > On Sun, May 23, 2021 at 09:05:24AM +0000, Visa Hankala wrote: > > > > > > > > When a CPU starts processing a soft interrupt, it reserves the > > > > > > > > handler > > > > > > > > to prevent concurrent execution. If the soft interrupt gets > > > > > > > > rescheduled > > > > > > > > during processing, the handler is run again by the same CPU. > > > > > > > > This breaks > > > > > > > > FIFO ordering, though. > > > > > > > > > > > > > > If you want to preserve FIFO you can reinsert the handler at the > > > > > > > queue > > > > > > > tail. That would be more fair. > > > > > > > > > > > > > > If FIFO is the current behavior I think we ought to keep it. > > > > > > > > > > > > I have updated the patch to preserve the FIFO order. > > > > > > > > > > > > > > +STAILQ_HEAD(x86_soft_intr_queue, x86_soft_intrhand); > > > > > > > > + > > > > > > > > +struct x86_soft_intr_queue softintr_queue[X86_NSOFTINTR]; > > > > > > > > > > > > > > Why did we switch to STAILQ? I know we don't have very many > > > > > > > softintr_disestablish() calls but isn't O(1) removal worth the > > > > > > > extra > > > > > > > pointer? > > > > > > > > > > > > I used STAILQ because it avoids the hassle of updating the list > > > > > > nodes' > > > > > > back pointers. softintr_disestablish() with multiple items pending > > > > > > in > > > > > > the queue is very rare in comparison to the normal > > > > > > softintr_schedule() / > > > > > > softintr_dispatch() cycle. > > > > > > > > > > > > However, I have changed the code back to using TAILQ. > > > > > > > > > > This looks good to me. I mean, it looked good before, but it still > > > > > looks good. > > > > > > > > > > I will run with it for a few days. > > > > > > > > > > Assuming I hit no issues I'll come back with an OK. > > > > > > > > > > Is there an easy way to exercise this code from userspace? There > > > > > aren't many softintr users. > > > > > > > > > > Maybe audio drivers? > > > > > > > > audio(4) is one option with a relatively high rate of scheduling. > > > > Serial communications drivers, such as com(4), might be useful for > > > > testing too. > > > > > > > > softintr_disestablish() can be exercised with uaudio(4) and ucom(4) > > > > for example. > > > > > > > > I am still uncertain whether the barrier in softintr_disestablish() > > > > is fully safe. The typical detach-side users are audio_detach(), > > > > com_detach() and usb_detach(). They should be fine because the > > > > surrounding code may sleep. However, sbus(4) worries me because it > > > > invokes softintr_disestablish() from PCMCIA intr_disestablish callback, > > > > and I do not know how wild the usage contexts can be. sbus(4) is > > > > specific to sparc64, though. > > > > > > Suprise-removal is a thing for PCI as well as PCMCIA and USB. And in > > > the PCI case this will call com_detach() and therefore > > > softintr_disestablish() from interrupt context, where you can't sleep. > > > > > > So I don't think that using some sort of barrier that sleeps is an > > > option. > > > > Well, com_detach() does things that may sleep, so then the existing code > > seems wrong. > > Hmm, actually, it seems I misremembered and PCI hotplug remove runs in > a task (see dev/pci/ppb.c). So maybe it is ok. > > > I will revise the diff so that it spins rather than sleeps when a handler > > is active. > > That wouldn't work on non-MP kernels isn't it?
Barrier-like removal is not reliable in interrupt context because an interrupt might have preempted the soft interrupt handler that the interrupt handler is about to remove. This applies to both MP and non-MP kernels. On a non-MP kernel, spinning, or sleeping, is not actually needed. Pending soft interrupts should run immediately when the system priority level allows them. If SPL is none on entry to softintr_disestablish(), the handler should have finished already. If SPL blocks soft interrupts on entry, the dequeueing clears any pending soft interrupt.
