Re: Please test: full poll/select(2) switch
On 29/10/21(Fri) 14:48, Alexandre Ratchov wrote: > On Fri, Oct 29, 2021 at 01:12:06PM +0100, Martin Pieuchot wrote: > > On 29/10/21(Fri) 13:12, Alexandre Ratchov wrote: > > > On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote: > > > > Diff below switches both poll(2) and select(2) to the kqueue-based > > > > implementation. > > > > > > > > In addition it switches libevent(3) to use poll(2) by default for > > > > testing purposes. > > > > > > > > I don't have any open bug left with this diff and I'm happily running > > > > GNOME with it. So I'd be happy if you could try to break it and report > > > > back. > > > > > > > > > > Without the below diff (copied from audio(4) driver), kernel panics > > > upon the first MIDI input byte. > > > > What is the panic? The mutex is taken recursively, right? > > > > Exactly, this is the "locking against myself", panic. > > AFAIU, the interrupt handler grabs the audio_lock and calls > midi_iintr(). It calls selwakeup(), which in turn calls > filt_midiread(), which attempts to grab the audio_lock a second time. > > > > ok? suggestion for a better fix? > > > > Without seeing the panic, I'm guessing this is correct. > > > > That suggest kevent(2) wasn't safe to use with midi(4). > > > > Yes, this is the very first time midi(4) is used with kevent(2). Then this is correct, thanks a lot. Please go ahead, ok mpi@
Re: Please test: full poll/select(2) switch
On Fri, Oct 29, 2021 at 01:12:06PM +0100, Martin Pieuchot wrote: > On 29/10/21(Fri) 13:12, Alexandre Ratchov wrote: > > On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote: > > > Diff below switches both poll(2) and select(2) to the kqueue-based > > > implementation. > > > > > > In addition it switches libevent(3) to use poll(2) by default for > > > testing purposes. > > > > > > I don't have any open bug left with this diff and I'm happily running > > > GNOME with it. So I'd be happy if you could try to break it and report > > > back. > > > > > > > Without the below diff (copied from audio(4) driver), kernel panics > > upon the first MIDI input byte. > > What is the panic? The mutex is taken recursively, right? > Exactly, this is the "locking against myself", panic. AFAIU, the interrupt handler grabs the audio_lock and calls midi_iintr(). It calls selwakeup(), which in turn calls filt_midiread(), which attempts to grab the audio_lock a second time. > > ok? suggestion for a better fix? > > Without seeing the panic, I'm guessing this is correct. > > That suggest kevent(2) wasn't safe to use with midi(4). > Yes, this is the very first time midi(4) is used with kevent(2).
Re: Please test: full poll/select(2) switch
On 29/10/21(Fri) 13:12, Alexandre Ratchov wrote: > On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote: > > Diff below switches both poll(2) and select(2) to the kqueue-based > > implementation. > > > > In addition it switches libevent(3) to use poll(2) by default for > > testing purposes. > > > > I don't have any open bug left with this diff and I'm happily running > > GNOME with it. So I'd be happy if you could try to break it and report > > back. > > > > Without the below diff (copied from audio(4) driver), kernel panics > upon the first MIDI input byte. What is the panic? The mutex is taken recursively, right? > ok? suggestion for a better fix? Without seeing the panic, I'm guessing this is correct. That suggest kevent(2) wasn't safe to use with midi(4). > Index: midi.c > === > RCS file: /cvs/src/sys/dev/midi.c,v > retrieving revision 1.48 > diff -u -p -r1.48 midi.c > --- midi.c25 Dec 2020 12:59:52 - 1.48 > +++ midi.c29 Oct 2021 11:09:47 - > @@ -386,9 +386,11 @@ filt_midiread(struct knote *kn, long hin > struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; > int retval; > > - mtx_enter(_lock); > + if ((hint & NOTE_SUBMIT) == 0) > + mtx_enter(_lock); > retval = !MIDIBUF_ISEMPTY(>inbuf); > - mtx_leave(_lock); > + if ((hint & NOTE_SUBMIT) == 0) > + mtx_leave(_lock); > > return (retval); > } > @@ -409,9 +411,11 @@ filt_midiwrite(struct knote *kn, long hi > struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; > intretval; > > - mtx_enter(_lock); > + if ((hint & NOTE_SUBMIT) == 0) > + mtx_enter(_lock); > retval = !MIDIBUF_ISFULL(>outbuf); > - mtx_leave(_lock); > + if ((hint & NOTE_SUBMIT) == 0) > + mtx_leave(_lock); > > return (retval); > } > >
Re: Please test: full poll/select(2) switch
On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote: > Diff below switches both poll(2) and select(2) to the kqueue-based > implementation. > > In addition it switches libevent(3) to use poll(2) by default for > testing purposes. > > I don't have any open bug left with this diff and I'm happily running > GNOME with it. So I'd be happy if you could try to break it and report > back. > Without the below diff (copied from audio(4) driver), kernel panics upon the first MIDI input byte. ok? suggestion for a better fix? Index: midi.c === RCS file: /cvs/src/sys/dev/midi.c,v retrieving revision 1.48 diff -u -p -r1.48 midi.c --- midi.c 25 Dec 2020 12:59:52 - 1.48 +++ midi.c 29 Oct 2021 11:09:47 - @@ -386,9 +386,11 @@ filt_midiread(struct knote *kn, long hin struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; int retval; - mtx_enter(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_enter(_lock); retval = !MIDIBUF_ISEMPTY(>inbuf); - mtx_leave(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_leave(_lock); return (retval); } @@ -409,9 +411,11 @@ filt_midiwrite(struct knote *kn, long hi struct midi_softc *sc = (struct midi_softc *)kn->kn_hook; intretval; - mtx_enter(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_enter(_lock); retval = !MIDIBUF_ISFULL(>outbuf); - mtx_leave(_lock); + if ((hint & NOTE_SUBMIT) == 0) + mtx_leave(_lock); return (retval); }
Re: Please test: full poll/select(2) switch
On Sat, Oct 23, 2021 at 10:40:56AM +0100, Martin Pieuchot wrote:
> Diff below switches both poll(2) and select(2) to the kqueue-based
> implementation.
Passed regress on i386.
> In addition it switches libevent(3) to use poll(2) by default for
> testing purposes.
It created some test failures in syslogd regress as it could not
see kqueue in ktrace. This is an expected failure.
> I don't have any open bug left with this diff and I'm happily running
> GNOME with it. So I'd be happy if you could try to break it and report
> back.
I think we should give it a try.
> +#ifdef DIAGNOSTIC
> + /*
> + * Lazily delete spurious events.
> + *
> + * This should not happen as long as kqueue_purge() is called
> + * at the end of every syscall. It migh be interesting to do
> + * like DragonFlyBSD and not always allocated a new knote in
> + * kqueue_register() with that lazy removal makes sense.
> + */
> + if (i >= nfds) {
> + DPRINTFN(0, "poll get out of range udata %lu vs serial %lu\n",
> + (unsigned long)kevp->udata, p->p_kq_serial);
> + kevp->flags = EV_DISABLE|EV_DELETE;
> + kqueue_register(p->p_kq, kevp, p);
> + return (0);
> + }
> + if ((int)kevp->ident != pl[i].fd) {
> + DPRINTFN(0, "poll get %lu/%d mismatch fd %u!=%d serial %lu\n",
> + i+1, nfds, (int)kevp->ident, pl[i].fd, p->p_kq_serial);
> + return (0);
> + }
> +#endif
DIAGNOSTIC should not change behavior by returning 0.
> + /* NOTE: POLLHUP and POLLOUT/POLLWRNORM are mutually exclusive*/
Remove "NOTE:" and the comment will fit into the line.
> +#if 1
> + if (pl[i].events != 0 && pl[i].events != POLLOUT)
> + DPRINTFN(0, "weird events %x\n", pl[i].events);
> +#endif
Why #if 1, should it be DIAGNOSTIC ?
> + /* Tell uper layer to ask for POLLUP only */
upper
Please test: full poll/select(2) switch
Diff below switches both poll(2) and select(2) to the kqueue-based
implementation.
In addition it switches libevent(3) to use poll(2) by default for
testing purposes.
I don't have any open bug left with this diff and I'm happily running
GNOME with it. So I'd be happy if you could try to break it and report
back.
Index: lib/libevent/event.c
===
RCS file: /cvs/src/lib/libevent/event.c,v
retrieving revision 1.41
diff -u -p -r1.41 event.c
--- lib/libevent/event.c1 May 2019 19:14:25 - 1.41
+++ lib/libevent/event.c23 Oct 2021 09:36:10 -
@@ -53,9 +53,9 @@ extern const struct eventop kqops;
/* In order of preference */
static const struct eventop *eventops[] = {
- ,
,
,
+ ,
NULL
};
Index: sys/kern/sys_generic.c
===
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.137
diff -u -p -r1.137 sys_generic.c
--- sys/kern/sys_generic.c 15 Oct 2021 06:59:57 - 1.137
+++ sys/kern/sys_generic.c 23 Oct 2021 09:14:59 -
@@ -55,6 +55,7 @@
#include
#include
#include
+#include
#ifdef KTRACE
#include
#endif
@@ -66,8 +67,23 @@
#include
-int selscan(struct proc *, fd_set *, fd_set *, int, int, register_t *);
-void pollscan(struct proc *, struct pollfd *, u_int, register_t *);
+/*
+ * Debug values:
+ * 1 - print implementation errors, things that should not happen.
+ * 2 - print ppoll(2) information, somewhat verbose
+ * 3 - print pselect(2) and ppoll(2) information, very verbose
+ */
+int kqpoll_debug = 0;
+#define DPRINTFN(v, x...) if (kqpoll_debug > v) { \
+ printf("%s(%d): ", curproc->p_p->ps_comm, curproc->p_tid); \
+ printf(x); \
+}
+
+int pselregister(struct proc *, fd_set *[], fd_set *[], int, int *, int *);
+int pselcollect(struct proc *, struct kevent *, fd_set *[], int *);
+int ppollregister(struct proc *, struct pollfd *, int, int *);
+int ppollcollect(struct proc *, struct kevent *, struct pollfd *, u_int);
+
int pollout(struct pollfd *, struct pollfd *, u_int);
int dopselect(struct proc *, int, fd_set *, fd_set *, fd_set *,
struct timespec *, const sigset_t *, register_t *);
@@ -584,11 +600,10 @@ int
dopselect(struct proc *p, int nd, fd_set *in, fd_set *ou, fd_set *ex,
struct timespec *timeout, const sigset_t *sigmask, register_t *retval)
{
+ struct kqueue_scan_state scan;
fd_mask bits[6];
fd_set *pibits[3], *pobits[3];
- struct timespec elapsed, start, stop;
- uint64_t nsecs;
- int s, ncoll, error = 0;
+ int error, ncollected = 0, nevents = 0;
u_int ni;
if (nd < 0)
@@ -618,6 +633,8 @@ dopselect(struct proc *p, int nd, fd_set
pobits[2] = (fd_set *)[5];
}
+ kqpoll_init();
+
#definegetbits(name, x) \
if (name && (error = copyin(name, pibits[x], ni))) \
goto done;
@@ -636,43 +653,61 @@ dopselect(struct proc *p, int nd, fd_set
if (sigmask)
dosigsuspend(p, *sigmask &~ sigcantmask);
-retry:
- ncoll = nselcoll;
- atomic_setbits_int(>p_flag, P_SELECT);
- error = selscan(p, pibits[0], pobits[0], nd, ni, retval);
- if (error || *retval)
+ /* Register kqueue events */
+ error = pselregister(p, pibits, pobits, nd, , );
+ if (error != 0)
goto done;
- if (timeout == NULL || timespecisset(timeout)) {
- if (timeout != NULL) {
- getnanouptime();
- nsecs = MIN(TIMESPEC_TO_NSEC(timeout), MAXTSLP);
- } else
- nsecs = INFSLP;
- s = splhigh();
- if ((p->p_flag & P_SELECT) == 0 || nselcoll != ncoll) {
- splx(s);
- goto retry;
- }
- atomic_clearbits_int(>p_flag, P_SELECT);
- error = tsleep_nsec(, PSOCK | PCATCH, "select", nsecs);
- splx(s);
+
+ /*
+* The poll/select family of syscalls has been designed to
+* block when file descriptors are not available, even if
+* there's nothing to wait for.
+*/
+ if (nevents == 0 && ncollected == 0) {
+ uint64_t nsecs = INFSLP;
+
if (timeout != NULL) {
- getnanouptime();
- timespecsub(, , );
- timespecsub(timeout, , timeout);
- if (timeout->tv_sec < 0)
- timespecclear(timeout);
+ if (!timespecisset(timeout))
+ goto done;
+ nsecs = MAX(1, MIN(TIMESPEC_TO_NSEC(timeout), MAXTSLP));
}
- if (error == 0 || error == EWOULDBLOCK)
-
