Re: vmd(8): simplify vcpu logic, removing uart & net reads
On Sun, Jul 11, 2021 at 08:10:42AM -0400, Dave Voutila wrote:
>
> Ping...looking for OK. Would like to get this committed this week.
>
Sorry this took so long. ok mlarkin.
Thanks to the numerous testers who ran with this for the past few
weeks.
> Dave Voutila writes:
>
> > Looking for an OK for this one now. Anyone?
> >
> > Dave Voutila writes:
> >
> >> Dave Voutila writes:
> >>
> >>> Looking for some broader testing of the following diff. It cleans up
> >>> some complicated logic predominantly left over from the early days of
> >>> vmd prior to its having a dedicated device thread.
> >>
> >> Still looking for tester feedback. I've been running this diff while
> >> hosting multiple guests continously (OpenBSD-current, Alpine 3.14,
> >> Debian 10.10, Ubuntu 20.04) with no issues.
> >>
> >> I know a few folks have told me they've applied the diff and have not
> >> seen issues.
> >
> > I've had positive reports from 4 people. Thanks everyone that tested and
> > provided feedback!
> >
> >>
> >> I'll prod for OK next week, so if you've tested the diff please let me
> >> know!
> >
> > OK to commit?
> >
> >>
> >>>
> >>> In summary, this diff:
> >>>
> >>> - Removes vionet "rx pending" state handling and removes the code path
> >>> for the vcpu thread to possibly take control of the virtio net device
> >>> and attempt a read of the underlying tap(4). (virtio.{c,h}, vm.c)
> >>>
> >>> - Removes ns8250 "rcv pending" state handling and removes the code path
> >>> for the vcpu thread to read the pty via com_rcv(). (ns8250.{c,h})
> >>>
> >>> In both of the above cases, the event handling thread will be notified
> >>> of readable data and deal with it.
> >>>
> >>> Why remove them? The logic is overly complicated and hard to reason
> >>> about for zero gain. (This diff results in no intended functional
> >>> change.) Plus, some of the above logic I helped add to deal with the
> >>> race conditions and state corruption over a year ago. The logic was
> >>> needed once upon a time, but shouldn't be needed at present.
> >>>
> >>> I've had positive testing feedback from abieber@ so far with at least
> >>> the ns8250/uart diff, but want to cast a broader net here with both
> >>> before either part is committed. I debated splitting these up, but
> >>> they're thematically related.
> >>>
> >>> -dv
> >>>
> >>> Index: virtio.c
> >>> ===
> >>> RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> >>> retrieving revision 1.91
> >>> diff -u -p -r1.91 virtio.c
> >>> --- virtio.c 21 Jun 2021 02:38:18 - 1.91
> >>> +++ virtio.c 23 Jun 2021 11:28:03 -
> >>> @@ -1254,12 +1254,12 @@ static int
> >>> vionet_rx(struct vionet_dev *dev)
> >>> {
> >>> char buf[PAGE_SIZE];
> >>> - int hasdata, num_enq = 0, spc = 0;
> >>> + int num_enq = 0, spc = 0;
> >>> struct ether_header *eh;
> >>> ssize_t sz;
> >>>
> >>> do {
> >>> - sz = read(dev->fd, buf, sizeof buf);
> >>> + sz = read(dev->fd, buf, sizeof(buf));
> >>> if (sz == -1) {
> >>> /*
> >>>* If we get EAGAIN, No data is currently available.
> >>> @@ -1270,21 +1270,17 @@ vionet_rx(struct vionet_dev *dev)
> >>> "device");
> >>> } else if (sz > 0) {
> >>> eh = (struct ether_header *)buf;
> >>> - if (!dev->lockedmac || sz < ETHER_HDR_LEN ||
> >>> + if (!dev->lockedmac ||
> >>> ETHER_IS_MULTICAST(eh->ether_dhost) ||
> >>> memcmp(eh->ether_dhost, dev->mac,
> >>> sizeof(eh->ether_dhost)) == 0)
> >>> num_enq += vionet_enq_rx(dev, buf, sz, &spc);
> >>> } else if (sz == 0) {
> >>> log_debug("process_rx: no data");
> >>> - hasdata = 0;
> >>> break;
> >>> }
> >>> + } while (spc > 0 && sz > 0);
> >>>
> >>> - hasdata = fd_hasdata(dev->fd);
> >>> - } while (spc && hasdata);
> >>> -
> >>> - dev->rx_pending = hasdata;
> >>> return (num_enq);
> >>> }
> >>>
> >>> @@ -1301,16 +1297,6 @@ vionet_rx_event(int fd, short kind, void
> >>>
> >>> mutex_lock(&dev->mutex);
> >>>
> >>> - /*
> >>> - * We already have other data pending to be received. The data that
> >>> - * has become available now will be enqueued to the vionet_dev
> >>> - * later.
> >>> - */
> >>> - if (dev->rx_pending) {
> >>> - mutex_unlock(&dev->mutex);
> >>> - return;
> >>> - }
> >>> -
> >>> if (vionet_rx(dev) > 0) {
> >>> /* XXX: vcpu_id */
> >>> vcpu_assert_pic_irq(dev->vm_id, 0, dev->irq);
> >>> @@ -1320,40 +1306,6 @@ vionet_rx_event(int fd, short kind, void
> >>> }
> >>>
> >>> /*
> >>> - * vionet_process_rx
> >>> - *
> >>> - * Processes any remaining pending receivable data for a vionet device.
> >>> - * Called on VCPU exit. Although we poll on the tap file descriptor of
> >>> - * a vionet_
Re: cron(8): add '@' interval time specifier
On Thu, Jul 15, 2021 at 11:20 AM Leo Unglaub wrote: > Hey, > this is a very clean solution to a very common problem that i have. A > lot of tasks that i run from cron have very different completion times. > Right now i use the -s [1] option in crontab to make sure only one task > is running at once and so that they don't overlap if they take longer to > complete than the interval i schedule them. But the problem with the -s > workflow is that the interval can get messed up if the interval is > smaller than the execution period. This will basically "skip" a run. > > With your patch this can be avioded in a very clean way. I have your > patch a try and for my local testing usecase it worked as expected. This > would clean up a lot of things in my personal workflow. Thank you very > mich for the patch Job, much appreciated! > > Thanks and greetings > Leo > > [1] https://man.openbsd.org/crontab.5#s > > On 10/07/2021 16:07, Job Snijders wrote: > > The below patch adds a new kind of time specifier: an interval (in > > minutes). When used, cron(8) will schedule the next instance of a job > > after the previous job has completed and a full interval has passed. > > > > A crontab(5) configured as following: > > > > $ crontab -l > > @3 sleep 100 > > > > Will result in this schedule: > > > > Jul 10 13:38:17 vurt cron[96937]: (CRON) STARTUP (V5.0) > > Jul 10 13:42:01 vurt cron[79385]: (job) CMD (sleep 100) > > Jul 10 13:47:01 vurt cron[3165]: (job) CMD (sleep 100) > > Jul 10 13:52:01 vurt cron[40539]: (job) CMD (sleep 100) > > Jul 10 13:57:01 vurt cron[84504]: (job) CMD (sleep 100) > > > > A use case could be running rpki-client more frequently than once an > > hour: > > > > @15 -n rpki-client && bgpctl reload > > > > The above is equivalent to: > > > > * * * * * -sn sleep 900 && rpki-client && bgpctl reload > > > > I borrowed the idea from FreeBSD's cron [1]. A difference between the > > below changeset and the freebsd implementation is that they specify the > > interval in seconds, while the below specifies in minutes. I was able > > to leverage the 'singleton' infrastructure. And removed a comment that > > reads like a TODO nobody is going to do. > > > > Thoughts? > Can't this type of problem also be solved with a shell script wrapper? An infinite loop that does work and then sleeps? Or perhaps a script that upon work completion, schedules itself in the future with at(1)? These could be kicked off initially with a @reboot crontab entry. Or maybe I'm missing something that cron adds to the equation, in which case, apologies. --david
Re: cron(8): add '@' interval time specifier
Hey, this is a very clean solution to a very common problem that i have. A lot of tasks that i run from cron have very different completion times. Right now i use the -s [1] option in crontab to make sure only one task is running at once and so that they don't overlap if they take longer to complete than the interval i schedule them. But the problem with the -s workflow is that the interval can get messed up if the interval is smaller than the execution period. This will basically "skip" a run. With your patch this can be avioded in a very clean way. I have your patch a try and for my local testing usecase it worked as expected. This would clean up a lot of things in my personal workflow. Thank you very mich for the patch Job, much appreciated! Thanks and greetings Leo [1] https://man.openbsd.org/crontab.5#s On 10/07/2021 16:07, Job Snijders wrote: The below patch adds a new kind of time specifier: an interval (in minutes). When used, cron(8) will schedule the next instance of a job after the previous job has completed and a full interval has passed. A crontab(5) configured as following: $ crontab -l @3 sleep 100 Will result in this schedule: Jul 10 13:38:17 vurt cron[96937]: (CRON) STARTUP (V5.0) Jul 10 13:42:01 vurt cron[79385]: (job) CMD (sleep 100) Jul 10 13:47:01 vurt cron[3165]: (job) CMD (sleep 100) Jul 10 13:52:01 vurt cron[40539]: (job) CMD (sleep 100) Jul 10 13:57:01 vurt cron[84504]: (job) CMD (sleep 100) A use case could be running rpki-client more frequently than once an hour: @15 -n rpki-client && bgpctl reload The above is equivalent to: * * * * * -sn sleep 900 && rpki-client && bgpctl reload I borrowed the idea from FreeBSD's cron [1]. A difference between the below changeset and the freebsd implementation is that they specify the interval in seconds, while the below specifies in minutes. I was able to leverage the 'singleton' infrastructure. And removed a comment that reads like a TODO nobody is going to do. Thoughts?
