| Message ID | 20100325134957.646980309@amt.cnet |
|---|---|
| State | New |
| Headers | show |
> /* 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
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.
> 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
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 > > >
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); } }
This can be used later to introduce generic iothread workers. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>