On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote: > Please test this diff, I have no midi(4) devices. > > midi(4) already uses `audio_lock' mutex(9) for filterops, but they are > still kernel locked. Wipe out old selwakeup API and make them MP safe. > knote_locked(9) will not grab kernel lock, so call it directly from > interrupt handlers instead of scheduling software interrupts.
https://marc.info/?l=openbsd-tech&m=167604232828221 has minor takeaways if you pay attention. > Index: sys/dev/midi.c > =================================================================== > RCS file: /cvs/src/sys/dev/midi.c,v > retrieving revision 1.55 > diff -u -p -r1.55 midi.c > --- sys/dev/midi.c 2 Jul 2022 08:50:41 -0000 1.55 > +++ sys/dev/midi.c 24 Sep 2023 19:57:56 -0000 > @@ -31,7 +31,6 @@ > #include <dev/audio_if.h> > #include <dev/midivar.h> > > -#define IPL_SOFTMIDI IPL_SOFTNET > #define DEVNAME(sc) ((sc)->dev.dv_xname) > > int midiopen(dev_t, int, int, struct proc *); > @@ -65,41 +64,38 @@ struct cfdriver midi_cd = { > > void filt_midiwdetach(struct knote *); > int filt_midiwrite(struct knote *, long); > +int filt_midimodify(struct kevent *, struct knote *); > +int filt_midiprocess(struct knote *, struct kevent *); > > const struct filterops midiwrite_filtops = { > - .f_flags = FILTEROP_ISFD, > + .f_flags = FILTEROP_ISFD | FILTEROP_MPSAFE, > .f_attach = NULL, > .f_detach = filt_midiwdetach, > .f_event = filt_midiwrite, > + .f_modify = filt_midimodify, > + .f_process = filt_midiprocess, > }; > > void filt_midirdetach(struct knote *); > int filt_midiread(struct knote *, long); > > const struct filterops midiread_filtops = { > - .f_flags = FILTEROP_ISFD, > + .f_flags = FILTEROP_ISFD | FILTEROP_MPSAFE, > .f_attach = NULL, > .f_detach = filt_midirdetach, > .f_event = filt_midiread, > + .f_modify = filt_midimodify, > + .f_process = filt_midiprocess, > }; > > void > -midi_buf_wakeup(void *addr) > +midi_buf_wakeup(struct midi_buffer *buf) > { > - struct midi_buffer *buf = addr; > - > if (buf->blocking) { > wakeup(&buf->blocking); > buf->blocking = 0; > } > - /* > - * As long as selwakeup() grabs the KERNEL_LOCK() make sure it is > - * already held here to avoid lock ordering problems with `audio_lock' > - */ > - KERNEL_ASSERT_LOCKED(); > - mtx_enter(&audio_lock); > - selwakeup(&buf->sel); > - mtx_leave(&audio_lock); > + knote_locked(&buf->klist, 0); > } > > void > @@ -117,13 +113,7 @@ midi_iintr(void *addr, int data) > > MIDIBUF_WRITE(mb, data); > > - /* > - * As long as selwakeup() needs to be protected by the > - * KERNEL_LOCK() we have to delay the wakeup to another > - * context to keep the interrupt context KERNEL_LOCK() > - * free. > - */ > - softintr_schedule(sc->inbuf.softintr); > + midi_buf_wakeup(mb); > } > > int > @@ -226,14 +216,7 @@ void > midi_out_stop(struct midi_softc *sc) > { > sc->isbusy = 0; > - > - /* > - * As long as selwakeup() needs to be protected by the > - * KERNEL_LOCK() we have to delay the wakeup to another > - * context to keep the interrupt context KERNEL_LOCK() > - * free. > - */ > - softintr_schedule(sc->outbuf.softintr); > + midi_buf_wakeup(&sc->outbuf); > } > > void > @@ -342,11 +325,11 @@ midikqfilter(dev_t dev, struct knote *kn > error = 0; > switch (kn->kn_filter) { > case EVFILT_READ: > - klist = &sc->inbuf.sel.si_note; > + klist = &sc->inbuf.klist; > kn->kn_fop = &midiread_filtops; > break; > case EVFILT_WRITE: > - klist = &sc->outbuf.sel.si_note; > + klist = &sc->outbuf.klist; > kn->kn_fop = &midiwrite_filtops; > break; > default: > @@ -355,9 +338,7 @@ midikqfilter(dev_t dev, struct knote *kn > } > kn->kn_hook = (void *)sc; > > - mtx_enter(&audio_lock); > - klist_insert_locked(klist, kn); > - mtx_leave(&audio_lock); > + klist_insert(klist, kn); > done: > device_unref(&sc->dev); > return error; > @@ -368,24 +349,15 @@ filt_midirdetach(struct knote *kn) > { > struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; > > - mtx_enter(&audio_lock); > - klist_remove_locked(&sc->inbuf.sel.si_note, kn); > - mtx_leave(&audio_lock); > + klist_remove(&sc->inbuf.klist, kn); > } > > int > filt_midiread(struct knote *kn, long hint) > { > struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; > - int retval; > - > - if ((hint & NOTE_SUBMIT) == 0) > - mtx_enter(&audio_lock); > - retval = !MIDIBUF_ISEMPTY(&sc->inbuf); > - if ((hint & NOTE_SUBMIT) == 0) > - mtx_leave(&audio_lock); > > - return (retval); > + return (!MIDIBUF_ISEMPTY(&sc->inbuf)); > } > > void > @@ -393,24 +365,39 @@ filt_midiwdetach(struct knote *kn) > { > struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; > > - mtx_enter(&audio_lock); > - klist_remove_locked(&sc->outbuf.sel.si_note, kn); > - mtx_leave(&audio_lock); > + klist_remove(&sc->outbuf.klist, kn); > } > > int > filt_midiwrite(struct knote *kn, long hint) > { > struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; > - int retval; > > - if ((hint & NOTE_SUBMIT) == 0) > - mtx_enter(&audio_lock); > - retval = !MIDIBUF_ISFULL(&sc->outbuf); > - if ((hint & NOTE_SUBMIT) == 0) > - mtx_leave(&audio_lock); > + return (!MIDIBUF_ISFULL(&sc->outbuf)); > +} > + > +int > +filt_midimodify(struct kevent *kev, struct knote *kn) > +{ > + int active; > + > + mtx_enter(&audio_lock); > + active = knote_modify(kev, kn); > + mtx_leave(&audio_lock); > + > + return active; > +} > + > +int > +filt_midiprocess(struct knote *kn, struct kevent *kev) > +{ > + int active; > + > + mtx_enter(&audio_lock); > + active = knote_process(kn, kev); > + mtx_leave(&audio_lock); > > - return (retval); > + return active; > } > > int > @@ -531,20 +518,8 @@ midiattach(struct device *parent, struct > } > #endif > > - sc->inbuf.softintr = softintr_establish(IPL_SOFTMIDI, > - midi_buf_wakeup, &sc->inbuf); > - if (sc->inbuf.softintr == NULL) { > - printf("%s: can't establish input softintr\n", DEVNAME(sc)); > - return; > - } > - > - sc->outbuf.softintr = softintr_establish(IPL_SOFTMIDI, > - midi_buf_wakeup, &sc->outbuf); > - if (sc->outbuf.softintr == NULL) { > - printf("%s: can't establish output softintr\n", DEVNAME(sc)); > - softintr_disestablish(sc->inbuf.softintr); > - return; > - } > + klist_init_mutex(&sc->inbuf.klist, &audio_lock); > + klist_init_mutex(&sc->outbuf.klist, &audio_lock); > > sc->hw_if = hwif; > sc->hw_hdl = hdl; > @@ -580,27 +555,19 @@ mididetach(struct device *self, int flag > KERNEL_ASSERT_LOCKED(); > if (sc->flags & FREAD) { > wakeup(&sc->inbuf.blocking); > - mtx_enter(&audio_lock); > - selwakeup(&sc->inbuf.sel); > - mtx_leave(&audio_lock); > + knote(&sc->inbuf.klist, 0); > } > if (sc->flags & FWRITE) { > wakeup(&sc->outbuf.blocking); > - mtx_enter(&audio_lock); > - selwakeup(&sc->outbuf.sel); > - mtx_leave(&audio_lock); > + knote(&sc->outbuf.klist, 0); > } > sc->hw_if->close(sc->hw_hdl); > sc->flags = 0; > } > > - klist_invalidate(&sc->inbuf.sel.si_note); > - klist_invalidate(&sc->outbuf.sel.si_note); > + klist_invalidate(&sc->inbuf.klist); > + klist_invalidate(&sc->outbuf.klist); > > - if (sc->inbuf.softintr) > - softintr_disestablish(sc->inbuf.softintr); > - if (sc->outbuf.softintr) > - softintr_disestablish(sc->outbuf.softintr); > return 0; > } > > Index: sys/dev/midivar.h > =================================================================== > RCS file: /cvs/src/sys/dev/midivar.h,v > retrieving revision 1.13 > diff -u -p -r1.13 midivar.h > --- sys/dev/midivar.h 21 Mar 2022 19:22:40 -0000 1.13 > +++ sys/dev/midivar.h 24 Sep 2023 19:57:56 -0000 > @@ -21,7 +21,7 @@ > > #include <dev/midi_if.h> > #include <sys/device.h> > -#include <sys/selinfo.h> > +#include <sys/event.h> > #include <sys/proc.h> > #include <sys/timeout.h> > > @@ -34,8 +34,7 @@ > #define MIDIBUF_MASK (MIDIBUF_SIZE - 1) > > struct midi_buffer { > - void *softintr; /* context to call selwakeup() */ > - struct selinfo sel; /* to record & wakeup poll(2) */ > + struct klist klist; /* to record & wakeup poll(2) */ > int blocking; /* read/write blocking */ > unsigned char data[MIDIBUF_SIZE]; > unsigned start, used; >
