On Fri, Oct 29, 2021 at 04:08:39PM +0200, Martin Pieuchot wrote:
> On 29/10/21(Fri) 14:47, Martin Pieuchot wrote:
> > On 29/10/21(Fri) 10:50, Anton Lindqvist wrote:
> > > On Fri, Oct 29, 2021 at 09:13:58AM +0100, Larry Hynes wrote:
> > > > Hi
> > > >
> > > > In last two snapshots I installed, poll seems to hang in certain
> > > > circumstances.
> > > >
> > > > Here's a reproducer, from Leah Neukirchen (cc'ed on this mail):
> >
> > Thanks, do you agree to license it under ISC, as in
> > /usr/share/misc/license.template, so we can put it in src/regress?
> >
> > > >
> > > > ------
> > > >
> > > > #include <stdio.h>
> > > > #include <poll.h>
> > > > #include <unistd.h>
> > > >
> > > > int
> > > > main()
> > > > {
> > > > struct pollfd fds[1];
> > > >
> > > > fds[0].fd = 0;
> > > > fds[0].events = POLLIN | POLLHUP;
> > > > close(0);
> > > >
> > > > printf("%d\n", poll(fds, 1, -1));
> > > > printf("%d\n", fds[0].revents & POLLNVAL);
> > > > }
> > > >
> > > > ------
> > > >
> > > > When compiled and run, that should hang on latest snap. It would be
> > > > expected to return immediately with POLLNVAL set.
> > >
> > > I think poll() needs similar handling as select() recently gained.
> > > Caution, only compile tested.
> >
> > Here's a simpler fix, do not sleep at all if we already have something
> > to report.
>
> After some discussions with anton@ I believe we should instead go with a
> version similar to what sys_select is doing. It is necessary to call
> ppollcollect() for FDs that are ready.
>
> Here's anton@'s version without extra argument:
Thanks, committed by now.
> Index: kern/sys_generic.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 sys_generic.c
> --- kern/sys_generic.c 24 Oct 2021 11:23:22 -0000 1.138
> +++ kern/sys_generic.c 29 Oct 2021 14:05:56 -0000
> @@ -1128,7 +1128,7 @@ doppoll(struct proc *p, struct pollfd *f
> {
> struct kqueue_scan_state scan;
> struct pollfd pfds[4], *pl = pfds;
> - int error, nevents = 0;
> + int error, ncollected, nevents = 0;
> size_t sz;
>
> /* Standards say no more than MAX_OPEN; this is possibly better. */
> @@ -1154,14 +1154,14 @@ doppoll(struct proc *p, struct pollfd *f
> dosigsuspend(p, *sigmask &~ sigcantmask);
>
> /* Register kqueue events */
> - *retval = ppollregister(p, pl, nfds, &nevents);
> + ncollected = ppollregister(p, pl, nfds, &nevents);
>
> /*
> * 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) {
> + if (nevents == 0 && ncollected == 0) {
> uint64_t nsecs = INFSLP;
>
> if (timeout != NULL) {
> @@ -1184,7 +1184,7 @@ doppoll(struct proc *p, struct pollfd *f
> struct kevent kev[KQ_NEVENTS];
> int i, ready, count;
>
> - /* Maxium number of events per iteration */
> + /* Maximum number of events per iteration */
> count = MIN(nitems(kev), nevents);
> ready = kqueue_scan(&scan, count, kev, timeout, p, &error);
> #ifdef KTRACE
> @@ -1193,7 +1193,7 @@ doppoll(struct proc *p, struct pollfd *f
> #endif
> /* Convert back events that are ready. */
> for (i = 0; i < ready; i++)
> - *retval += ppollcollect(p, &kev[i], pl, nfds);
> + ncollected += ppollcollect(p, &kev[i], pl, nfds);
>
> /*
> * Stop if there was an error or if we had enough
> @@ -1205,6 +1205,7 @@ doppoll(struct proc *p, struct pollfd *f
> nevents -= ready;
> }
> kqueue_scan_finish(&scan);
> + *retval = ncollected;
> done:
> /*
> * NOTE: poll(2) is not restarted after a signal and EWOULDBLOCK is
>
