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;
> >
>
