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'll prod for OK next week, so if you've tested the diff please let me
know!
>
> 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 -0000 1.91
> +++ virtio.c 23 Jun 2021 11:28:03 -0000
> @@ -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_dev in a separate thread, this function still needs to be
> - * called on VCPU exit: it can happen that not all data fits into the
> - * receive queue of the vionet_dev immediately. So any outstanding data
> - * is handled here.
> - *
> - * Parameters:
> - * vm_id: VM ID of the VM for which to process vionet events
> - */
> -void
> -vionet_process_rx(uint32_t vm_id)
> -{
> - int i;
> -
> - for (i = 0 ; i < nr_vionet; i++) {
> - mutex_lock(&vionet[i].mutex);
> - if (!vionet[i].rx_added) {
> - mutex_unlock(&vionet[i].mutex);
> - continue;
> - }
> -
> - if (vionet[i].rx_pending) {
> - if (vionet_rx(&vionet[i])) {
> - vcpu_assert_pic_irq(vm_id, 0, vionet[i].irq);
> - }
> - }
> - mutex_unlock(&vionet[i].mutex);
> - }
> -}
> -
> -/*
> * Must be called with dev->mutex acquired.
> */
> void
> @@ -1383,7 +1335,6 @@ vionet_notify_rx(struct vionet_dev *dev)
> /* Compute offset into avail ring */
> avail = (struct vring_avail *)(vr + dev->vq[RXQ].vq_availoffset);
>
> - dev->rx_added = 1;
> dev->vq[RXQ].notified_avail = avail->idx - 1;
>
> free(vr);
> @@ -1937,7 +1888,6 @@ virtio_init(struct vmd_vm *vm, int child
> vionet[i].vq[TXQ].last_avail = 0;
> vionet[i].vq[TXQ].notified_avail = 0;
> vionet[i].fd = child_taps[i];
> - vionet[i].rx_pending = 0;
> vionet[i].vm_id = vcp->vcp_id;
> vionet[i].vm_vmid = vm->vm_vmid;
> vionet[i].irq = pci_get_dev_irq(id);
> @@ -2222,7 +2172,6 @@ vionet_restore(int fd, struct vmd_vm *vm
> return (-1);
> }
> vionet[i].fd = child_taps[i];
> - vionet[i].rx_pending = 0;
> vionet[i].vm_id = vcp->vcp_id;
> vionet[i].vm_vmid = vm->vm_vmid;
> vionet[i].irq = pci_get_dev_irq(vionet[i].pci_id);
> Index: virtio.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.h,v
> retrieving revision 1.40
> diff -u -p -r1.40 virtio.h
> --- virtio.h 21 Jun 2021 02:38:18 -0000 1.40
> +++ virtio.h 23 Jun 2021 11:28:03 -0000
> @@ -217,8 +217,7 @@ struct vionet_dev {
>
> struct virtio_vq_info vq[VIRTIO_MAX_QUEUES];
>
> - int fd, rx_added;
> - int rx_pending;
> + int fd;
> uint32_t vm_id;
> uint32_t vm_vmid;
> int irq;
> Index: vm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vm.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 vm.c
> --- vm.c 16 Jun 2021 16:55:02 -0000 1.63
> +++ vm.c 23 Jun 2021 11:28:03 -0000
> @@ -1711,9 +1711,6 @@ vcpu_exit(struct vm_run_params *vrp)
> __progname, vrp->vrp_exit_reason);
> }
>
> - /* Process any pending traffic */
> - vionet_process_rx(vrp->vrp_vm_id);
> -
> vrp->vrp_continue = 1;
>
> return (0);
>
>
> Index: ns8250.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/ns8250.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 ns8250.c
> --- ns8250.c 16 Jun 2021 16:55:02 -0000 1.31
> +++ ns8250.c 24 Jun 2021 02:20:10 -0000
> @@ -36,14 +36,10 @@
> extern char *__progname;
> struct ns8250_dev com1_dev;
>
> -/* Flags to distinguish calling threads to com_rcv */
> -#define NS8250_DEV_THREAD 0
> -#define NS8250_CPU_THREAD 1
> -
> static struct vm_dev_pipe dev_pipe;
>
> static void com_rcv_event(int, short, void *);
> -static void com_rcv(struct ns8250_dev *, uint32_t, uint32_t, int);
> +static void com_rcv(struct ns8250_dev *, uint32_t, uint32_t);
>
> /*
> * ns8250_pipe_dispatch
> @@ -58,11 +54,6 @@ ns8250_pipe_dispatch(int fd, short event
>
> msg = vm_pipe_recv(&dev_pipe);
> switch(msg) {
> - case NS8250_ZERO_READ:
> - log_debug("%s: resetting events after zero byte read",
> __func__);
> - event_del(&com1_dev.event);
> - event_add(&com1_dev.wake, NULL);
> - break;
> case NS8250_RATELIMIT:
> evtimer_add(&com1_dev.rate, &com1_dev.rate_tv);
> break;
> @@ -110,7 +101,6 @@ ns8250_init(int fd, uint32_t vmid)
> com1_dev.fd = fd;
> com1_dev.irq = 4;
> com1_dev.portid = NS8250_COM1;
> - com1_dev.rcv_pending = 0;
> com1_dev.vmid = vmid;
> com1_dev.byte_out = 0;
> com1_dev.regs.divlo = 1;
> @@ -163,17 +153,8 @@ com_rcv_event(int fd, short kind, void *
> return;
> }
>
> - /*
> - * We already have other data pending to be received. The data that
> - * has become available now will be moved to the com port later by
> - * the vcpu.
> - */
> - if (!com1_dev.rcv_pending) {
> - if (com1_dev.regs.lsr & LSR_RXRDY)
> - com1_dev.rcv_pending = 1;
> - else
> - com_rcv(&com1_dev, (uintptr_t)arg, 0,
> NS8250_DEV_THREAD);
> - }
> + if ((com1_dev.regs.lsr & LSR_RXRDY) == 0)
> + com_rcv(&com1_dev, (uintptr_t)arg, 0);
>
> /* If pending interrupt, inject */
> if ((com1_dev.regs.iir & IIR_NOPEND) == 0) {
> @@ -216,7 +197,7 @@ com_rcv_handle_break(struct ns8250_dev *
> * Must be called with the mutex of the com device acquired
> */
> static void
> -com_rcv(struct ns8250_dev *com, uint32_t vm_id, uint32_t vcpu_id, int thread)
> +com_rcv(struct ns8250_dev *com, uint32_t vm_id, uint32_t vcpu_id)
> {
> char buf[2];
> ssize_t sz;
> @@ -236,13 +217,9 @@ com_rcv(struct ns8250_dev *com, uint32_t
> if (errno != EAGAIN)
> log_warn("unexpected read error on com device");
> } else if (sz == 0) {
> - if (thread == NS8250_DEV_THREAD) {
> - event_del(&com->event);
> - event_add(&com->wake, NULL);
> - } else {
> - /* Called by vcpu thread, use pipe for event changes */
> - vm_pipe_send(&dev_pipe, NS8250_ZERO_READ);
> - }
> + /* Zero read typically occurs on a disconnect */
> + event_del(&com->event);
> + event_add(&com->wake, NULL);
> return;
> } else if (sz != 1 && sz != 2)
> log_warnx("unexpected read return value %zd on com device", sz);
> @@ -258,8 +235,6 @@ com_rcv(struct ns8250_dev *com, uint32_t
> com->regs.iir &= ~IIR_NOPEND;
> }
> }
> -
> - com->rcv_pending = fd_hasdata(com->fd);
> }
>
> /*
> @@ -337,9 +312,6 @@ vcpu_process_com_data(struct vm_exit *ve
> */
> if (com1_dev.regs.iir == 0x0)
> com1_dev.regs.iir = 0x1;
> -
> - if (com1_dev.rcv_pending)
> - com_rcv(&com1_dev, vm_id, vcpu_id, NS8250_CPU_THREAD);
> }
>
> /* If pending interrupt, make sure it gets injected */
> @@ -688,7 +660,6 @@ ns8250_restore(int fd, int con_fd, uint3
> com1_dev.fd = con_fd;
> com1_dev.irq = 4;
> com1_dev.portid = NS8250_COM1;
> - com1_dev.rcv_pending = 0;
> com1_dev.vmid = vmid;
> com1_dev.byte_out = 0;
> com1_dev.regs.divlo = 1;
> Index: ns8250.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/ns8250.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 ns8250.h
> --- ns8250.h 11 Dec 2019 06:45:16 -0000 1.9
> +++ ns8250.h 24 Jun 2021 02:20:10 -0000
> @@ -69,7 +69,6 @@ struct ns8250_dev {
> enum ns8250_portid portid;
> int fd;
> int irq;
> - int rcv_pending;
> uint32_t vmid;
> uint64_t byte_out;
> uint32_t baudrate;
--
-Dave Voutila
