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

Reply via email to