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.
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;
