Refactor dopselect() and doppoll()

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Refactor dopselect() and doppoll()

Matthew Dempsky-3
Currently there's a lot of redundancy between dopselect() and
doppoll().  This diff cleans them up in the following ways:

  - Better variable names.  Instead of "rts", "ats", and "tts" they're
    now called "deadline", "now", and "diff"; and "ncoll" is now
    "selcookie".  They're also all more minimally scoped.

  - Prefer loops over goto.

  - Loosely styled after kern_synch.c's sleep_{setup,finish}() APIs,
    there are now selsetup(), selabort(), and selsleep() methods that
    dopselect() and doppoll() are implemented in terms of.  Moreover,
    "P_SELECT", "nselcoll", and "selwait" are now only accessed within
    sel{setup,record,abort,sleep,wakeup}(), making it easier to update
    and review their interactions.

  - poll() and ppoll() now show up in "ps" as "select" instead of
    "poll".  This is just for simplicity because I don't think
    distinguishing them offers much value when they're basically just
    different APIs for the same underlying logic, but it's easy to
    restore if people care.

Other than the last point, this should be behavior preserving.
Followup changes will tweak things a bit further, but having this diff
in place should make them easier to review.

ok?


Index: sys_generic.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.90
diff -u -p -r1.90 sys_generic.c
--- sys_generic.c 12 Jul 2014 21:21:19 -0000 1.90
+++ sys_generic.c 12 Jul 2014 22:18:57 -0000
@@ -62,6 +62,9 @@
 
 #include <uvm/uvm_extern.h>
 
+int selsetup(struct proc *);
+void selabort(struct proc *);
+int selsleep(struct proc *, int, const struct timespec *);
 int selscan(struct proc *, fd_set *, fd_set *, int, int, register_t *);
 void pollscan(struct proc *, struct pollfd *, u_int, register_t *);
 int pollout(struct pollfd *, struct pollfd *, u_int);
@@ -602,8 +605,8 @@ dopselect(struct proc *p, int nd, fd_set
 {
  fd_mask bits[6];
  fd_set *pibits[3], *pobits[3];
- struct timespec ats, rts, tts;
- int s, ncoll, error = 0, timo;
+ struct timespec deadline;
+ int error = 0;
  u_int ni;
 
  if (nd < 0)
@@ -649,43 +652,27 @@ dopselect(struct proc *p, int nd, fd_set
 #endif
 
  if (tsp) {
- getnanouptime(&rts);
- timespecadd(tsp, &rts, &ats);
- } else {
- ats.tv_sec = 0;
- ats.tv_nsec = 0;
+ struct timespec now;
+ getnanouptime(&now);
+ timespecadd(&now, tsp, &deadline);
  }
- timo = 0;
 
  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)
- goto done;
- if (tsp) {
- getnanouptime(&rts);
- if (timespeccmp(&rts, &ats, >=))
- goto done;
- timespecsub(&ats, &rts, &tts);
- timo = tts.tv_sec > 24 * 60 * 60 ?
- 24 * 60 * 60 * hz : tstohz(&tts);
- }
- s = splhigh();
- if ((p->p_flag & P_SELECT) == 0 || nselcoll != ncoll) {
- splx(s);
- goto retry;
+ for (;;) {
+ int selcookie = selsetup(p);
+ error = selscan(p, pibits[0], pobits[0], nd, ni, retval);
+ if (error || *retval) {
+ selabort(p);
+ break;
+ }
+ error = selsleep(p, selcookie, tsp ? &deadline : NULL);
+ if (error)
+ break;
  }
- atomic_clearbits_int(&p->p_flag, P_SELECT);
- error = tsleep(&selwait, PSOCK | PCATCH, "select", timo);
- splx(s);
- if (error == 0)
- goto retry;
+
 done:
- atomic_clearbits_int(&p->p_flag, P_SELECT);
  /* select is not restarted after signals... */
  if (error == ERESTART)
  error = EINTR;
@@ -766,6 +753,27 @@ selfalse(dev_t dev, int events, struct p
 }
 
 /*
+ * Setup to wait for a selectable event.
+ *
+ * Returns a select cookie value that should be passed to selsleep().
+ */
+int
+selsetup(struct proc *p)
+{
+ atomic_setbits_int(&p->p_flag, P_SELECT);
+ return (nselcoll);
+}
+
+/*
+ * Abort waiting for a selectable event.
+ */
+void
+selabort(struct proc *p)
+{
+ atomic_clearbits_int(&p->p_flag, P_SELECT);
+}
+
+/*
  * Record a select request.
  */
 void
@@ -785,6 +793,39 @@ selrecord(struct proc *selector, struct
 }
 
 /*
+ * Sleep until a recorded select event has occurred.
+ *
+ * Returns 0 if caller should rescan for events, EWOULDBLOCK if deadline has
+ * passed, or EINTR/EAGAIN if select should terminate early.
+ */
+int
+selsleep(struct proc *p, int selcookie, const struct timespec *deadline)
+{
+ int error, s, timo = 0;
+ if (deadline != NULL) {
+ struct timespec now, diff;
+ getnanouptime(&now);
+ if (timespeccmp(&now, deadline, >=)) {
+ selabort(p);
+ return (EWOULDBLOCK);
+ }
+ timespecsub(deadline, &now, &diff);
+ timo = diff.tv_sec > 24 * 60 * 60 ?
+ 24 * 60 * 60 * hz : tstohz(&diff);
+ }
+ s = splhigh();
+ if ((p->p_flag & P_SELECT) == 0 || nselcoll != selcookie) {
+ splx(s);
+ selabort(p);
+ return (0);
+ }
+ atomic_clearbits_int(&p->p_flag, P_SELECT);
+ error = tsleep(&selwait, PSOCK | PCATCH, "select", timo);
+ splx(s);
+ return (error);
+}
+
+/*
  * Do a wakeup when a selectable event occurs.
  */
 void
@@ -933,9 +974,8 @@ doppoll(struct proc *p, struct pollfd *f
 {
  size_t sz;
  struct pollfd pfds[4], *pl = pfds;
- struct timespec ats, rts, tts;
- int timo, ncoll, i, s, error;
- extern int nselcoll, selwait;
+ struct timespec deadline;
+ int i, error;
 
  /* Standards say no more than MAX_OPEN; this is possibly better. */
  if (nfds > min((int)p->p_rlimit[RLIMIT_NOFILE].rlim_cur, maxfiles))
@@ -953,45 +993,27 @@ doppoll(struct proc *p, struct pollfd *f
  for (i = 0; i < nfds; i++)
  pl[i].revents = 0;
 
- if (tsp != NULL) {
- getnanouptime(&rts);
- timespecadd(tsp, &rts, &ats);
- } else {
- ats.tv_sec = 0;
- ats.tv_nsec = 0;
+ if (tsp) {
+ struct timespec now;
+ getnanouptime(&now);
+ timespecadd(&now, tsp, &deadline);
  }
- timo = 0;
 
  if (sigmask)
  dosigsuspend(p, *sigmask &~ sigcantmask);
 
-retry:
- ncoll = nselcoll;
- atomic_setbits_int(&p->p_flag, P_SELECT);
- pollscan(p, pl, nfds, retval);
- if (*retval)
- goto done;
- if (tsp != NULL) {
- getnanouptime(&rts);
- if (timespeccmp(&rts, &ats, >=))
- goto done;
- timespecsub(&ats, &rts, &tts);
- timo = tts.tv_sec > 24 * 60 * 60 ?
- 24 * 60 * 60 * hz : tstohz(&tts);
- }
- s = splhigh();
- if ((p->p_flag & P_SELECT) == 0 || nselcoll != ncoll) {
- splx(s);
- goto retry;
+ for (;;) {
+ int selcookie = selsetup(p);
+ pollscan(p, pl, nfds, retval);
+ if (*retval) {
+ selabort(p);
+ break;
+ }
+ error = selsleep(p, selcookie, tsp ? &deadline : NULL);
+ if (error)
+ break;
  }
- atomic_clearbits_int(&p->p_flag, P_SELECT);
- error = tsleep(&selwait, PSOCK | PCATCH, "poll", timo);
- splx(s);
- if (error == 0)
- goto retry;
 
-done:
- atomic_clearbits_int(&p->p_flag, P_SELECT);
  /*
  * NOTE: poll(2) is not restarted after a signal and EWOULDBLOCK is
  *       ignored (since the whole point is to see what would block).

Reply | Threaded
Open this post in threaded view
|

Re: Refactor dopselect() and doppoll()

Philip Guenther-2
On Sun, Jul 13, 2014 at 12:40 AM, Matthew Dempsky <[hidden email]>
wrote:

> Currently there's a lot of redundancy between dopselect() and
> doppoll().  This diff cleans them up in the following ways:
>
>   - Better variable names.  Instead of "rts", "ats", and "tts" they're
>     now called "deadline", "now", and "diff"; and "ncoll" is now
>     "selcookie".  They're also all more minimally scoped.
>
>   - Prefer loops over goto.
>
>   - Loosely styled after kern_synch.c's sleep_{setup,finish}() APIs,
>     there are now selsetup(), selabort(), and selsleep() methods that
>     dopselect() and doppoll() are implemented in terms of.  Moreover,
>     "P_SELECT", "nselcoll", and "selwait" are now only accessed within
>     sel{setup,record,abort,sleep,wakeup}(), making it easier to update
>     and review their interactions.
>
>   - poll() and ppoll() now show up in "ps" as "select" instead of
>     "poll".  This is just for simplicity because I don't think
>     distinguishing them offers much value when they're basically just
>     different APIs for the same underlying logic, but it's easy to
>     restore if people care.
>
> Other than the last point, this should be behavior preserving.
> Followup changes will tweak things a bit further, but having this diff
> in place should make them easier to review.
>
> ok?
>

Makes sense to me, though I would use a do {} while (error==0); loop
instead of for(;;){<...> if (error) break;}


Philip Guenther