[Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Catalin Vasile]

What is the logic behind vring_need_event() when used with
virtqueue_kick_prepare()?
What does the keyword >>just<< refer to from the following context:
/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
/* Assuming a given event_idx value from the other size, if
 * we have just incremented index from old to new_idx,
 * should we trigger an event? */
?
I am sending 2 jobs, one after another, and the second one just does
not want to kick, although the first one finished completely and the
backend went back to interrupt mode, all because vring_need_event()
returns false.

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

On Mon, Jul 06, 2015 at 06:13:29PM +0300, Catalin Vasile wrote:
> What is the logic behind vring_need_event() when used with
> virtqueue_kick_prepare()?
> What does the keyword >>just<< refer to from the following context:
> /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
> /* Assuming a given event_idx value from the other size, if
>  * we have just incremented index from old to new_idx,
>  * should we trigger an event? */
> ?

"just" means since the last time the host/guest-visible index field was
changed.  After avail or used rings have been processed, the index field
for that ring is published to the host/guest.  At that point a check is
made whether the other side needs to be kicked.

> I am sending 2 jobs, one after another, and the second one just does
> not want to kick, although the first one finished completely and the
> backend went back to interrupt mode, all because vring_need_event()
> returns false.

Maybe the vhost driver called vhost_disable_notify() and hasn't
re-enabled notify yet?

This could happen if the guest adds buffers to the virtqueue while the
host is processing the virtqueue.  Take a look at the vhost_net code for
how to correctly disable and re-enable notify without race conditions on
the host.

The idea behind disabling notify is to eliminate unnecessary
vmexits/notifications since the host is already processing the virtqueue
and will see new buffers.  It's like a polling vs interrupt mode.

If the vhost driver on the host doesn't implement it correctly, then the
device could stop responding to the avail ring.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Catalin Vasile]

My vhost module respects the format vhost-net uses:

/* <code> summary */
mutex_lock(&vq->mutex);
vhost_disable_notify();
for (;;) {
        head = vhost_get_vq_desc();
       if (head == vq->num) {
            if (unlikely(vhost_enable_notify())) {
                vhost_disable_notify();
                continue;
            }
            break;
       }
       vhost_add_used_and_signal();
}
mutex_unlock(&vq->mutex);
/* </code> */

I have made a lot of printk() calls and the first job gets processed
completely, and gets through all those calls:
1. it goes into a first loop and processes the first job (get
descriptor, work with the descriptor, add used and signal).
2. On the second loop it hits head == vq->num, and goes back to
listening to notifications (successfully, it does not get into the
fallback).

Now in the guest:
1. sends first job and the paramers used to call vring_need_event() are:
vring_avail_event=0, new=1, old=0 (which makes the function evaluate to "0 < 1")
2. the queue is kicked and vhost does its job.
3. the guest driver reaches the end of the first job, and lets the
following job take its course, only this time vring_need_event()
receives the following parameters:
vring_avail_event=0, new=2, old=1 (which makes the function evaluate to "1 < 1")
so a kick is not actually sent because vring_need_event() returns
false. From what I see as the definition for vring_need_event(), it
does not actually look at flags.
"if (vq->event) {" evaluates to true in both cases, so it always
verifies those indexes (it does not go on the branch which verifies
flags).
I am also pretty sure the jobs are serialized in the guest driver, and
do not cross each other's path. One of the reasons is that every
function that sends a job must hold a mutex that protects the
virtqueue.
The guest driver blocks awaiting an interrupt for the job being
finished, but vhost does not get woken up to process the job in the
first places, because a notification is not actually triggered because
of what I have explained above.

On Tue, Jul 7, 2015 at 1:17 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 06, 2015 at 06:13:29PM +0300, Catalin Vasile wrote:
>> What is the logic behind vring_need_event() when used with
>> virtqueue_kick_prepare()?
>> What does the keyword >>just<< refer to from the following context:
>> /* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
>> /* Assuming a given event_idx value from the other size, if
>>  * we have just incremented index from old to new_idx,
>>  * should we trigger an event? */
>> ?
>
> "just" means since the last time the host/guest-visible index field was
> changed.  After avail or used rings have been processed, the index field
> for that ring is published to the host/guest.  At that point a check is
> made whether the other side needs to be kicked.
>
>> I am sending 2 jobs, one after another, and the second one just does
>> not want to kick, although the first one finished completely and the
>> backend went back to interrupt mode, all because vring_need_event()
>> returns false.
>
> Maybe the vhost driver called vhost_disable_notify() and hasn't
> re-enabled notify yet?
>
> This could happen if the guest adds buffers to the virtqueue while the
> host is processing the virtqueue.  Take a look at the vhost_net code for
> how to correctly disable and re-enable notify without race conditions on
> the host.
>
> The idea behind disabling notify is to eliminate unnecessary
> vmexits/notifications since the host is already processing the virtqueue
> and will see new buffers.  It's like a polling vs interrupt mode.
>
> If the vhost driver on the host doesn't implement it correctly, then the
> device could stop responding to the avail ring.

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

On Tue, Jul 07, 2015 at 02:43:06PM +0300, Catalin Vasile wrote:
> My vhost module respects the format vhost-net uses:
> 
> /* <code> summary */
> mutex_lock(&vq->mutex);
> vhost_disable_notify();
> for (;;) {
>         head = vhost_get_vq_desc();
>        if (head == vq->num) {
>             if (unlikely(vhost_enable_notify())) {
>                 vhost_disable_notify();
>                 continue;
>             }
>             break;
>        }
>        vhost_add_used_and_signal();
> }
> mutex_unlock(&vq->mutex);
> /* </code> */
> 
> I have made a lot of printk() calls and the first job gets processed
> completely, and gets through all those calls:
> 1. it goes into a first loop and processes the first job (get
> descriptor, work with the descriptor, add used and signal).
> 2. On the second loop it hits head == vq->num, and goes back to
> listening to notifications (successfully, it does not get into the
> fallback).
> 
> Now in the guest:
> 1. sends first job and the paramers used to call vring_need_event() are:
> vring_avail_event=0, new=1, old=0 (which makes the function evaluate to "0 < 1")
> 2. the queue is kicked and vhost does its job.
> 3. the guest driver reaches the end of the first job, and lets the
> following job take its course, only this time vring_need_event()
> receives the following parameters:
> vring_avail_event=0, new=2, old=1 (which makes the function evaluate to "1 < 1")

This means you need to look at the vhost code because vring_avail_event
shouldn't be 0.

Why wasn't vhost_update_avail_event() called?

Maybe the feature bit negotiation for your device is broken and vhost
thinks VIRTIO_RING_F_EVENT_IDX is not set.

Or does the used ring have the VRING_USED_F_NO_NOTIFY flag?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Catalin Vasile]

I have managed to deal with it.
The thing is I was using one of the latest versions of qemu, but an
old Linux Kernel version of 3.12.

On Thu, Jul 9, 2015 at 1:43 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 07, 2015 at 02:43:06PM +0300, Catalin Vasile wrote:
>> My vhost module respects the format vhost-net uses:
>>
>> /* <code> summary */
>> mutex_lock(&vq->mutex);
>> vhost_disable_notify();
>> for (;;) {
>>         head = vhost_get_vq_desc();
>>        if (head == vq->num) {
>>             if (unlikely(vhost_enable_notify())) {
>>                 vhost_disable_notify();
>>                 continue;
>>             }
>>             break;
>>        }
>>        vhost_add_used_and_signal();
>> }
>> mutex_unlock(&vq->mutex);
>> /* </code> */
>>
>> I have made a lot of printk() calls and the first job gets processed
>> completely, and gets through all those calls:
>> 1. it goes into a first loop and processes the first job (get
>> descriptor, work with the descriptor, add used and signal).
>> 2. On the second loop it hits head == vq->num, and goes back to
>> listening to notifications (successfully, it does not get into the
>> fallback).
>>
>> Now in the guest:
>> 1. sends first job and the paramers used to call vring_need_event() are:
>> vring_avail_event=0, new=1, old=0 (which makes the function evaluate to "0 < 1")
>> 2. the queue is kicked and vhost does its job.
>> 3. the guest driver reaches the end of the first job, and lets the
>> following job take its course, only this time vring_need_event()
>> receives the following parameters:
>> vring_avail_event=0, new=2, old=1 (which makes the function evaluate to "1 < 1")
>
> This means you need to look at the vhost code because vring_avail_event
> shouldn't be 0.
>
> Why wasn't vhost_update_avail_event() called?
>
> Maybe the feature bit negotiation for your device is broken and vhost
> thinks VIRTIO_RING_F_EVENT_IDX is not set.
>
> Or does the used ring have the VRING_USED_F_NO_NOTIFY flag?

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 315 bytes --]

On Thu, Jul 09, 2015 at 01:45:50PM +0300, Catalin Vasile wrote:
> I have managed to deal with it.
> The thing is I was using one of the latest versions of qemu, but an
> old Linux Kernel version of 3.12.

Old guest kernel or old host kernel?

New QEMU should support old guest kernels.  Maybe you have found a bug?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Catalin Vasile]

Both. The compiled kernel was common for both.

On Thu, Jul 16, 2015 at 3:52 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jul 09, 2015 at 01:45:50PM +0300, Catalin Vasile wrote:
>> I have managed to deal with it.
>> The thing is I was using one of the latest versions of qemu, but an
>> old Linux Kernel version of 3.12.
>
> Old guest kernel or old host kernel?
>
> New QEMU should support old guest kernels.  Maybe you have found a bug?

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Stefan Hajnoczi]

On Thu, Jul 16, 2015 at 1:54 PM, Catalin Vasile
<catalinvasile92@gmail.com> wrote:
> Both. The compiled kernel was common for both.

Does vhost_net work with the old kernel + new QEMU combo?

Stefan

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Catalin Vasile]

Do you mean vhost_net - old kernel, qemu - latest, guest - latest?

On Thu, Jul 16, 2015 at 7:33 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jul 16, 2015 at 1:54 PM, Catalin Vasile
> <catalinvasile92@gmail.com> wrote:
>> Both. The compiled kernel was common for both.
>
> Does vhost_net work with the old kernel + new QEMU combo?
>
> Stefan

Re: [Qemu-devel] [virtio guest] vring_need_event() from virtqueue_kick_prepare()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

On Fri, Jul 17, 2015 at 09:30:03AM +0300, Catalin Vasile wrote:
> Do you mean vhost_net - old kernel, qemu - latest, guest - latest?

I mean whatever combination didn't work for your custom vhost module.
If vhost_net is broken too there is probably a vhost/QEMU bug that
should be fixed.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]