Patchwork [1/2] Pass QEMUIOWorker to qemu_notify_event

login
register
mail settings
Submitter Marcelo Tosatti
Date March 25, 2010, 1:47 p.m.
Message ID <20100325134957.646980309@amt.cnet>
Download mbox | patch
Permalink /patch/48518/
State New
Headers show

Comments

Marcelo Tosatti - March 25, 2010, 1:47 p.m.
This can be used later to introduce generic iothread workers.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Paul Brook - March 25, 2010, 9:06 p.m.
>  /* 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
Marcelo Tosatti - March 26, 2010, 3:55 a.m.
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.
Paul Brook - March 26, 2010, 3:23 p.m.
> 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
Anthony Liguori - March 26, 2010, 3:40 p.m.
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
>
>
>

Patch

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);
     }
 }