Patchwork [v6,18/18] virtio-serial-bus: wake up iothread upon guest read notification

login
register
mail settings
Submitter Amit Shah
Date April 27, 2010, 12:34 p.m.
Message ID <1272371652-23087-19-git-send-email-amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/51076/
State New
Headers show

Comments

Amit Shah - April 27, 2010, 12:34 p.m.
From: Marcelo Tosatti <mtosatti@redhat.com>

Wake up iothread when buffers are consumed.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Anthony Liguori - April 27, 2010, 5:41 p.m.
On 04/27/2010 07:34 AM, Amit Shah wrote:
> From: Marcelo Tosatti<mtosatti@redhat.com>
>
> Wake up iothread when buffers are consumed.
>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>    

What's the race here?  This looks very odd to me.

Regards,

Anthony Liguori

> ---
>   hw/virtio-serial-bus.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 97694d5..ee868e9 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -417,6 +417,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
>
>   static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>   {
> +    qemu_notify_event();
>   }
>
>   static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
>
Marcelo Tosatti - April 27, 2010, 5:58 p.m.
On Tue, Apr 27, 2010 at 12:41:27PM -0500, Anthony Liguori wrote:
> On 04/27/2010 07:34 AM, Amit Shah wrote:
> >From: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >Wake up iothread when buffers are consumed.
> >
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> 
> What's the race here?  This looks very odd to me.

We discussed this on the following thread:

http://www.mail-archive.com/kvm@vger.kernel.org/msg29249.html
Anthony Liguori - April 27, 2010, 6:13 p.m.
On 04/27/2010 12:58 PM, Marcelo Tosatti wrote:
> On Tue, Apr 27, 2010 at 12:41:27PM -0500, Anthony Liguori wrote:
>    
>> On 04/27/2010 07:34 AM, Amit Shah wrote:
>>      
>>> From: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Wake up iothread when buffers are consumed.
>>>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>        
>> What's the race here?  This looks very odd to me.
>>      
> We discussed this on the following thread:
>
> http://www.mail-archive.com/kvm@vger.kernel.org/msg29249.html
>    

I don't think there was ever an adequate explanation of exactly what was 
happening.

My suspicion is that the real problem is due to the can_read() handlers 
whereas it's necessary to invoke the main loop for can_read() to be 
executed again to cause the event to be polled.  The better solution 
though is to not use the can_read() handler and instead explicitly 
register and deregister the read callbacks.

qemu_set_fd_handler() would require a notify call but that makes sense.

I think this is a good justification for removing uses of 
qemu_set_fd_handler2().

Regards,

Anthony Liguori
Amit Shah - April 28, 2010, 7:29 a.m.
On (Tue) Apr 27 2010 [12:41:27], Anthony Liguori wrote:
> On 04/27/2010 07:34 AM, Amit Shah wrote:
>> From: Marcelo Tosatti<mtosatti@redhat.com>
>>
>> Wake up iothread when buffers are consumed.
>>
>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>    
>
> What's the race here?  This looks very odd to me.

When the guest indicates it has added buffers to the vq, the iothread
can then start consuming them. Without this notification, the iothread
only polls for free buffers when it times out or gets woken up
otherwise.

Other virtio devices do it similarly as well.

		Amit
Anthony Liguori - April 28, 2010, 1:25 p.m.
On 04/28/2010 02:29 AM, Amit Shah wrote:
> On (Tue) Apr 27 2010 [12:41:27], Anthony Liguori wrote:
>    
>> On 04/27/2010 07:34 AM, Amit Shah wrote:
>>      
>>> From: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Wake up iothread when buffers are consumed.
>>>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>
>>>        
>> What's the race here?  This looks very odd to me.
>>      
> When the guest indicates it has added buffers to the vq, the iothread
> can then start consuming them. Without this notification, the iothread
> only polls for free buffers when it times out or gets woken up
> otherwise.
>    

When you say, polls for free buffers, what do you mean by that?

You mean, there's a can_read() somewhere that checks for free buffers?

I think switching to qemu_set_fd_handler() would be a better solution.

Regards,

Anthony Liguori

> Other virtio devices do it similarly as well.
>
> 		Amit
>
Amit Shah - April 28, 2010, 4:34 p.m.
On (Wed) Apr 28 2010 [08:25:59], Anthony Liguori wrote:
> On 04/28/2010 02:29 AM, Amit Shah wrote:
>> On (Tue) Apr 27 2010 [12:41:27], Anthony Liguori wrote:
>>    
>>> On 04/27/2010 07:34 AM, Amit Shah wrote:
>>>      
>>>> From: Marcelo Tosatti<mtosatti@redhat.com>
>>>>
>>>> Wake up iothread when buffers are consumed.
>>>>
>>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>>
>>>>        
>>> What's the race here?  This looks very odd to me.
>>>      
>> When the guest indicates it has added buffers to the vq, the iothread
>> can then start consuming them. Without this notification, the iothread
>> only polls for free buffers when it times out or gets woken up
>> otherwise.
>>    
>
> When you say, polls for free buffers, what do you mean by that?
>
> You mean, there's a can_read() somewhere that checks for free buffers?

Not just can_read(), in-qemu applications too can query when writes to
guests will go through.

> I think switching to qemu_set_fd_handler() would be a better solution.

Hm, there's no fd here. How to signal to apps (ports) when a guest
becomes writable?

		Amit

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 97694d5..ee868e9 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -417,6 +417,7 @@  static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
+    qemu_notify_event();
 }
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)