Re: vmd(8): simplify vcpu logic, removing uart & net reads
Mike Larkin writes: > 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. > Committed. Thanks again, testers! -dv
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: vmd(8): simplify vcpu logic, removing uart & net reads
Ping...looking for OK. Would like to get this committed this week.
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.c21 Jun 2021 02:38:18 - 1.91
>>> +++ virtio.c23 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_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
>>> - */
>>
Re: vmd(8): simplify vcpu logic, removing uart & net reads
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_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_adde
Re: vmd(8): simplify vcpu logic, removing uart & net reads
Hi Dave,
> On 3 Jul 2021, at 19:08, Matthias Schmidt wrote:
>
> Hi Dave,
>
> * Dave Voutila wrote:
>> 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.
>
> I have the diff running since one week on -current with stable/current
> and an Archlinux guest and have noticed no regression so far.
>
> Cheers
>
> Matthias
No issues on my side as well.
Mischa
Re: vmd(8): simplify vcpu logic, removing uart & net reads
Hi Dave,
* Dave Voutila wrote:
> 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.
I have the diff running since one week on -current with stable/current
and an Archlinux guest and have noticed no regression so far.
Cheers
Matthias
Re: vmd(8): simplify vcpu logic, removing uart & net reads
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 - 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_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(
vmd(8): simplify vcpu logic, removing uart & net reads
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.c21 Jun 2021 02:38:18 - 1.91
+++ virtio.c23 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_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 +18
