[patch uq/master 0/2] wake iothread on virtio kick / flush_coalesced_mmio smp_wmb

[patch uq/master 0/2] wake iothread on virtio kick / flush_coalesced_mmio smp_wmb

[Marcelo Tosatti]

See individual patches for details.

[Qemu-devel] [patch uq/master 0/2] wake iothread on virtio kick / flush_coalesced_mmio smp_wmb

[Marcelo Tosatti]

See individual patches for details.

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

[Marcelo Tosatti]

[-- Attachment #1: virtio-iothread-wake --]
[-- Type: text/plain, Size: 653 bytes --]

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;
     case VIRTIO_PCI_STATUS:
         vdev->status = val & 0xFF;

[Qemu-devel] [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Marcelo Tosatti]

[-- Attachment #1: virtio-iothread-wake --]
[-- Type: text/plain, Size: 651 bytes --]

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;
     case VIRTIO_PCI_STATUS:
         vdev->status = val & 0xFF;

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Marcelo Tosatti]

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?

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Marcelo Tosatti]

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?

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

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

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

[Marcelo Tosatti]

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.

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Marcelo Tosatti]

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.

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

[Anthony Liguori]

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
>    

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Anthony Liguori]

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
>    

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

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

[Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Anthony Liguori]

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

Re: [Qemu-devel] Re: [patch uq/master 1/2] virtio-pci: wake up iothread on VIRTIO_PCI_QUEUE_NOTIFY

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

[patch 0/2] introduce QEMUIOWorker and wake up iothread on virtio-serial-bus notification

[Marcelo Tosatti]

[Qemu-devel] [patch 0/2] introduce QEMUIOWorker and wake up iothread on virtio-serial-bus notification

[Marcelo Tosatti]

[patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Marcelo Tosatti]

[-- Attachment #1: qemu-io-worker --]
[-- Type: text/plain, Size: 4233 bytes --]

This can be used later to introduce generic iothread workers.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-ioworker/async.c
===================================================================
--- qemu-ioworker.orig/async.c
+++ qemu-ioworker/async.c
@@ -180,7 +180,7 @@ void qemu_bh_schedule(QEMUBH *bh)
     bh->scheduled = 1;
     bh->idle = 0; 
     /* stop the currently executing CPU to execute the BH ASAP */
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_bh_cancel(QEMUBH *bh)
Index: qemu-ioworker/hw/mac_dbdma.c
===================================================================
--- qemu-ioworker.orig/hw/mac_dbdma.c
+++ qemu-ioworker/hw/mac_dbdma.c
@@ -655,7 +655,7 @@ void DBDMA_register_channel(void *dbdma,
 
 void DBDMA_schedule(void)
 {
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static void
Index: qemu-ioworker/hw/virtio-net.c
===================================================================
--- qemu-ioworker.orig/hw/virtio-net.c
+++ qemu-ioworker/hw/virtio-net.c
@@ -359,7 +359,7 @@ static void virtio_net_handle_rx(VirtIOD
 
     /* We now have RX buffers, signal to the IO thread to break out of the
      * select to re-poll the tap file descriptor */
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static int virtio_net_can_receive(VLANClientState *nc)
Index: qemu-ioworker/qemu-common.h
===================================================================
--- qemu-ioworker.orig/qemu-common.h
+++ qemu-ioworker/qemu-common.h
@@ -234,11 +234,17 @@ typedef uint64_t pcibus_t;
 void cpu_save(QEMUFile *f, void *opaque);
 int cpu_load(QEMUFile *f, void *opaque, int version_id);
 
+typedef struct QEMUIOWorker {
+    void *opaque;
+} QEMUIOWorker;
+
 /* Force QEMU to stop what it's doing and service IO */
 void qemu_service_io(void);
 
 /* Force QEMU to process pending events */
-void qemu_notify_event(void);
+void qemu_notify_event(QEMUIOWorker *worker);
+
+extern QEMUIOWorker *main_io_worker;
 
 /* Unblock cpu */
 void qemu_cpu_kick(void *env);
Index: qemu-ioworker/vl.c
===================================================================
--- qemu-ioworker.orig/vl.c
+++ qemu-ioworker/vl.c
@@ -274,6 +274,9 @@ uint8_t qemu_uuid[16];
 static QEMUBootSetHandler *boot_set_handler;
 static void *boot_set_opaque;
 
+QEMUIOWorker iothread_worker;
+QEMUIOWorker *main_io_worker = &iothread_worker;
+
 #ifdef SIGRTMIN
 #define SIG_IPI (SIGRTMIN+4)
 #else
@@ -885,7 +888,7 @@ void qemu_mod_timer(QEMUTimer *ts, int64
         }
         /* Interrupt execution to force deadline recalculation.  */
         if (use_icount)
-            qemu_notify_event();
+            qemu_notify_event(main_io_worker);
     }
 }
 
@@ -1062,7 +1065,7 @@ static void host_alarm_handler(int host_
         }
 #endif
         timer_alarm_pending = 1;
-        qemu_notify_event();
+        qemu_notify_event(main_io_worker);
     }
 }
 
@@ -2928,7 +2931,7 @@ static int ram_load(QEMUFile *f, void *o
 
 void qemu_service_io(void)
 {
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 /***********************************************************/
@@ -3180,26 +3183,26 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_system_shutdown_request(void)
 {
     shutdown_requested = 1;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_system_powerdown_request(void)
 {
     powerdown_requested = 1;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 #ifdef CONFIG_IOTHREAD
 static void qemu_system_vmstop_request(int reason)
 {
     vmstop_requested = reason;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 #endif
 
@@ -3341,7 +3344,7 @@ void qemu_cpu_kick(void *env)
     return;
 }
 
-void qemu_notify_event(void)
+void qemu_notify_event(QEMUIOWorker *worker)
 {
     CPUState *env = cpu_single_env;
 
@@ -3727,7 +3730,7 @@ void qemu_init_vcpu(void *_env)
         tcg_init_vcpu(env);
 }
 
-void qemu_notify_event(void)
+void qemu_notify_event(QEMUIOWorker *worker)
 {
     qemu_event_increment();
 }

[Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Marcelo Tosatti]

[-- Attachment #1: qemu-io-worker --]
[-- Type: text/plain, Size: 4231 bytes --]

This can be used later to introduce generic iothread workers.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-ioworker/async.c
===================================================================
--- qemu-ioworker.orig/async.c
+++ qemu-ioworker/async.c
@@ -180,7 +180,7 @@ void qemu_bh_schedule(QEMUBH *bh)
     bh->scheduled = 1;
     bh->idle = 0; 
     /* stop the currently executing CPU to execute the BH ASAP */
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_bh_cancel(QEMUBH *bh)
Index: qemu-ioworker/hw/mac_dbdma.c
===================================================================
--- qemu-ioworker.orig/hw/mac_dbdma.c
+++ qemu-ioworker/hw/mac_dbdma.c
@@ -655,7 +655,7 @@ void DBDMA_register_channel(void *dbdma,
 
 void DBDMA_schedule(void)
 {
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static void
Index: qemu-ioworker/hw/virtio-net.c
===================================================================
--- qemu-ioworker.orig/hw/virtio-net.c
+++ qemu-ioworker/hw/virtio-net.c
@@ -359,7 +359,7 @@ static void virtio_net_handle_rx(VirtIOD
 
     /* We now have RX buffers, signal to the IO thread to break out of the
      * select to re-poll the tap file descriptor */
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static int virtio_net_can_receive(VLANClientState *nc)
Index: qemu-ioworker/qemu-common.h
===================================================================
--- qemu-ioworker.orig/qemu-common.h
+++ qemu-ioworker/qemu-common.h
@@ -234,11 +234,17 @@ typedef uint64_t pcibus_t;
 void cpu_save(QEMUFile *f, void *opaque);
 int cpu_load(QEMUFile *f, void *opaque, int version_id);
 
+typedef struct QEMUIOWorker {
+    void *opaque;
+} QEMUIOWorker;
+
 /* Force QEMU to stop what it's doing and service IO */
 void qemu_service_io(void);
 
 /* Force QEMU to process pending events */
-void qemu_notify_event(void);
+void qemu_notify_event(QEMUIOWorker *worker);
+
+extern QEMUIOWorker *main_io_worker;
 
 /* Unblock cpu */
 void qemu_cpu_kick(void *env);
Index: qemu-ioworker/vl.c
===================================================================
--- qemu-ioworker.orig/vl.c
+++ qemu-ioworker/vl.c
@@ -274,6 +274,9 @@ uint8_t qemu_uuid[16];
 static QEMUBootSetHandler *boot_set_handler;
 static void *boot_set_opaque;
 
+QEMUIOWorker iothread_worker;
+QEMUIOWorker *main_io_worker = &iothread_worker;
+
 #ifdef SIGRTMIN
 #define SIG_IPI (SIGRTMIN+4)
 #else
@@ -885,7 +888,7 @@ void qemu_mod_timer(QEMUTimer *ts, int64
         }
         /* Interrupt execution to force deadline recalculation.  */
         if (use_icount)
-            qemu_notify_event();
+            qemu_notify_event(main_io_worker);
     }
 }
 
@@ -1062,7 +1065,7 @@ static void host_alarm_handler(int host_
         }
 #endif
         timer_alarm_pending = 1;
-        qemu_notify_event();
+        qemu_notify_event(main_io_worker);
     }
 }
 
@@ -2928,7 +2931,7 @@ static int ram_load(QEMUFile *f, void *o
 
 void qemu_service_io(void)
 {
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 /***********************************************************/
@@ -3180,26 +3183,26 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_system_shutdown_request(void)
 {
     shutdown_requested = 1;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_system_powerdown_request(void)
 {
     powerdown_requested = 1;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 #ifdef CONFIG_IOTHREAD
 static void qemu_system_vmstop_request(int reason)
 {
     vmstop_requested = reason;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 #endif
 
@@ -3341,7 +3344,7 @@ void qemu_cpu_kick(void *env)
     return;
 }
 
-void qemu_notify_event(void)
+void qemu_notify_event(QEMUIOWorker *worker)
 {
     CPUState *env = cpu_single_env;
 
@@ -3727,7 +3730,7 @@ void qemu_init_vcpu(void *_env)
         tcg_init_vcpu(env);
 }
 
-void qemu_notify_event(void)
+void qemu_notify_event(QEMUIOWorker *worker)
 {
     qemu_event_increment();
 }

Re: [Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Anthony Liguori]

On 03/11/2010 08:45 PM, Marcelo Tosatti wrote:
> This can be used later to introduce generic iothread workers.
>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>    

Could you rebase this?  It failed to apply in a strange way that made me 
nervous...

Regards,

Anthony Liguori

> Index: qemu-ioworker/async.c
> ===================================================================
> --- qemu-ioworker.orig/async.c
> +++ qemu-ioworker/async.c
> @@ -180,7 +180,7 @@ void qemu_bh_schedule(QEMUBH *bh)
>       bh->scheduled = 1;
>       bh->idle = 0;
>       /* stop the currently executing CPU to execute the BH ASAP */
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   void qemu_bh_cancel(QEMUBH *bh)
> Index: qemu-ioworker/hw/mac_dbdma.c
> ===================================================================
> --- qemu-ioworker.orig/hw/mac_dbdma.c
> +++ qemu-ioworker/hw/mac_dbdma.c
> @@ -655,7 +655,7 @@ void DBDMA_register_channel(void *dbdma,
>
>   void DBDMA_schedule(void)
>   {
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   static void
> Index: qemu-ioworker/hw/virtio-net.c
> ===================================================================
> --- qemu-ioworker.orig/hw/virtio-net.c
> +++ qemu-ioworker/hw/virtio-net.c
> @@ -359,7 +359,7 @@ static void virtio_net_handle_rx(VirtIOD
>
>       /* We now have RX buffers, signal to the IO thread to break out of the
>        * select to re-poll the tap file descriptor */
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   static int virtio_net_can_receive(VLANClientState *nc)
> Index: qemu-ioworker/qemu-common.h
> ===================================================================
> --- qemu-ioworker.orig/qemu-common.h
> +++ qemu-ioworker/qemu-common.h
> @@ -234,11 +234,17 @@ typedef uint64_t pcibus_t;
>   void cpu_save(QEMUFile *f, void *opaque);
>   int cpu_load(QEMUFile *f, void *opaque, int version_id);
>
> +typedef struct QEMUIOWorker {
> +    void *opaque;
> +} QEMUIOWorker;
> +
>   /* Force QEMU to stop what it's doing and service IO */
>   void qemu_service_io(void);
>
>   /* Force QEMU to process pending events */
> -void qemu_notify_event(void);
> +void qemu_notify_event(QEMUIOWorker *worker);
> +
> +extern QEMUIOWorker *main_io_worker;
>
>   /* Unblock cpu */
>   void qemu_cpu_kick(void *env);
> Index: qemu-ioworker/vl.c
> ===================================================================
> --- qemu-ioworker.orig/vl.c
> +++ qemu-ioworker/vl.c
> @@ -274,6 +274,9 @@ uint8_t qemu_uuid[16];
>   static QEMUBootSetHandler *boot_set_handler;
>   static void *boot_set_opaque;
>
> +QEMUIOWorker iothread_worker;
> +QEMUIOWorker *main_io_worker =&iothread_worker;
> +
>   #ifdef SIGRTMIN
>   #define SIG_IPI (SIGRTMIN+4)
>   #else
> @@ -885,7 +888,7 @@ void qemu_mod_timer(QEMUTimer *ts, int64
>           }
>           /* Interrupt execution to force deadline recalculation.  */
>           if (use_icount)
> -            qemu_notify_event();
> +            qemu_notify_event(main_io_worker);
>       }
>   }
>
> @@ -1062,7 +1065,7 @@ static void host_alarm_handler(int host_
>           }
>   #endif
>           timer_alarm_pending = 1;
> -        qemu_notify_event();
> +        qemu_notify_event(main_io_worker);
>       }
>   }
>
> @@ -2928,7 +2931,7 @@ static int ram_load(QEMUFile *f, void *o
>
>   void qemu_service_io(void)
>   {
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   /***********************************************************/
> @@ -3180,26 +3183,26 @@ void qemu_system_reset_request(void)
>       } else {
>           reset_requested = 1;
>       }
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   void qemu_system_shutdown_request(void)
>   {
>       shutdown_requested = 1;
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   void qemu_system_powerdown_request(void)
>   {
>       powerdown_requested = 1;
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   #ifdef CONFIG_IOTHREAD
>   static void qemu_system_vmstop_request(int reason)
>   {
>       vmstop_requested = reason;
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>   #endif
>
> @@ -3341,7 +3344,7 @@ void qemu_cpu_kick(void *env)
>       return;
>   }
>
> -void qemu_notify_event(void)
> +void qemu_notify_event(QEMUIOWorker *worker)
>   {
>       CPUState *env = cpu_single_env;
>
> @@ -3727,7 +3730,7 @@ void qemu_init_vcpu(void *_env)
>           tcg_init_vcpu(env);
>   }
>
> -void qemu_notify_event(void)
> +void qemu_notify_event(QEMUIOWorker *worker)
>   {
>       qemu_event_increment();
>   }
>
>
>
>
>
>    

Re: [Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Anthony Liguori]

On 03/11/2010 08:45 PM, Marcelo Tosatti wrote:
> This can be used later to introduce generic iothread workers.
>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>    

Could you rebase this?  It failed to apply in a strange way that made me 
nervous...

Regards,

Anthony Liguori

> Index: qemu-ioworker/async.c
> ===================================================================
> --- qemu-ioworker.orig/async.c
> +++ qemu-ioworker/async.c
> @@ -180,7 +180,7 @@ void qemu_bh_schedule(QEMUBH *bh)
>       bh->scheduled = 1;
>       bh->idle = 0;
>       /* stop the currently executing CPU to execute the BH ASAP */
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   void qemu_bh_cancel(QEMUBH *bh)
> Index: qemu-ioworker/hw/mac_dbdma.c
> ===================================================================
> --- qemu-ioworker.orig/hw/mac_dbdma.c
> +++ qemu-ioworker/hw/mac_dbdma.c
> @@ -655,7 +655,7 @@ void DBDMA_register_channel(void *dbdma,
>
>   void DBDMA_schedule(void)
>   {
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   static void
> Index: qemu-ioworker/hw/virtio-net.c
> ===================================================================
> --- qemu-ioworker.orig/hw/virtio-net.c
> +++ qemu-ioworker/hw/virtio-net.c
> @@ -359,7 +359,7 @@ static void virtio_net_handle_rx(VirtIOD
>
>       /* We now have RX buffers, signal to the IO thread to break out of the
>        * select to re-poll the tap file descriptor */
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   static int virtio_net_can_receive(VLANClientState *nc)
> Index: qemu-ioworker/qemu-common.h
> ===================================================================
> --- qemu-ioworker.orig/qemu-common.h
> +++ qemu-ioworker/qemu-common.h
> @@ -234,11 +234,17 @@ typedef uint64_t pcibus_t;
>   void cpu_save(QEMUFile *f, void *opaque);
>   int cpu_load(QEMUFile *f, void *opaque, int version_id);
>
> +typedef struct QEMUIOWorker {
> +    void *opaque;
> +} QEMUIOWorker;
> +
>   /* Force QEMU to stop what it's doing and service IO */
>   void qemu_service_io(void);
>
>   /* Force QEMU to process pending events */
> -void qemu_notify_event(void);
> +void qemu_notify_event(QEMUIOWorker *worker);
> +
> +extern QEMUIOWorker *main_io_worker;
>
>   /* Unblock cpu */
>   void qemu_cpu_kick(void *env);
> Index: qemu-ioworker/vl.c
> ===================================================================
> --- qemu-ioworker.orig/vl.c
> +++ qemu-ioworker/vl.c
> @@ -274,6 +274,9 @@ uint8_t qemu_uuid[16];
>   static QEMUBootSetHandler *boot_set_handler;
>   static void *boot_set_opaque;
>
> +QEMUIOWorker iothread_worker;
> +QEMUIOWorker *main_io_worker =&iothread_worker;
> +
>   #ifdef SIGRTMIN
>   #define SIG_IPI (SIGRTMIN+4)
>   #else
> @@ -885,7 +888,7 @@ void qemu_mod_timer(QEMUTimer *ts, int64
>           }
>           /* Interrupt execution to force deadline recalculation.  */
>           if (use_icount)
> -            qemu_notify_event();
> +            qemu_notify_event(main_io_worker);
>       }
>   }
>
> @@ -1062,7 +1065,7 @@ static void host_alarm_handler(int host_
>           }
>   #endif
>           timer_alarm_pending = 1;
> -        qemu_notify_event();
> +        qemu_notify_event(main_io_worker);
>       }
>   }
>
> @@ -2928,7 +2931,7 @@ static int ram_load(QEMUFile *f, void *o
>
>   void qemu_service_io(void)
>   {
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   /***********************************************************/
> @@ -3180,26 +3183,26 @@ void qemu_system_reset_request(void)
>       } else {
>           reset_requested = 1;
>       }
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   void qemu_system_shutdown_request(void)
>   {
>       shutdown_requested = 1;
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   void qemu_system_powerdown_request(void)
>   {
>       powerdown_requested = 1;
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>
>   #ifdef CONFIG_IOTHREAD
>   static void qemu_system_vmstop_request(int reason)
>   {
>       vmstop_requested = reason;
> -    qemu_notify_event();
> +    qemu_notify_event(main_io_worker);
>   }
>   #endif
>
> @@ -3341,7 +3344,7 @@ void qemu_cpu_kick(void *env)
>       return;
>   }
>
> -void qemu_notify_event(void)
> +void qemu_notify_event(QEMUIOWorker *worker)
>   {
>       CPUState *env = cpu_single_env;
>
> @@ -3727,7 +3730,7 @@ void qemu_init_vcpu(void *_env)
>           tcg_init_vcpu(env);
>   }
>
> -void qemu_notify_event(void)
> +void qemu_notify_event(QEMUIOWorker *worker)
>   {
>       qemu_event_increment();
>   }
>
>
>
>
>
>    

[Qemu-devel] [patch 0/2] introduce QEMUIOWorker and wake up iothread on virtio-serial-bus notification (v2)

[Marcelo Tosatti]

Rebased to latest qemu.git.

[Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Marcelo Tosatti]

[-- Attachment #1: qemu-io-worker --]
[-- Type: text/plain, Size: 4500 bytes --]

This can be used later to introduce generic iothread workers.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-ioworker/async.c
===================================================================
--- qemu-ioworker.orig/async.c
+++ qemu-ioworker/async.c
@@ -180,7 +180,7 @@ void qemu_bh_schedule(QEMUBH *bh)
     bh->scheduled = 1;
     bh->idle = 0;
     /* stop the currently executing CPU to execute the BH ASAP */
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_bh_cancel(QEMUBH *bh)
Index: qemu-ioworker/hw/mac_dbdma.c
===================================================================
--- qemu-ioworker.orig/hw/mac_dbdma.c
+++ qemu-ioworker/hw/mac_dbdma.c
@@ -655,7 +655,7 @@ void DBDMA_register_channel(void *dbdma,
 
 void DBDMA_schedule(void)
 {
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static void
Index: qemu-ioworker/hw/virtio-net.c
===================================================================
--- qemu-ioworker.orig/hw/virtio-net.c
+++ qemu-ioworker/hw/virtio-net.c
@@ -360,7 +360,7 @@ static void virtio_net_handle_rx(VirtIOD
 
     /* We now have RX buffers, signal to the IO thread to break out of the
      * select to re-poll the tap file descriptor */
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static int virtio_net_can_receive(VLANClientState *nc)
Index: qemu-ioworker/qemu-common.h
===================================================================
--- qemu-ioworker.orig/qemu-common.h
+++ qemu-ioworker/qemu-common.h
@@ -235,11 +235,17 @@ typedef uint64_t pcibus_t;
 void cpu_save(QEMUFile *f, void *opaque);
 int cpu_load(QEMUFile *f, void *opaque, int version_id);
 
+typedef struct QEMUIOWorker {
+    void *opaque;
+} QEMUIOWorker;
+
 /* Force QEMU to stop what it's doing and service IO */
 void qemu_service_io(void);
 
 /* Force QEMU to process pending events */
-void qemu_notify_event(void);
+void qemu_notify_event(QEMUIOWorker *worker);
+
+extern QEMUIOWorker *main_io_worker;
 
 /* Unblock cpu */
 void qemu_cpu_kick(void *env);
Index: qemu-ioworker/vl.c
===================================================================
--- qemu-ioworker.orig/vl.c
+++ qemu-ioworker/vl.c
@@ -258,6 +258,9 @@ uint8_t qemu_uuid[16];
 static QEMUBootSetHandler *boot_set_handler;
 static void *boot_set_opaque;
 
+QEMUIOWorker iothread_worker;
+QEMUIOWorker *main_io_worker = &iothread_worker;
+
 #ifdef SIGRTMIN
 #define SIG_IPI (SIGRTMIN+4)
 #else
@@ -1884,7 +1887,7 @@ static int ram_load(QEMUFile *f, void *o
 
 void qemu_service_io(void)
 {
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 /***********************************************************/
@@ -2137,19 +2140,19 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_system_shutdown_request(void)
 {
     shutdown_requested = 1;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void qemu_system_powerdown_request(void)
 {
     powerdown_requested = 1;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 static int cpu_can_run(CPUState *env)
@@ -2313,7 +2316,7 @@ void qemu_cpu_kick(void *env)
     return;
 }
 
-void qemu_notify_event(void)
+void qemu_notify_event(QEMUIOWorker *worker)
 {
     CPUState *env = cpu_single_env;
 
@@ -2701,7 +2704,7 @@ void qemu_init_vcpu(void *_env)
         tcg_init_vcpu(env);
 }
 
-void qemu_notify_event(void)
+void qemu_notify_event(QEMUIOWorker *worker)
 {
     qemu_event_increment();
 }
@@ -2709,7 +2712,7 @@ void qemu_notify_event(void)
 static void qemu_system_vmstop_request(int reason)
 {
     vmstop_requested = reason;
-    qemu_notify_event();
+    qemu_notify_event(main_io_worker);
 }
 
 void vm_stop(int reason)
Index: qemu-ioworker/qemu-timer.c
===================================================================
--- qemu-ioworker.orig/qemu-timer.c
+++ qemu-ioworker/qemu-timer.c
@@ -547,7 +547,7 @@ void qemu_mod_timer(QEMUTimer *ts, int64
         }
         /* Interrupt execution to force deadline recalculation.  */
         if (use_icount)
-            qemu_notify_event();
+            qemu_notify_event(main_io_worker);
     }
 }
 
@@ -775,7 +775,7 @@ static void host_alarm_handler(int host_
 
         t->expired = alarm_has_dynticks(t);
         t->pending = 1;
-        qemu_notify_event();
+        qemu_notify_event(main_io_worker);
     }
 }
 

Re: [Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Paul Brook]

>  /* Force QEMU to process pending events */
> -void qemu_notify_event(void);
> +void qemu_notify_event(QEMUIOWorker *worker);

>  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    qemu_notify_event(main_io_worker);
>  }

This feels completely wrong.

Devices shouldn't know or care about implementation details like this. How is 
a device supposed to know which worker it should be waking up?

qemu_notify_event is an ugly hack to workaround the fact that our character 
device API is polled. If shouldn't exist in the first place, instead we should 
have a proper mechanism for device emulation to notify the CharDriverState 
when it is ready to recieve more data.

Paul

Re: [Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Marcelo Tosatti]

On Thu, Mar 25, 2010 at 09:06:00PM +0000, Paul Brook wrote:
> >  /* Force QEMU to process pending events */
> > -void qemu_notify_event(void);
> > +void qemu_notify_event(QEMUIOWorker *worker);
> 
> >  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> > +    qemu_notify_event(main_io_worker);
> >  }
> 
> This feels completely wrong.
> 
> Devices shouldn't know or care about implementation details like this. How is 
> a device supposed to know which worker it should be waking up?

Its not. It could use qemu_notify_event(DeviceInfo->worker), and have no
knowledge of the internals.

> qemu_notify_event is an ugly hack to workaround the fact that our character 
> device API is polled. If shouldn't exist in the first place, instead we should 
> have a proper mechanism for device emulation to notify the CharDriverState 
> when it is ready to recieve more data.

qemu_notify_event is used to break the main_loop_wait out of select(),
to reprocess events that are handled by it (such as ability to receive
more data). Perhaps it is appropriate to move that notification to
another level, but there are other users of it at device level already.

Re: [Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Paul Brook]

> On Thu, Mar 25, 2010 at 09:06:00PM +0000, Paul Brook wrote:
> > >  /* Force QEMU to process pending events */
> > > -void qemu_notify_event(void);
> > > +void qemu_notify_event(QEMUIOWorker *worker);
> > >
> > >  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> > >  {
> > > +    qemu_notify_event(main_io_worker);
> > >  }
> >
> > This feels completely wrong.
> >
> > Devices shouldn't know or care about implementation details like this.
> > How is a device supposed to know which worker it should be waking up?
> 
> Its not. It could use qemu_notify_event(DeviceInfo->worker), and have no
> knowledge of the internals.

In that case I think you're abusing this API.

I'm very wary of introducing random bits of code that allegedly allow future 
use of threads.  Exploiting thread level parallelism is a hard problem that 
needs proper design.  A such I object to this patch, and think we first need 
to decide what form of concurrency model we want to use in QEMU.

Paul

Re: [Qemu-devel] [patch 1/2] Pass QEMUIOWorker to qemu_notify_event

[Anthony Liguori]

On 03/26/2010 10:23 AM, Paul Brook wrote:
>> On Thu, Mar 25, 2010 at 09:06:00PM +0000, Paul Brook wrote:
>>      
>>>>   /* Force QEMU to process pending events */
>>>> -void qemu_notify_event(void);
>>>> +void qemu_notify_event(QEMUIOWorker *worker);
>>>>
>>>>   static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>>>>   {
>>>> +    qemu_notify_event(main_io_worker);
>>>>   }
>>>>          
>>> This feels completely wrong.
>>>
>>> Devices shouldn't know or care about implementation details like this.
>>> How is a device supposed to know which worker it should be waking up?
>>>        
>> Its not. It could use qemu_notify_event(DeviceInfo->worker), and have no
>> knowledge of the internals.
>>      
> In that case I think you're abusing this API.
>
> I'm very wary of introducing random bits of code that allegedly allow future
> use of threads.  Exploiting thread level parallelism is a hard problem that
> needs proper design.  A such I object to this patch, and think we first need
> to decide what form of concurrency model we want to use in QEMU.
>    

I agree.  There's a lot of context missing from a proposal like this.

Regards,

Anthony Liguori

> Paul
>
>
>    

[Qemu-devel] [patch 2/2] virtio-serial-bus: wake up iothread upon guest read notification

[Marcelo Tosatti]

[-- Attachment #1: virtio-serial-wake --]
[-- Type: text/plain, Size: 539 bytes --]

Wake up iothread when buffers are consumed.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-ioworker/hw/virtio-serial-bus.c
===================================================================
--- qemu-ioworker.orig/hw/virtio-serial-bus.c
+++ qemu-ioworker/hw/virtio-serial-bus.c
@@ -331,6 +331,7 @@ static void handle_output(VirtIODevice *
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
+    qemu_notify_event(main_io_worker);
 }
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)

[patch 2/2] virtio-serial-bus: wake up iothread upon guest read notification

[Marcelo Tosatti]

[-- Attachment #1: virtio-serial-wake --]
[-- Type: text/plain, Size: 541 bytes --]

Wake up iothread when buffers are consumed.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-ioworker/hw/virtio-serial-bus.c
===================================================================
--- qemu-ioworker.orig/hw/virtio-serial-bus.c
+++ qemu-ioworker/hw/virtio-serial-bus.c
@@ -331,6 +331,7 @@ static void handle_output(VirtIODevice *
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
+    qemu_notify_event(main_io_worker);
 }
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)

[Qemu-devel] [patch 2/2] virtio-serial-bus: wake up iothread upon guest read notification

[Marcelo Tosatti]

[-- Attachment #1: virtio-serial-wake --]
[-- Type: text/plain, Size: 539 bytes --]

Wake up iothread when buffers are consumed.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu-ioworker/hw/virtio-serial-bus.c
===================================================================
--- qemu-ioworker.orig/hw/virtio-serial-bus.c
+++ qemu-ioworker/hw/virtio-serial-bus.c
@@ -331,6 +331,7 @@ static void handle_output(VirtIODevice *
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
+    qemu_notify_event(main_io_worker);
 }
 
 static uint32_t get_features(VirtIODevice *vdev, uint32_t features)

Re: [patch 2/2] virtio-serial-bus: wake up iothread upon guest read notification

[Amit Shah]

On (Thu) Mar 11 2010 [23:45:51], Marcelo Tosatti wrote:
> Wake up iothread when buffers are consumed.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: qemu-ioworker/hw/virtio-serial-bus.c
> ===================================================================
> --- qemu-ioworker.orig/hw/virtio-serial-bus.c
> +++ qemu-ioworker/hw/virtio-serial-bus.c
> @@ -331,6 +331,7 @@ static void handle_output(VirtIODevice *
>  
>  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    qemu_notify_event(main_io_worker);
>  }

ACK, the host lets us know buffers are consumed and new buffers have
been added to the pool so that we can start sending more data.

Before this patch my tests took 16m18s to run.
After this patch my tests take 1m17s to run.

Both tests done with just one buffer made available in the virtio-queues.

		Amit

[Qemu-devel] Re: [patch 2/2] virtio-serial-bus: wake up iothread upon guest read notification

[Amit Shah]

On (Thu) Mar 11 2010 [23:45:51], Marcelo Tosatti wrote:
> Wake up iothread when buffers are consumed.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: qemu-ioworker/hw/virtio-serial-bus.c
> ===================================================================
> --- qemu-ioworker.orig/hw/virtio-serial-bus.c
> +++ qemu-ioworker/hw/virtio-serial-bus.c
> @@ -331,6 +331,7 @@ static void handle_output(VirtIODevice *
>  
>  static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  {
> +    qemu_notify_event(main_io_worker);
>  }

ACK, the host lets us know buffers are consumed and new buffers have
been added to the pool so that we can start sending more data.

Before this patch my tests took 16m18s to run.
After this patch my tests take 1m17s to run.

Both tests done with just one buffer made available in the virtio-queues.

		Amit

[patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Marcelo Tosatti]

[-- Attachment #1: smp-wmb-kvm-all --]
[-- Type: text/plain, Size: 828 bytes --]

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/kvm-all.c
===================================================================
--- qemu.orig/kvm-all.c
+++ qemu/kvm-all.c
@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port, 
     return 1;
 }
 
+/* FIXME: arch dependant, x86 version */
+#define smp_wmb()   asm volatile("" ::: "memory")
+
 void kvm_flush_coalesced_mmio_buffer(void)
 {
 #ifdef KVM_CAP_COALESCED_MMIO
@@ -730,7 +733,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
             ent = &ring->coalesced_mmio[ring->first];
 
             cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
-            /* FIXME smp_wmb() */
+            smp_wmb();
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
     }

[Qemu-devel] [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Marcelo Tosatti]

[-- Attachment #1: smp-wmb-kvm-all --]
[-- Type: text/plain, Size: 826 bytes --]

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/kvm-all.c
===================================================================
--- qemu.orig/kvm-all.c
+++ qemu/kvm-all.c
@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port, 
     return 1;
 }
 
+/* FIXME: arch dependant, x86 version */
+#define smp_wmb()   asm volatile("" ::: "memory")
+
 void kvm_flush_coalesced_mmio_buffer(void)
 {
 #ifdef KVM_CAP_COALESCED_MMIO
@@ -730,7 +733,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
             ent = &ring->coalesced_mmio[ring->first];
 
             cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
-            /* FIXME smp_wmb() */
+            smp_wmb();
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
     }

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> Cc: "Michael S. Tsirkin"<mst@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> Index: qemu/kvm-all.c
> ===================================================================
> --- qemu.orig/kvm-all.c
> +++ qemu/kvm-all.c
> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>       return 1;
>   }
>
> +/* FIXME: arch dependant, x86 version */
> +#define smp_wmb()   asm volatile("" ::: "memory")
> +
>    

sfence?  what about other arches?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> Cc: "Michael S. Tsirkin"<mst@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>
> Index: qemu/kvm-all.c
> ===================================================================
> --- qemu.orig/kvm-all.c
> +++ qemu/kvm-all.c
> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>       return 1;
>   }
>
> +/* FIXME: arch dependant, x86 version */
> +#define smp_wmb()   asm volatile("" ::: "memory")
> +
>    

sfence?  what about other arches?

-- 
error compiling committee.c: too many arguments to function

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Marcelo Tosatti]

On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> >Cc: "Michael S. Tsirkin"<mst@redhat.com>
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >Index: qemu/kvm-all.c
> >===================================================================
> >--- qemu.orig/kvm-all.c
> >+++ qemu/kvm-all.c
> >@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
> >      return 1;
> >  }
> >
> >+/* FIXME: arch dependant, x86 version */
> >+#define smp_wmb()   asm volatile("" ::: "memory")
> >+
> 
> sfence?  

There is no need (for this case). Older read cannot be reordered with
write, writes are not reordered with other writes, writes by a single
processor are observed in the same order by all processors.

> what about other arches?

They need to be fixed? PPC needs an instruction apparently.

Is there any objection to including this patch?

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Marcelo Tosatti]

On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
> >Cc: "Michael S. Tsirkin"<mst@redhat.com>
> >Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> >
> >Index: qemu/kvm-all.c
> >===================================================================
> >--- qemu.orig/kvm-all.c
> >+++ qemu/kvm-all.c
> >@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
> >      return 1;
> >  }
> >
> >+/* FIXME: arch dependant, x86 version */
> >+#define smp_wmb()   asm volatile("" ::: "memory")
> >+
> 
> sfence?  

There is no need (for this case). Older read cannot be reordered with
write, writes are not reordered with other writes, writes by a single
processor are observed in the same order by all processors.

> what about other arches?

They need to be fixed? PPC needs an instruction apparently.

Is there any objection to including this patch?

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 04:45 PM, Marcelo Tosatti wrote:
> On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
>    
>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>      
>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Index: qemu/kvm-all.c
>>> ===================================================================
>>> --- qemu.orig/kvm-all.c
>>> +++ qemu/kvm-all.c
>>> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>>>       return 1;
>>>   }
>>>
>>> +/* FIXME: arch dependant, x86 version */
>>> +#define smp_wmb()   asm volatile("" ::: "memory")
>>> +
>>>        
>> sfence?
>>      
> There is no need (for this case). Older read cannot be reordered with
> write, writes are not reordered with other writes, writes by a single
> processor are observed in the same order by all processors.
>    

Well, Linux does use sfence.  Perhaps it's only needed for WC writes 
(movnti and friends), but better be careful here.

>> what about other arches?
>>      
> They need to be fixed? PPC needs an instruction apparently.
>
> Is there any objection to including this patch?
>    

I imagine all arches need an instruction.  For reads as well.

Note, gcc has a __sync_synchronize() builtin that compiles to mfence on 
x86.  We might use that as a baseline for both rmb and wmb, and let each 
arch override it incrementally.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 04:45 PM, Marcelo Tosatti wrote:
> On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
>    
>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>      
>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>> Index: qemu/kvm-all.c
>>> ===================================================================
>>> --- qemu.orig/kvm-all.c
>>> +++ qemu/kvm-all.c
>>> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>>>       return 1;
>>>   }
>>>
>>> +/* FIXME: arch dependant, x86 version */
>>> +#define smp_wmb()   asm volatile("" ::: "memory")
>>> +
>>>        
>> sfence?
>>      
> There is no need (for this case). Older read cannot be reordered with
> write, writes are not reordered with other writes, writes by a single
> processor are observed in the same order by all processors.
>    

Well, Linux does use sfence.  Perhaps it's only needed for WC writes 
(movnti and friends), but better be careful here.

>> what about other arches?
>>      
> They need to be fixed? PPC needs an instruction apparently.
>
> Is there any objection to including this patch?
>    

I imagine all arches need an instruction.  For reads as well.

Note, gcc has a __sync_synchronize() builtin that compiles to mfence on 
x86.  We might use that as a baseline for both rmb and wmb, and let each 
arch override it incrementally.

-- 
error compiling committee.c: too many arguments to function

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Michael S. Tsirkin]

On Mon, Feb 22, 2010 at 04:57:29PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:45 PM, Marcelo Tosatti wrote:
>> On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
>>    
>>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>>      
>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>>
>>>> Index: qemu/kvm-all.c
>>>> ===================================================================
>>>> --- qemu.orig/kvm-all.c
>>>> +++ qemu/kvm-all.c
>>>> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>>>>       return 1;
>>>>   }
>>>>
>>>> +/* FIXME: arch dependant, x86 version */
>>>> +#define smp_wmb()   asm volatile("" ::: "memory")
>>>> +
>>>>        
>>> sfence?
>>>      
>> There is no need (for this case). Older read cannot be reordered with
>> write, writes are not reordered with other writes, writes by a single
>> processor are observed in the same order by all processors.
>>    
>
> Well, Linux does use sfence.

At least on 64 bit it doesnt.

>  Perhaps it's only needed for WC writes  
> (movnti and friends), but better be careful here.
>
>>> what about other arches?
>>>      
>> They need to be fixed? PPC needs an instruction apparently.
>>
>> Is there any objection to including this patch?
>>    
>
> I imagine all arches need an instruction.  For reads as well.
>
> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on  
> x86.  We might use that as a baseline for both rmb and wmb, and let each  
> arch override it incrementally.

This it what my patch did. Note it only works well for recent gcc.

> -- 
> error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Michael S. Tsirkin]

On Mon, Feb 22, 2010 at 04:57:29PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:45 PM, Marcelo Tosatti wrote:
>> On Mon, Feb 22, 2010 at 04:23:32PM +0200, Avi Kivity wrote:
>>    
>>> On 02/22/2010 03:59 PM, Marcelo Tosatti wrote:
>>>      
>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>>
>>>> Index: qemu/kvm-all.c
>>>> ===================================================================
>>>> --- qemu.orig/kvm-all.c
>>>> +++ qemu/kvm-all.c
>>>> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
>>>>       return 1;
>>>>   }
>>>>
>>>> +/* FIXME: arch dependant, x86 version */
>>>> +#define smp_wmb()   asm volatile("" ::: "memory")
>>>> +
>>>>        
>>> sfence?
>>>      
>> There is no need (for this case). Older read cannot be reordered with
>> write, writes are not reordered with other writes, writes by a single
>> processor are observed in the same order by all processors.
>>    
>
> Well, Linux does use sfence.

At least on 64 bit it doesnt.

>  Perhaps it's only needed for WC writes  
> (movnti and friends), but better be careful here.
>
>>> what about other arches?
>>>      
>> They need to be fixed? PPC needs an instruction apparently.
>>
>> Is there any objection to including this patch?
>>    
>
> I imagine all arches need an instruction.  For reads as well.
>
> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on  
> x86.  We might use that as a baseline for both rmb and wmb, and let each  
> arch override it incrementally.

This it what my patch did. Note it only works well for recent gcc.

> -- 
> error compiling committee.c: too many arguments to function

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 04:57 PM, Michael S. Tsirkin wrote:
>
>    
>>> There is no need (for this case). Older read cannot be reordered with
>>> write, writes are not reordered with other writes, writes by a single
>>> processor are observed in the same order by all processors.
>>>
>>>        
>> Well, Linux does use sfence.
>>      
> At least on 64 bit it doesnt.
>    

Right, I was looking at wmb(), not smp_wmb().

>> I imagine all arches need an instruction.  For reads as well.
>>
>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>> arch override it incrementally.
>>      
> This it what my patch did. Note it only works well for recent gcc.
>    

Do you know how recent?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 04:57 PM, Michael S. Tsirkin wrote:
>
>    
>>> There is no need (for this case). Older read cannot be reordered with
>>> write, writes are not reordered with other writes, writes by a single
>>> processor are observed in the same order by all processors.
>>>
>>>        
>> Well, Linux does use sfence.
>>      
> At least on 64 bit it doesnt.
>    

Right, I was looking at wmb(), not smp_wmb().

>> I imagine all arches need an instruction.  For reads as well.
>>
>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>> arch override it incrementally.
>>      
> This it what my patch did. Note it only works well for recent gcc.
>    

Do you know how recent?

-- 
error compiling committee.c: too many arguments to function

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Michael S. Tsirkin]

On Mon, Feb 22, 2010 at 05:08:00PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:57 PM, Michael S. Tsirkin wrote:
>>
>>    
>>>> There is no need (for this case). Older read cannot be reordered with
>>>> write, writes are not reordered with other writes, writes by a single
>>>> processor are observed in the same order by all processors.
>>>>
>>>>        
>>> Well, Linux does use sfence.
>>>      
>> At least on 64 bit it doesnt.
>>    
>
> Right, I was looking at wmb(), not smp_wmb().
>
>>> I imagine all arches need an instruction.  For reads as well.
>>>
>>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>>> arch override it incrementally.
>>>      
>> This it what my patch did. Note it only works well for recent gcc.
>>    
>
> Do you know how recent?

IIRC 4.3 has broken implementation.
4.4 seems OK as far as I can tell.

> -- 
> error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Michael S. Tsirkin]

On Mon, Feb 22, 2010 at 05:08:00PM +0200, Avi Kivity wrote:
> On 02/22/2010 04:57 PM, Michael S. Tsirkin wrote:
>>
>>    
>>>> There is no need (for this case). Older read cannot be reordered with
>>>> write, writes are not reordered with other writes, writes by a single
>>>> processor are observed in the same order by all processors.
>>>>
>>>>        
>>> Well, Linux does use sfence.
>>>      
>> At least on 64 bit it doesnt.
>>    
>
> Right, I was looking at wmb(), not smp_wmb().
>
>>> I imagine all arches need an instruction.  For reads as well.
>>>
>>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>>> arch override it incrementally.
>>>      
>> This it what my patch did. Note it only works well for recent gcc.
>>    
>
> Do you know how recent?

IIRC 4.3 has broken implementation.
4.4 seems OK as far as I can tell.

> -- 
> error compiling committee.c: too many arguments to function

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 05:08 PM, Michael S. Tsirkin wrote:
>
>>
>>>> I imagine all arches need an instruction.  For reads as well.
>>>>
>>>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>>>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>>>> arch override it incrementally.
>>>>
>>>>          
>>> This it what my patch did. Note it only works well for recent gcc.
>>>
>>>        
>> Do you know how recent?
>>      
> IIRC 4.3 has broken implementation.
> 4.4 seems OK as far as I can tell.
>    

Well, so that idea's down.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 05:08 PM, Michael S. Tsirkin wrote:
>
>>
>>>> I imagine all arches need an instruction.  For reads as well.
>>>>
>>>> Note, gcc has a __sync_synchronize() builtin that compiles to mfence on
>>>> x86.  We might use that as a baseline for both rmb and wmb, and let each
>>>> arch override it incrementally.
>>>>
>>>>          
>>> This it what my patch did. Note it only works well for recent gcc.
>>>
>>>        
>> Do you know how recent?
>>      
> IIRC 4.3 has broken implementation.
> 4.4 seems OK as far as I can tell.
>    

Well, so that idea's down.

-- 
error compiling committee.c: too many arguments to function

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Michael S. Tsirkin]

On Mon, Feb 22, 2010 at 10:59:08AM -0300, Marcelo Tosatti wrote:
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

We'll need implementation for other arches, I'll dust off
my patch that adds it and repost, but for now this
is better than what we have.

> Index: qemu/kvm-all.c
> ===================================================================
> --- qemu.orig/kvm-all.c
> +++ qemu/kvm-all.c
> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port, 
>      return 1;
>  }
>  
> +/* FIXME: arch dependant, x86 version */
> +#define smp_wmb()   asm volatile("" ::: "memory")
> +
>  void kvm_flush_coalesced_mmio_buffer(void)
>  {
>  #ifdef KVM_CAP_COALESCED_MMIO
> @@ -730,7 +733,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
>              ent = &ring->coalesced_mmio[ring->first];
>  
>              cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> -            /* FIXME smp_wmb() */
> +            smp_wmb();
>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>          }
>      }
> 

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Michael S. Tsirkin]

On Mon, Feb 22, 2010 at 10:59:08AM -0300, Marcelo Tosatti wrote:
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

We'll need implementation for other arches, I'll dust off
my patch that adds it and repost, but for now this
is better than what we have.

> Index: qemu/kvm-all.c
> ===================================================================
> --- qemu.orig/kvm-all.c
> +++ qemu/kvm-all.c
> @@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port, 
>      return 1;
>  }
>  
> +/* FIXME: arch dependant, x86 version */
> +#define smp_wmb()   asm volatile("" ::: "memory")
> +
>  void kvm_flush_coalesced_mmio_buffer(void)
>  {
>  #ifdef KVM_CAP_COALESCED_MMIO
> @@ -730,7 +733,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
>              ent = &ring->coalesced_mmio[ring->first];
>  
>              cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> -            /* FIXME smp_wmb() */
> +            smp_wmb();
>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>          }
>      }
> 

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Marcelo Tosatti]

Acked-by: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/kvm-all.c
===================================================================
--- qemu.orig/kvm-all.c
+++ qemu/kvm-all.c
@@ -21,6 +21,7 @@
 #include <linux/kvm.h>
 
 #include "qemu-common.h"
+#include "qemu-barrier.h"
 #include "sysemu.h"
 #include "hw/hw.h"
 #include "gdbstub.h"
@@ -730,7 +731,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
             ent = &ring->coalesced_mmio[ring->first];
 
             cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
-            /* FIXME smp_wmb() */
+            smp_wmb();
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
     }
Index: qemu/qemu-barrier.h
===================================================================
--- /dev/null
+++ qemu/qemu-barrier.h
@@ -0,0 +1,7 @@
+#ifndef __QEMU_BARRIER_H
+#define __QEMU_BARRIER_H 1
+
+/* FIXME: arch dependant, x86 version */
+#define smp_wmb()   asm volatile("" ::: "memory")
+
+#endif

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Marcelo Tosatti]

Acked-by: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/kvm-all.c
===================================================================
--- qemu.orig/kvm-all.c
+++ qemu/kvm-all.c
@@ -21,6 +21,7 @@
 #include <linux/kvm.h>
 
 #include "qemu-common.h"
+#include "qemu-barrier.h"
 #include "sysemu.h"
 #include "hw/hw.h"
 #include "gdbstub.h"
@@ -730,7 +731,7 @@ void kvm_flush_coalesced_mmio_buffer(voi
             ent = &ring->coalesced_mmio[ring->first];
 
             cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
-            /* FIXME smp_wmb() */
+            smp_wmb();
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
     }
Index: qemu/qemu-barrier.h
===================================================================
--- /dev/null
+++ qemu/qemu-barrier.h
@@ -0,0 +1,7 @@
+#ifndef __QEMU_BARRIER_H
+#define __QEMU_BARRIER_H 1
+
+/* FIXME: arch dependant, x86 version */
+#define smp_wmb()   asm volatile("" ::: "memory")
+
+#endif

Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 06:57 PM, Marcelo Tosatti wrote:
> Acked-by: "Michael S. Tsirkin"<mst@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [patch uq/master 2/2] kvm-all.c: define smp_wmb and use it for coalesced mmio

[Avi Kivity]

On 02/22/2010 06:57 PM, Marcelo Tosatti wrote:
> Acked-by: "Michael S. Tsirkin"<mst@redhat.com>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function