Patchwork [uq/master,1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

login
register
mail settings
Submitter Marcelo Tosatti
Date Feb. 22, 2010, 1:59 p.m.
Message ID <20100222140209.878250600@amt.cnet>
Download mbox | patch
Permalink /patch/45962/
State New
Headers show

Comments

Marcelo Tosatti - Feb. 22, 2010, 1:59 p.m.
VIRTIO_PCI_QUEUE_NOTIFY is used to inform availability of new buffers,
so wakeup the iothread to process that information immediately.

Reported-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Avi Kivity - Feb. 22, 2010, 2:20 p.m.
On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> VIRTIO_PCI_QUEUE_NOTIFY is used to inform availability of new buffers,
> so wakeup the iothread to process that information immediately.
>
> Reported-by: Amit Shah<amit.shah@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> Index: qemu/hw/virtio-pci.c
> ===================================================================
> --- qemu.orig/hw/virtio-pci.c
> +++ qemu/hw/virtio-pci.c
> @@ -204,6 +204,7 @@ static void virtio_ioport_write(void *op
>           break;
>       case VIRTIO_PCI_QUEUE_NOTIFY:
>           virtio_queue_notify(vdev, val);
> +        qemu_notify_event();
>           break;
>    

virtio_queue_notify() will call ->handle_output(), which should either 
do what's needed to be done, or wake up some iothread itself.
Marcelo Tosatti - Feb. 22, 2010, 2:29 p.m.
On Mon, Feb 22, 2010 at 04:20:52PM +0200, Avi Kivity wrote:
> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> >VIRTIO_PCI_QUEUE_NOTIFY is used to inform availability of new buffers,
> >so wakeup the iothread to process that information immediately.
> >
> >Reported-by: Amit Shah<amit.shah@redhat.com>
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >Index: qemu/hw/virtio-pci.c
> >===================================================================
> >--- qemu.orig/hw/virtio-pci.c
> >+++ qemu/hw/virtio-pci.c
> >@@ -204,6 +204,7 @@ static void virtio_ioport_write(void *op
> >          break;
> >      case VIRTIO_PCI_QUEUE_NOTIFY:
> >          virtio_queue_notify(vdev, val);
> >+        qemu_notify_event();
> >          break;
> 
> virtio_queue_notify() will call ->handle_output(), which should
> either do what's needed to be done, or wake up some iothread itself.

kick is used to inform either output processing, in which case
->handle_output() does what its supposed to.

But its also used to inform availability of new buffers, which is common
to all virtio devices. So what is the point pushing this to
->handle_output?

Are you concerned about spurious wakeups?
Avi Kivity - Feb. 22, 2010, 2:51 p.m.
On 02/22/2010 04:29 PM, Marcelo Tosatti wrote:
> On Mon, Feb 22, 2010 at 04:20:52PM +0200, Avi Kivity wrote:
>    
>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>      
>>> VIRTIO_PCI_QUEUE_NOTIFY is used to inform availability of new buffers,
>>> so wakeup the iothread to process that information immediately.
>>>
>>> Reported-by: Amit Shah<amit.shah@redhat.com>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Index: qemu/hw/virtio-pci.c
>>> ===================================================================
>>> --- qemu.orig/hw/virtio-pci.c
>>> +++ qemu/hw/virtio-pci.c
>>> @@ -204,6 +204,7 @@ static void virtio_ioport_write(void *op
>>>           break;
>>>       case VIRTIO_PCI_QUEUE_NOTIFY:
>>>           virtio_queue_notify(vdev, val);
>>> +        qemu_notify_event();
>>>           break;
>>>        
>> virtio_queue_notify() will call ->handle_output(), which should
>> either do what's needed to be done, or wake up some iothread itself.
>>      
> kick is used to inform either output processing, in which case
> ->handle_output() does what its supposed to.
>
> But its also used to inform availability of new buffers, which is common
> to all virtio devices. So what is the point pushing this to
> ->handle_output?
>    

I don't understand what this means.  ->handle_output() _is_ informing 
the device model of new buffers.  What more is needed?

> Are you concerned about spurious wakeups?
>    

Yes.  Also, qemu_notify_event() is an undirected notification (wakes up 
all iothreads, and all devices), whereas ->handle_output() is directed 
(wakes up exactly what is needed).

What's the underlying problem?  A new input buffer has become available, 
and we need to re-poll the incoming file descriptor?  If so, that's best 
done from ->handle_output() (either by waking the iothread or calling 
read() itself and perhaps receiving -EAGAIN).
Marcelo Tosatti - Feb. 22, 2010, 3:16 p.m.
On Mon, Feb 22, 2010 at 04:51:46PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:29 PM, Marcelo Tosatti wrote:
> >On Mon, Feb 22, 2010 at 04:20:52PM +0200, Avi Kivity wrote:
> >>On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> >>>VIRTIO_PCI_QUEUE_NOTIFY is used to inform availability of new buffers,
> >>>so wakeup the iothread to process that information immediately.
> >>>
> >>>Reported-by: Amit Shah<amit.shah@redhat.com>
> >>>Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >>>
> >>>Index: qemu/hw/virtio-pci.c
> >>>===================================================================
> >>>--- qemu.orig/hw/virtio-pci.c
> >>>+++ qemu/hw/virtio-pci.c
> >>>@@ -204,6 +204,7 @@ static void virtio_ioport_write(void *op
> >>>          break;
> >>>      case VIRTIO_PCI_QUEUE_NOTIFY:
> >>>          virtio_queue_notify(vdev, val);
> >>>+        qemu_notify_event();
> >>>          break;
> >>virtio_queue_notify() will call ->handle_output(), which should
> >>either do what's needed to be done, or wake up some iothread itself.
> >kick is used to inform either output processing, in which case
> >->handle_output() does what its supposed to.
> >
> >But its also used to inform availability of new buffers, which is common
> >to all virtio devices. So what is the point pushing this to
> >->handle_output?
> 
> I don't understand what this means.  ->handle_output() _is_
> informing the device model of new buffers.  What more is needed?
> 
> >Are you concerned about spurious wakeups?
> 
> Yes.  Also, qemu_notify_event() is an undirected notification (wakes
> up all iothreads, and all devices), whereas ->handle_output() is
> directed (wakes up exactly what is needed).
>
> What's the underlying problem?  A new input buffer has become
> available, and we need to re-poll the incoming file descriptor?  If
> so, that's best done from ->handle_output() (either by waking the
> iothread or calling read() itself and perhaps receiving -EAGAIN).

Yes. Sure, perhaps calling read() itself is appropriate, and i see 
your point that >handle_output contains more context for a smarter
decision.

But one can argue thats an improvement on top of a dumb wakeup.
Anthony Liguori - Feb. 22, 2010, 3:29 p.m.
On 02/22/2010 09:16 AM, Marcelo Tosatti wrote:
>>> Are you concerned about spurious wakeups?
>>>        
>> Yes.  Also, qemu_notify_event() is an undirected notification (wakes
>> up all iothreads, and all devices), whereas ->handle_output() is
>> directed (wakes up exactly what is needed).
>>
>> What's the underlying problem?  A new input buffer has become
>> available, and we need to re-poll the incoming file descriptor?  If
>> so, that's best done from ->handle_output() (either by waking the
>> iothread or calling read() itself and perhaps receiving -EAGAIN).
>>      
> Yes. Sure, perhaps calling read() itself is appropriate, and i see
> your point that>handle_output contains more context for a smarter
> decision.
>
> But one can argue thats an improvement on top of a dumb wakeup.
>    

Spurious calls to qemu_notify_event() also make it difficult to tell 
when it's actually necessary to call qemu_notify_event() vs. when it's 
just something that doesn't hurt.

Regards,

Anthony Liguori

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Avi Kivity - Feb. 22, 2010, 3:32 p.m.
On 02/22/2010 05:29 PM, Anthony Liguori wrote:
> On 02/22/2010 09:16 AM, Marcelo Tosatti wrote:
>>>> Are you concerned about spurious wakeups?
>>> Yes.  Also, qemu_notify_event() is an undirected notification (wakes
>>> up all iothreads, and all devices), whereas ->handle_output() is
>>> directed (wakes up exactly what is needed).
>>>
>>> What's the underlying problem?  A new input buffer has become
>>> available, and we need to re-poll the incoming file descriptor?  If
>>> so, that's best done from ->handle_output() (either by waking the
>>> iothread or calling read() itself and perhaps receiving -EAGAIN).
>> Yes. Sure, perhaps calling read() itself is appropriate, and i see
>> your point that>handle_output contains more context for a smarter
>> decision.
>>
>> But one can argue thats an improvement on top of a dumb wakeup.
>
> Spurious calls to qemu_notify_event() also make it difficult to tell 
> when it's actually necessary to call qemu_notify_event() vs. when it's 
> just something that doesn't hurt.

One improvement in this area would be to add a context parameter (which 
eventually resolves to the underlying thread).  Currently we'd ignore it 
since we have just one iothread, but it would serve to document what's 
being polled, and later direct the wakeup to the correct thread.
Anthony Liguori - Feb. 22, 2010, 3:42 p.m.
On 02/22/2010 09:32 AM, Avi Kivity wrote:
> On 02/22/2010 05:29 PM, Anthony Liguori wrote:
>> On 02/22/2010 09:16 AM, Marcelo Tosatti wrote:
>>>>> Are you concerned about spurious wakeups?
>>>> Yes.  Also, qemu_notify_event() is an undirected notification (wakes
>>>> up all iothreads, and all devices), whereas ->handle_output() is
>>>> directed (wakes up exactly what is needed).
>>>>
>>>> What's the underlying problem?  A new input buffer has become
>>>> available, and we need to re-poll the incoming file descriptor?  If
>>>> so, that's best done from ->handle_output() (either by waking the
>>>> iothread or calling read() itself and perhaps receiving -EAGAIN).
>>> Yes. Sure, perhaps calling read() itself is appropriate, and i see
>>> your point that>handle_output contains more context for a smarter
>>> decision.
>>>
>>> But one can argue thats an improvement on top of a dumb wakeup.
>>
>> Spurious calls to qemu_notify_event() also make it difficult to tell 
>> when it's actually necessary to call qemu_notify_event() vs. when 
>> it's just something that doesn't hurt.
>
> One improvement in this area would be to add a context parameter 
> (which eventually resolves to the underlying thread).  Currently we'd 
> ignore it since we have just one iothread, but it would serve to 
> document what's being polled, and later direct the wakeup to the 
> correct thread.

Ends up looking a lot like a condition.  It's not necessarily a bad 
thing to model.

Regards,

Anthony Liguori
Avi Kivity - Feb. 22, 2010, 3:55 p.m.
On 02/22/2010 05:42 PM, Anthony Liguori wrote:
>>> Spurious calls to qemu_notify_event() also make it difficult to tell 
>>> when it's actually necessary to call qemu_notify_event() vs. when 
>>> it's just something that doesn't hurt.
>>
>> One improvement in this area would be to add a context parameter 
>> (which eventually resolves to the underlying thread).  Currently we'd 
>> ignore it since we have just one iothread, but it would serve to 
>> document what's being polled, and later direct the wakeup to the 
>> correct thread.
>
>
> Ends up looking a lot like a condition.  It's not necessarily a bad 
> thing to model.
>

But the implementation is very different - condition variables sleep in 
pthread_cond_wait, the iothread sleeps in poll().

qemu_notify_event() is really requesting iothread t to re-add file fd to 
its poll set.  Maybe we should make this a CharDriverState method.
Marcelo Tosatti - March 12, 2010, 2:45 a.m.

Patch

Index: qemu/hw/virtio-pci.c
===================================================================
--- qemu.orig/hw/virtio-pci.c
+++ qemu/hw/virtio-pci.c
@@ -204,6 +204,7 @@  static void virtio_ioport_write(void *op
         break;
     case VIRTIO_PCI_QUEUE_NOTIFY:
         virtio_queue_notify(vdev, val);
+        qemu_notify_event();
         break;
     case VIRTIO_PCI_STATUS:
         vdev->status = val & 0xFF;