On Sat, Dec 26, 2020 at 04:48:23PM -0600, Scott Cheloha wrote:
> Now that we've removed bd_rdStart from the bpf_d struct, removing
> ticks from bpf(4) itself is straightforward.
>
> - bd_rtout becomes a timespec; update bpfioctl() accordingly.
> Cap it at MAXTSLP nanoseconds to avoid arithmetic overflow
> in bpfread().
>
> - At the start of bpfread(), if a timeout is set, find the end
> of the read as an absolute uptime. This is the point where
> we want to avoid overflow: if bd_rtout is only MAXTSLP
> nanoseconds the timespecadd(3) will effectively never overflow.
>
> - Before going to sleep, if we have a timeout set, compute how
> much longer to sleep in nanoseconds.
>
> Here's a spot where an absolute timeout sleep would save a
> little code, but we don't have such an interface yet. Worth
> keeping in mind for the future, though.
Are there any other places that would be useful though? bpf is pretty
special.
> dlg@: You said you wanted to simplify this loop. Unsure what shape
> that cleanup would take. Should we clean it up before this change?
I wanted to have a single msleep_nsec call and pass INFSLP when it
should sleep forever.. You saw my first attempt at that. It had
issues.
> Thoughts? ok?
How would this look if you used a uint64_t for nsecs for bd_rtout,
and the nsec uptime thing from your pool diff instead of timespecs
and nanouptime?
What's the thinking behind nanouptime instead of getnanouptime? More
generally, what will getnanouptime do when ticks go away?
dlg
>
> Index: bpf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/bpf.c,v
> retrieving revision 1.199
> diff -u -p -r1.199 bpf.c
> --- bpf.c 26 Dec 2020 16:30:58 -0000 1.199
> +++ bpf.c 26 Dec 2020 22:05:04 -0000
> @@ -60,6 +60,7 @@
> #include <sys/selinfo.h>
> #include <sys/sigio.h>
> #include <sys/task.h>
> +#include <sys/time.h>
>
> #include <net/if.h>
> #include <net/bpf.h>
> @@ -77,9 +78,6 @@
>
> #define PRINET 26 /* interruptible */
>
> -/* from kern/kern_clock.c; incremented each clock tick. */
> -extern int ticks;
> -
> /*
> * The default read buffer size is patchable.
> */
> @@ -380,7 +378,7 @@ bpfopen(dev_t dev, int flag, int mode, s
> smr_init(&bd->bd_smr);
> sigio_init(&bd->bd_sigio);
>
> - bd->bd_rtout = 0; /* no timeout by default */
> + timespecclear(&bd->bd_rtout); /* no timeout by default */
> bd->bd_rnonblock = ISSET(flag, FNONBLOCK);
>
> bpf_get(bd);
> @@ -428,9 +426,11 @@ bpfclose(dev_t dev, int flag, int mode,
> int
> bpfread(dev_t dev, struct uio *uio, int ioflag)
> {
> + struct timespec diff, end, now;
> + uint64_t nsecs;
> struct bpf_d *d;
> caddr_t hbuf;
> - int end, error, hlen, nticks;
> + int error, hlen, timeout;
>
> KERNEL_ASSERT_LOCKED();
>
> @@ -453,8 +453,11 @@ bpfread(dev_t dev, struct uio *uio, int
> /*
> * If there's a timeout, mark when the read should end.
> */
> - if (d->bd_rtout)
> - end = ticks + (int)d->bd_rtout;
> + timeout = timespecisset(&d->bd_rtout);
> + if (timeout) {
> + nanouptime(&now);
> + timespecadd(&now, &d->bd_rtout, &end);
> + }
>
> /*
> * If the hold buffer is empty, then do a timed sleep, which
> @@ -483,21 +486,26 @@ bpfread(dev_t dev, struct uio *uio, int
> if (d->bd_rnonblock) {
> /* User requested non-blocking I/O */
> error = EWOULDBLOCK;
> - } else if (d->bd_rtout == 0) {
> + } else if (timeout == 0) {
> /* No read timeout set. */
> d->bd_nreaders++;
> error = msleep_nsec(d, &d->bd_mtx, PRINET|PCATCH,
> "bpf", INFSLP);
> d->bd_nreaders--;
> - } else if ((nticks = end - ticks) > 0) {
> - /* Read timeout has not expired yet. */
> - d->bd_nreaders++;
> - error = msleep(d, &d->bd_mtx, PRINET|PCATCH, "bpf",
> - nticks);
> - d->bd_nreaders--;
> } else {
> - /* Read timeout has expired. */
> - error = EWOULDBLOCK;
> + nanouptime(&now);
> + if (timespeccmp(&now, &end, <)) {
> + /* Read timeout has not expired yet. */
> + timespecsub(&end, &now, &diff);
> + nsecs = TIMESPEC_TO_NSEC(&diff);
> + d->bd_nreaders++;
> + error = msleep_nsec(d, &d->bd_mtx,
> + PRINET|PCATCH, "bpf", nsecs);
> + d->bd_nreaders--;
> + } else {
> + /* Read timeout has expired. */
> + error = EWOULDBLOCK;
> + }
> }
> if (error == EINTR || error == ERESTART)
> goto out;
> @@ -861,27 +869,17 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
> case BIOCSRTIMEOUT:
> {
> struct timeval *tv = (struct timeval *)addr;
> - u_long rtout;
>
> - /* Compute number of ticks. */
> if (tv->tv_sec < 0 || !timerisvalid(tv)) {
> error = EINVAL;
> break;
> }
> - if (tv->tv_sec > INT_MAX / hz) {
> - error = EOVERFLOW;
> - break;
> - }
> - rtout = tv->tv_sec * hz;
> - if (tv->tv_usec / tick > INT_MAX - rtout) {
> + if (TIMEVAL_TO_NSEC(tv) > MAXTSLP) {
> error = EOVERFLOW;
> break;
> }
> - rtout += tv->tv_usec / tick;
> mtx_enter(&d->bd_mtx);
> - d->bd_rtout = rtout;
> - if (d->bd_rtout == 0 && tv->tv_usec != 0)
> - d->bd_rtout = 1;
> + TIMEVAL_TO_TIMESPEC(tv, &d->bd_rtout);
> mtx_leave(&d->bd_mtx);
> break;
> }
> @@ -893,9 +891,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
> {
> struct timeval *tv = (struct timeval *)addr;
>
> + memset(tv, 0, sizeof(*tv));
> mtx_enter(&d->bd_mtx);
> - tv->tv_sec = d->bd_rtout / hz;
> - tv->tv_usec = (d->bd_rtout % hz) * tick;
> + TIMESPEC_TO_TIMEVAL(tv, &d->bd_rtout);
> mtx_leave(&d->bd_mtx);
> break;
> }
> Index: bpfdesc.h
> ===================================================================
> RCS file: /cvs/src/sys/net/bpfdesc.h,v
> retrieving revision 1.43
> diff -u -p -r1.43 bpfdesc.h
> --- bpfdesc.h 26 Dec 2020 16:30:58 -0000 1.43
> +++ bpfdesc.h 26 Dec 2020 22:05:04 -0000
> @@ -78,7 +78,7 @@ struct bpf_d {
> int bd_in_uiomove; /* for debugging purpose */
>
> struct bpf_if *bd_bif; /* interface descriptor */
> - u_long bd_rtout; /* [m] Read timeout in 'ticks' */
> + struct timespec bd_rtout; /* [m] Read timeout if non-zero */
> u_long bd_nreaders; /* [m] # threads asleep in bpfread() */
> int bd_rnonblock; /* true if nonblocking reads are set */
> struct bpf_program_smr
