On Mon, Sep 25, 2023 at 05:39:34AM +0000, Visa Hankala wrote: > 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. >
The only significant difference is mididetach() where you did not replaced selwakeup() with knote(). Did I missed something? @@ -577,30 +562,20 @@ mididetach(struct device *self, int flag * in read/write/ioctl, which return EIO. */ if (sc->flags) { - KERNEL_ASSERT_LOCKED(); - if (sc->flags & FREAD) { + if (sc->flags & FREAD) wakeup(&sc->inbuf.blocking); - mtx_enter(&audio_lock); - selwakeup(&sc->inbuf.sel); - mtx_leave(&audio_lock); - } > > 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; > > >