wake iothread on virtio kick / flush_coalesced_mmio smp_wmb

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<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Index: qemu/hw/virtio-pci.c
===================================================================

Quoted 6 lines · Marcelo Tosatti

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<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

hw/virtio-pci.c static void virtio_ioport_write(void *op · line 208

Quoted 10 lines

Index: qemu/hw/virtio-pci.c
===================================================================

hw/virtio-pci.c +1 -0

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

Quoted 8 lines · Avi Kivity

Quoted 6 lines · Marcelo Tosatti

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<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Quoted 14 lines

hw/virtio-pci.c static void virtio_ioport_write(void *op · line 209

Quoted 11 lines

Index: qemu/hw/virtio-pci.c
===================================================================

hw/virtio-pci.c +1 -0

@@ -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?

Quoted 20 lines · Marcelo Tosatti

Quoted 7 lines · Avi Kivity

Quoted 6 lines · Marcelo Tosatti

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<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Quoted 12 lines

hw/virtio-pci.c static void virtio_ioport_write(void *op · line 208

Quoted 10 lines

Index: qemu/hw/virtio-pci.c
===================================================================

hw/virtio-pci.c +1 -0

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

Quoted 6 lines

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?

Quoted 1 line

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

Quoted 11 lines

Quoted 2 lines

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.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Marcelo Tosatti

Quoted 9 lines

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 14 lines · Anthony Liguori

Quoted 13 lines · Marcelo Tosatti

Quoted 9 lines

Quoted 1 line

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 3 lines

Quoted 1 line

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.

This can be used later to introduce generic iothread workers.

Signed-off-by Marcelo Tosatti<[email protected]>

Index: qemu-ioworker/async.c
===================================================================

async.c +1 -1

@@ -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
===================================================================

hw/mac_dbdma.c +1 -1

@@ -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
===================================================================

hw/virtio-net.c +1 -1

@@ -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-common.h +7 -1

@@ -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
===================================================================

vl.c +12 -9

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

hw/mac_dbdma.c void DBDMA_register_channel(void *dbdma, · line 659

Quoted 8 lines

/* Force QEMU to process pending events */

hw/mac_dbdma.c +2 -1

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

Quoted 11 lines

hw/mac_dbdma.c void DBDMA_register_channel(void *dbdma, · line 659

Quoted 8 lines

/* Force QEMU to process pending events */

hw/mac_dbdma.c +2 -1

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

Quoted 2 lines

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.

Quoted 13 lines · Paul Brook

Quoted 12 lines

hw/mac_dbdma.c void DBDMA_register_channel(void *dbdma, · line 659

Quoted 10 lines

/* Force QEMU to process pending events */

hw/mac_dbdma.c +2 -1

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

Quoted 6 lines

Quoted 3 lines

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

Wake up iothread when buffers are consumed.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: qemu-ioworker/hw/virtio-serial-bus.c
===================================================================

Cc "Michael S. Tsirkin"<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Index: qemu/kvm-all.c
===================================================================

Quoted 11 lines · Marcelo Tosatti

Cc "Michael S. Tsirkin"<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Index: qemu/kvm-all.c
===================================================================

kvm-all.c +0 -0

@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
     return 1;
 }
 

kvm-all.c static int kvm_handle_io(uint16_t port, · line 723

Quoted 3 lines

kvm-all.c +3 -0

+/* FIXME: arch dependant, x86 version */
+#define smp_wmb()   asm volatile("" ::: "memory")
+

sfence?

what about other arches?

Quoted 13 lines · Avi Kivity

Quoted 11 lines · Marcelo Tosatti

Cc "Michael S. Tsirkin"<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Index: qemu/kvm-all.c
===================================================================

kvm-all.c +0 -0

@@ -718,6 +718,9 @@ static int kvm_handle_io(uint16_t port,
      return 1;
  }
 

Quoted 6 lines

kvm-all.c static int kvm_handle_io(uint16_t port,

Quoted 4 lines

kvm-all.c +3 -0

+/* 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.

Quoted 2 lines

what about other arches?

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

Is there any objection to including this patch?

Quoted 21 lines · Marcelo Tosatti

Quoted 19 lines · Avi Kivity

Quoted 8 lines · Marcelo Tosatti

Cc "Michael S. Tsirkin"<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>

Index: qemu/kvm-all.c
===================================================================

kvm-all.c static int kvm_handle_io(uint16_t port, · line 724

Quoted 7 lines

kvm-all.c +3 -0

@@ -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?

Quoted 4 lines

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.

Quoted 4 lines

Quoted 2 lines

what about other arches?

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

Quoted 2 lines

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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 9 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines

Quoted 7 lines

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

Quoted 2 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 4 lines

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?

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by Marcelo Tosatti <[email protected]>

Index: qemu/kvm-all.c
===================================================================

kvm-all.c +4 -1

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

Acked-by "Michael S. Tsirkin"<[email protected]>

Signed-off-by Marcelo Tosatti<[email protected]>