On Tue, Dec 15, 2020 at 05:23:45PM +0000, Visa Hankala wrote:
> On Tue, Dec 15, 2020 at 07:46:01AM -0300, Martin Pieuchot wrote:
> > @@ -636,43 +651,59 @@ dopselect(struct proc *p, int nd, fd_set
> > if (sigmask)
> > dosigsuspend(p, *sigmask &~ sigcantmask);
> >
> > -retry:
> > - ncoll = nselcoll;
> > - atomic_setbits_int(&p->p_flag, P_SELECT);
> > - error = selscan(p, pibits[0], pobits[0], nd, ni, retval);
> > - if (error || *retval)
> > + /* Register kqueue events */
> > + if ((error = pselregister(p, pibits[0], nd, ni, &nevents) != 0))
> > goto done;
>
> The above has parentheses wrong and returns 1 (EPERM) as error.
> The lines should read:
>
> if ((error = pselregister(p, pibits[0], nd, ni, &nevents)) != 0)
> goto done;
>
>
> In addition to the above, I noticed that select(2) behaves differently
> than before when a file descriptor that is being monitored is closed by
> another thread. The old implementation returns EBADF. The new code keeps
> on waiting on the underlying object.
>
> The diff below makes kqueue clear kqpoll's fd event registration on
> fd close. However, it does not make select(2) return an error, the fd
> just will not cause a wakeup any longer. I think I have an idea on how
> to correct that but I need to consider it some more.
The following diff adds the ability to return EBADF on fd close. The
idea is to set the error pending on the kqueue instance and deliver it
as the return value of kqueue_scan().
The error condition could also be reported via a kevent struct that
kqueue_scan() returns: set EV_ERROR in kev->flags and error code
in kev->data. However, doing so is somewhat contrived. knote_remove()
would have to rewire the knote, or allocate a new one, with a suitable
f_event code and activate it. In addition, the code would have to use
a dedicated knlist for these knotes, or play some tricks with kq_knlist
or kq_knhash, so that the knotes are still reachable from the kqueue
through some knlist.
The diff assumes eager kqpoll purging after poll(2) or select(2).
Lazy purging needs an additional check that skips raising EBADF if
the knote is not part of current scan.
Index: kern/kern_event.c
===================================================================
RCS file: src/sys/kern/kern_event.c,v
retrieving revision 1.150
diff -u -p -r1.150 kern_event.c
--- kern/kern_event.c 16 Dec 2020 15:07:30 -0000 1.150
+++ kern/kern_event.c 16 Dec 2020 16:27:12 -0000
@@ -95,6 +95,8 @@ void knote_enqueue(struct knote *kn);
void knote_dequeue(struct knote *kn);
int knote_acquire(struct knote *kn);
void knote_release(struct knote *kn);
+void knote_activate(struct knote *);
+void knote_remove(struct proc *p, struct knlist *list, int purge);
void filt_kqdetach(struct knote *kn);
int filt_kqueue(struct knote *kn, long hint);
@@ -504,8 +506,14 @@ kqpoll_init(void)
struct proc *p = curproc;
struct filedesc *fdp;
- if (p->p_kq != NULL)
+ if (p->p_kq != NULL) {
+ /*
+ * Clear any pending error that was raised after
+ * previous scan.
+ */
+ p->p_kq->kq_error = 0;
return;
+ }
p->p_kq = kqueue_alloc(p->p_fd);
p->p_kq_serial = arc4random();
@@ -962,6 +970,15 @@ retry:
}
s = splhigh();
+
+ if (kq->kq_error != 0) {
+ /* Deliver the pending error. */
+ error = kq->kq_error;
+ kq->kq_error = 0;
+ splx(s);
+ goto done;
+ }
+
if (kq->kq_count == 0) {
/*
* Successive loops are only necessary if there are more
@@ -1173,10 +1190,10 @@ kqueue_purge(struct proc *p, struct kque
KERNEL_ASSERT_LOCKED();
for (i = 0; i < kq->kq_knlistsize; i++)
- knote_remove(p, &kq->kq_knlist[i]);
+ knote_remove(p, &kq->kq_knlist[i], 1);
if (kq->kq_knhashmask != 0) {
for (i = 0; i < kq->kq_knhashmask + 1; i++)
- knote_remove(p, &kq->kq_knhash[i]);
+ knote_remove(p, &kq->kq_knhash[i], 1);
}
}
@@ -1356,9 +1373,10 @@ knote(struct klist *list, long hint)
* remove all knotes from a specified knlist
*/
void
-knote_remove(struct proc *p, struct knlist *list)
+knote_remove(struct proc *p, struct knlist *list, int purge)
{
struct knote *kn;
+ struct kqueue *kq;
int s;
while ((kn = SLIST_FIRST(list)) != NULL) {
@@ -1369,6 +1387,21 @@ knote_remove(struct proc *p, struct knli
}
splx(s);
kn->kn_fop->f_detach(kn);
+
+ /*
+ * Notify poll(2) and select(2) when a monitored
+ * file descriptor is closed.
+ */
+ if (!purge && (kn->kn_flags & __EV_POLL) != 0) {
+ kq = kn->kn_kq;
+ s = splhigh();
+ if (kq->kq_error == 0) {
+ kq->kq_error = EBADF;
+ kqueue_wakeup(kq);
+ }
+ splx(s);
+ }
+
knote_drop(kn, p);
}
}
@@ -1399,7 +1432,7 @@ knote_fdclose(struct proc *p, int fd)
continue;
list = &kq->kq_knlist[fd];
- knote_remove(p, list);
+ knote_remove(p, list, 0);
}
KERNEL_UNLOCK();
}
Index: sys/event.h
===================================================================
RCS file: src/sys/sys/event.h,v
retrieving revision 1.49
diff -u -p -r1.49 event.h
--- sys/event.h 9 Dec 2020 18:58:19 -0000 1.49
+++ sys/event.h 16 Dec 2020 16:27:12 -0000
@@ -218,8 +218,6 @@ extern const struct filterops dead_filto
extern void kqpoll_init(void);
extern void kqpoll_exit(void);
extern void knote(struct klist *list, long hint);
-extern void knote_activate(struct knote *);
-extern void knote_remove(struct proc *p, struct knlist *list);
extern void knote_fdclose(struct proc *p, int fd);
extern void knote_processexit(struct proc *);
extern int kqueue_register(struct kqueue *kq,
Index: sys/eventvar.h
===================================================================
RCS file: src/sys/sys/eventvar.h,v
retrieving revision 1.9
diff -u -p -r1.9 eventvar.h
--- sys/eventvar.h 7 Apr 2020 13:27:52 -0000 1.9
+++ sys/eventvar.h 16 Dec 2020 16:27:12 -0000
@@ -59,6 +59,7 @@ struct kqueue {
#define KQ_SEL 0x01
#define KQ_SLEEP 0x02
#define KQ_DYING 0x04
+ int kq_error; /* pending error */
};
#endif /* !_SYS_EVENTVAR_H_ */
