performance improvement for windows guests, running on top of virtio block device

The following patch allows us to improve Windows virtio block driver performance on small size requests. Additionally, it leads to reducing of cpu usage on write IOs

hw/virtio-blk.c +3 -0

index a2f0639..0e3a8d5 100644
@@ -28,6 +28,7 @@ typedef struct VirtIOBlock
      char serial_str[BLOCK_SERIAL_STRLEN + 1];
      QEMUBH *bh;
      size_t config_size;
+    unsigned int pending;
  } VirtIOBlock;
 
  static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)

@@ -87,6 +88,8 @@ typedef struct VirtIOBlockReq
      struct VirtIOBlockReq *next;
  } VirtIOBlockReq;
 
+static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue
 *vq);
+
  static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
  {

      VirtIOBlock *s = req->dev;

hw/virtio-blk.c +0 -0

@@ -95,6 +98,11 @@ static void virtio_blk_req_complete(VirtIOBlockReq
 *req, int status)

hw/virtio-blk.c +3 -0

@@ -340,6 +348,9 @@ static void virtio_blk_handle_output(VirtIODevice
 *vdev, VirtQueue *vq)
              exit(1);
          }
 
+        if(++s->pending == 1)
+            virtio_queue_set_notification(s->vq, 0);
+
          req->out = (void *)req->elem.out_sg[0].iov_base;
          req->in = (void *)req->elem.in_sg[req->elem.in_num -

1].iov_base;

Quoted 36 lines · Avi Kivity

Quoted 4 lines · Vadim Rozenfeld

The following patch allows us to improve Windows virtio block driver performance on small size requests. Additionally, it leads to reducing of cpu usage on write IOs

Note, this is not an improvement for Windows specifically.

Quoted 28 lines

hw/virtio-blk.c +3 -0

index a2f0639..0e3a8d5 100644
@@ -28,6 +28,7 @@ typedef struct VirtIOBlock
 char serial_str[BLOCK_SERIAL_STRLEN + 1];
 QEMUBH *bh;
 size_t config_size;
+ unsigned int pending;
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)

@@ -87,6 +88,8 @@ typedef struct VirtIOBlockReq
 struct VirtIOBlockReq *next;
 } VirtIOBlockReq;
 
+static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue
 *vq);
+
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {

VirtIOBlock *s = req->dev;

hw/virtio-blk.c +0 -0

@@ -95,6 +98,11 @@ static void virtio_blk_req_complete(VirtIOBlockReq
 *req, int status)

virtqueue_push(s->vq,&req->elem, req->qiov.size + sizeof(*req->in));
virtio_notify(&s->vdev, s->vq);

Quoted 3 lines

Quoted 3 lines

+ if(--s->pending == 0) {
+ virtio_queue_set_notification(s->vq, 1);
+ virtio_blk_handle_output(&s->vdev, s->vq);

The above line should be moved out of the 'if'.

Attached results with rhel5.4 (qemu0.11) for win2k8 32bit guest. Note the drastic reduction in cpu consumption.

Quoted 30 lines

Quoted 3 lines

+ }
+

Coding style: space after if. See the CODING_STYLE file.

Quoted 9 lines

hw/virtio-blk.c +3 -0

@@ -340,6 +348,9 @@ static void virtio_blk_handle_output(VirtIODevice
 *vdev, VirtQueue *vq)
 exit(1);
 }
 
+ if(++s->pending == 1)
+ virtio_queue_set_notification(s->vq, 0);
+
 req->out = (void *)req->elem.out_sg[0].iov_base;

Quoted 3 lines

req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;

Coding style: space after if, braces after if.

Your patch is word wrapped, please send it correctly. Easiest using git send-email.

The patch has potential to reduce performance on volumes with multiple spindles. Consider two processes issuing sequential reads into a RAID array. With this patch, the reads will be executed sequentially rather than in parallel, so I think a follow-on patch to make the minimum depth a parameter (set by the guest? the host?) would be helpful.

Quoted 2 lines

Attached results with rhel5.4 (qemu0.11) for win2k8 32bit guest. Note the drastic reduction in cpu consumption.

Attachment did not survive the email server, so you'll have to trust me saying that cpu consumption was done from 65% -> 40% for reads and from 80% --> 30% for writes

FYI below is the manually applied patch without all the wrapping:
static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)

  {
      VirtIOBlock *s = req->dev;

As Dor points out, the call to virtio_blk_handle_output() wants to be before the test for pending, so we scan the ring as early as possible

The patch has potential to reduce performance on volumes with multiple spindles. Consider two processes issuing sequential reads into a RAID array. With this patch, the reads will be executed sequentially rather than in parallel, so I think a follow-on patch to make the minimum depth a parameter (set by the guest? the host?) would be helpful.

As Dor points out, the call to virtio_blk_handle_output() wants to be before the test for pending, so we scan the ring as early as possible

Quoted 6 lines · Avi Kivity

The patch has potential to reduce performance on volumes with multiple spindles. Consider two processes issuing sequential reads into a RAID array. With this patch, the reads will be executed sequentially rather than in parallel, so I think a follow-on patch to make the minimum depth a parameter (set by the guest? the host?) would be helpful.

Let's think about the life cycle of I/O requests a bit.

We have an idle virtqueue (aka one virtio-blk device). The first (read) request comes in, we get the virtio notify from the guest, which calls into virtio_blk_handle_output. With the new code we now disable the notify once we start processing the first request. If the second request hits the queue before we call into virtio_blk_get_request the second time we're fine even with the new code as we keep picking it up. If however it hits after we leave virtio_blk_handle_output, but before we complete the first request we do indeed introduce additional latency.

So instead of disabling notify while requests are active we might want to only disable it while we are inside virtio_blk_handle_output. Something like the following minimally tested patch:

Quoted 3 lines · Avi Kivity

As Dor points out, the call to virtio_blk_handle_output() wants to be before the test for pending, so we scan the ring as early as possible

It could cause a race window where we add an entry to the ring after we run virtio_blk_handle_output, but before re-enabling the notification. But I think my variant of the patch that I just posted should deal with this in an even better way.

Quoted 6 lines · Avi Kivity

The patch has potential to reduce performance on volumes with multiple spindles. Consider two processes issuing sequential reads into a RAID array. With this patch, the reads will be executed sequentially rather than in parallel, so I think a follow-on patch to make the minimum depth a parameter (set by the guest? the host?) would be helpful.

Let's think about the life cycle of I/O requests a bit.

We have an idle virtqueue (aka one virtio-blk device). The first (read) request comes in, we get the virtio notify from the guest, which calls into virtio_blk_handle_output. With the new code we now disable the notify once we start processing the first request. If the second request hits the queue before we call into virtio_blk_get_request the second time we're fine even with the new code as we keep picking it up. If however it hits after we leave virtio_blk_handle_output, but before we complete the first request we do indeed introduce additional latency.

So instead of disabling notify while requests are active we might want to only disable it while we are inside virtio_blk_handle_output. Something like the following minimally tested patch:

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

hw/virtio-blk.c +11 -0

@@ -328,7 +328,15 @@ static void virtio_blk_handle_output(Vir
      int num_writes = 0;
      BlockDriverState *old_bs = NULL;
 
+    /*
+     * While we are processing requests there is no need to get further
+     * notifications from the guest - it'll just burn cpu cycles doing
+     * useless context switches into the host.
+     */
+    virtio_queue_set_notification(s->vq, 0);
+
      while ((req = virtio_blk_get_request(s))) {
+handle_request:
          if (req->elem.out_num<  1 || req->elem.in_num<  1) {
              fprintf(stderr, "virtio-blk missing headers\n");
              exit(1);

@@ -358,6 +366,18 @@ static void virtio_blk_handle_output(Vir
          }
      }
 
+    /*
+     * Once we're done processing all pending requests re-enable the queue
+     * notification.  If there's an entry pending after we enabled

Quoted 3 lines

So instead of disabling notify while requests are active we might want to only disable it while we are inside virtio_blk_handle_output. Something like the following minimally tested patch:

I'd suggest that we get even more aggressive and install an idle bottom half that checks the queue for newly submitted requests. If we keep getting requests submitted before a new one completes, we'll never take an I/O exit.

The same approach is probably a good idea for virtio-net.

Quoted 7 lines · Anthony Liguori

Quoted 3 lines

So instead of disabling notify while requests are active we might want to only disable it while we are inside virtio_blk_handle_output. Something like the following minimally tested patch:

I'd suggest that we get even more aggressive and install an idle bottom half that checks the queue for newly submitted requests. If we keep getting requests submitted before a new one completes, we'll never take an I/O exit.

That has the downside of bouncing a cache line on unrelated exits.

It probably doesn't matter with qemu as it is now, since it will
bounce qemu_mutex, but it will hurt with large guests (especially if they have many rings).

IMO we should get things to work well without riding on unrelated exits, especially as we're trying to reduce those exits.

Quoted 1 line

The same approach is probably a good idea for virtio-net.

With vhost-net you don't see exits.

Quoted 1 line

That has the downside of bouncing a cache line on unrelated exits.

The read and write sides of the ring are widely separated in physical memory specifically to avoid cache line bouncing.

Quoted 4 lines

It probably doesn't matter with qemu as it is now, since it will
bounce qemu_mutex, but it will hurt with large guests (especially if they have many rings).

IMO we should get things to work well without riding on unrelated exits, especially as we're trying to reduce those exits.

A block I/O request can potentially be very, very long lived. By serializing requests like this, there's a high likelihood that it's going to kill performance with anything capable of processing multiple requests.

OTOH, if we aggressively poll the ring when we have an opportunity to, there's very little down side to that and it addresses the serialization problem.

Quoted 4 lines · Anthony Liguori

Quoted 1 line

That has the downside of bouncing a cache line on unrelated exits.

The read and write sides of the ring are widely separated in physical memory specifically to avoid cache line bouncing.

I meant, exits on random vcpus will cause the cacheline containing the notification disable flag to bounce around. As it is, we read it on the vcpu that owns the queue and write it on that vcpu or the I/O thread.

Quoted 6 lines

Quoted 4 lines

It probably doesn't matter with qemu as it is now, since it will
bounce qemu_mutex, but it will hurt with large guests (especially if they have many rings).

IMO we should get things to work well without riding on unrelated exits, especially as we're trying to reduce those exits.

A block I/O request can potentially be very, very long lived. By serializing requests like this, there's a high likelihood that it's going to kill performance with anything capable of processing multiple requests.

Right, that's why I suggested having a queue depth at which disabling notification kicks in. The patch hardcodes this depth to 1, unpatched qemu is infinite, a good value is probably spindle count + VAT.

Quoted 1 line

OTOH, if we aggressively poll the ring when we have an opportunity to, there's very little down side to that and it addresses the serialization problem.

But we can't guarantee that we'll get those opportunities, so it doesn't address the problem in a general way. A guest that doesn't use hpet and only has a single virtio-blk device will not have any reason to exit to qemu.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

OTOH, if we aggressively poll the ring when we have an opportunity to, there's very little down side to that and it addresses the serialization problem.

But we can't guarantee that we'll get those opportunities, so it doesn't address the problem in a general way. A guest that doesn't use hpet and only has a single virtio-blk device will not have any reason to exit to qemu.

We can mitigate this with a timer but honestly, we need to do perf measurements to see. My feeling is that we will need some more aggressive form of polling than just waiting for IO completion. I don't think queue depth is enough because it assumes that all requests are equal. When dealing with cache=off or even just storage with it's own cache, that's simply not the case.

Maybe we can adapt behaviour dynamically based on how fast results come in.

Quoted 7 lines · Avi Kivity

Quoted 4 lines · Anthony Liguori

Quoted 1 line

That has the downside of bouncing a cache line on unrelated exits.

The read and write sides of the ring are widely separated in physical memory specifically to avoid cache line bouncing.

I meant, exits on random vcpus will cause the cacheline containing the notification disable flag to bounce around. As it is, we read it on the vcpu that owns the queue and write it on that vcpu or the I/O thread.

Bottom halves are always run from the IO thread.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

It probably doesn't matter with qemu as it is now, since it will
bounce qemu_mutex, but it will hurt with large guests (especially if they have many rings).

IMO we should get things to work well without riding on unrelated exits, especially as we're trying to reduce those exits.

A block I/O request can potentially be very, very long lived. By serializing requests like this, there's a high likelihood that it's going to kill performance with anything capable of processing multiple requests.

Right, that's why I suggested having a queue depth at which disabling notification kicks in. The patch hardcodes this depth to 1, unpatched qemu is infinite, a good value is probably spindle count + VAT.

That means we would need a user visible option which is quite unfortunate.

Also, that logic only really makes sense with cache=off. With cache=writethrough, you can get pathological cases whereas you have an uncached access followed by cached accesses. In fact, with read-ahead, this is probably not an uncommon scenario.

Quoted 3 lines

Quoted 1 line

OTOH, if we aggressively poll the ring when we have an opportunity to, there's very little down side to that and it addresses the serialization problem.

But we can't guarantee that we'll get those opportunities, so it doesn't address the problem in a general way. A guest that doesn't use hpet and only has a single virtio-blk device will not have any reason to exit to qemu.

We can mitigate this with a timer but honestly, we need to do perf measurements to see. My feeling is that we will need some more aggressive form of polling than just waiting for IO completion. I don't think queue depth is enough because it assumes that all requests are equal. When dealing with cache=off or even just storage with it's own cache, that's simply not the case.

Based on our experiences with virtio-net, what I'd suggest is to make a lot of tunable options (ring size, various tx mitigation schemes, timeout durations, etc) and then we can do some deep performance studies to see how things interact with each other.

I think we should do that before making any changes because I'm deeply concerned that we'll introduce significant performance regressions.

Quoted 5 lines · Anthony Liguori

Based on our experiences with virtio-net, what I'd suggest is to make a lot of tunable options (ring size, various tx mitigation schemes, timeout durations, etc) and then we can do some deep performance studies to see how things interact with each other.

I think we should do that before making any changes because I'm deeply concerned that we'll introduce significant performance regressions.

I agree. We can start with this patch, with a tunable depth, defaulting to current behaviour.

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

Based on our experiences with virtio-net, what I'd suggest is to make a lot of tunable options (ring size, various tx mitigation schemes, timeout durations, etc) and then we can do some deep performance studies to see how things interact with each other.

I think we should do that before making any changes because I'm deeply concerned that we'll introduce significant performance regressions.

I agree. We can start with this patch, with a tunable depth, defaulting to current behaviour.

I wouldn't be opposed to that provided we made it clear that these options were not supported long term. I don't want management tools (like libvirt) to start relying on them.

Quoted 8 lines · Anthony Liguori

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

Based on our experiences with virtio-net, what I'd suggest is to make a lot of tunable options (ring size, various tx mitigation schemes, timeout durations, etc) and then we can do some deep performance studies to see how things interact with each other.

I think we should do that before making any changes because I'm deeply concerned that we'll introduce significant performance regressions.

Quoted 4 lines

Quoted 1 line

I agree. We can start with this patch, with a tunable depth, defaulting to current behaviour.

I wouldn't be opposed to that provided we made it clear that these options were not supported long term. I don't want management tools (like libvirt) to start relying on them.

x-option-name for experimental options?

-device disk,if=virtio,x-queue-depth-suppress-notify=4

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

Quoted 3 lines

So instead of disabling notify while requests are active we might want to only disable it while we are inside virtio_blk_handle_output. Something like the following minimally tested patch:

Quoted 7 lines

Quoted 5 lines

I'd suggest that we get even more aggressive and install an idle bottom half that checks the queue for newly submitted requests. If we keep getting requests submitted before a new one completes, we'll never take an I/O exit.

That has the downside of bouncing a cache line on unrelated exits.

The read and write sides of the ring are widely separated in physical memory specifically to avoid cache line bouncing.

Quoted 6 lines

It probably doesn't matter with qemu as it is now, since it will
bounce qemu_mutex, but it will hurt with large guests (especially if they have many rings).

IMO we should get things to work well without riding on unrelated exits, especially as we're trying to reduce those exits.

A block I/O request can potentially be very, very long lived. By serializing requests like this, there's a high likelihood that it's going to kill performance with anything capable of processing multiple requests.

OTOH, if we aggressively poll the ring when we have an opportunity to, there's very little down side to that and it addresses the serialization problem.

Quoted 3 lines

Quoted 1 line

The same approach is probably a good idea for virtio-net.

With vhost-net you don't see exits.

The point is, when we've disabled notification, we should poll on the ring for additional requests instead of waiting for one to complete before looking at another one.

Even with vhost-net, this logic is still applicable although potentially harder to achieve.

Regards,

Anthony Liguori

Quoted 11 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

Quoted 1 line

That has the downside of bouncing a cache line on unrelated exits.

The read and write sides of the ring are widely separated in physical memory specifically to avoid cache line bouncing.

I meant, exits on random vcpus will cause the cacheline containing the notification disable flag to bounce around. As it is, we read it on the vcpu that owns the queue and write it on that vcpu or the I/O thread.

Bottom halves are always run from the IO thread.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

It probably doesn't matter with qemu as it is now, since it will
bounce qemu_mutex, but it will hurt with large guests (especially if they have many rings).

IMO we should get things to work well without riding on unrelated exits, especially as we're trying to reduce those exits.

Quoted 8 lines

Quoted 4 lines

A block I/O request can potentially be very, very long lived. By serializing requests like this, there's a high likelihood that it's going to kill performance with anything capable of processing multiple requests.

Right, that's why I suggested having a queue depth at which disabling notification kicks in. The patch hardcodes this depth to 1, unpatched qemu is infinite, a good value is probably spindle count + VAT.

That means we would need a user visible option which is quite unfortunate.

Also, that logic only really makes sense with cache=off. With cache=writethrough, you can get pathological cases whereas you have an uncached access followed by cached accesses. In fact, with read-ahead, this is probably not an uncommon scenario.

Quoted 8 lines

Quoted 3 lines

OTOH, if we aggressively poll the ring when we have an opportunity to, there's very little down side to that and it addresses the serialization problem.

But we can't guarantee that we'll get those opportunities, so it doesn't address the problem in a general way. A guest that doesn't use hpet and only has a single virtio-blk device will not have any reason to exit to qemu.

We can mitigate this with a timer but honestly, we need to do perf measurements to see. My feeling is that we will need some more aggressive form of polling than just waiting for IO completion. I don't think queue depth is enough because it assumes that all requests are equal. When dealing with cache=off or even just storage with it's own cache, that's simply not the case.

Regards,

Anthony Liguori

Bottom halves are run at the very end of the event loop which means that they're guaranteed to be the last thing run. idle bottom halves can be rescheduled without causing an infinite loop and do not affect the select timeout (which normal bottom halves do).

Quoted 5 lines

Bottom halves are run at the very end of the event loop which means that they're guaranteed to be the last thing run. idle bottom halves can be rescheduled without causing an infinite loop and do not affect the select timeout (which normal bottom halves do).

Idle bottom halves (i.e. qemu_bh_schedule_idle) are just bugs waiting to happen, and should never be used for anything.

Paul

Quoted 2 lines

Idle bottom halves (i.e. qemu_bh_schedule_idle) are just bugs waiting to happen, and should never be used for anything.

Idle bottom halves make considerable more sense than the normal bottom halves.

The fact that rescheduling a bottom half within a bottom half results in an infinite loop is absurd. It is equally absurd that bottoms halves alter the select timeout. The result of that is that if a bottom half schedules another bottom half, and that bottom half schedules the previous, you get a tight infinite loop. Since bottom halves are used often times deep within functions, the result is very subtle infinite loops (that we've absolutely encountered in the past).

A main loop should have only a few characteristics. It should enable timeouts (based on select), it should enable fd event dispatch, and it should allow for idle functions to be registered. There should be no guarantees on when idle functions are executed other than they'll eventually be executed.

The way we use "bottom halves" today should be implemented in terms of a relative timeout of 0 or an absolute timeout of now. The fact that we can't do that in our main loop is due to the very strange dependencies deep within various devices on io dispatch ordering. I would love to eliminate this but I've not been able to spend any time on this recently.

Quoted 6 lines

Quoted 3 lines

Idle bottom halves (i.e. qemu_bh_schedule_idle) are just bugs waiting to happen, and should never be used for anything.

Idle bottom halves make considerable more sense than the normal bottom halves.

Quoted 8 lines

The fact that rescheduling a bottom half within a bottom half results in an infinite loop is absurd. It is equally absurd that bottoms halves alter the select timeout. The result of that is that if a bottom half schedules another bottom half, and that bottom half schedules the previous, you get a tight infinite loop. Since bottom halves are used often times deep within functions, the result is very subtle infinite loops (that we've absolutely encountered in the past).

I disagree. The "select timeout" is a completely irrelevant implementation detail. Anything that relies on it is just plain wrong.

If you require a delay
then you should be using a timer. If scheduling a BH directly then you should expect it to be processed without delay.

If a BH reschedules itself (indirectly or indirectly) without useful work occuring then you absolutely should expect an infinite loop. Rescheduling itself after doing useful work should never cause an infinite loop. The only way it can loop inifinitely is if we have infinite amount of work to do, in which case you loose either way. Looping over work via recursive BHs is probably not the most efficient way to do things, but I guess it may sometimes be the simplest in practice.

Interaction between multiple BH is slightly trickier. By my reading BH are processed in the order they are created. It may be reasonable to guarantee that BH are processed in the order they are scheduled. However I'm reluctant to even go that far without a good use-case. You could probably come up with arguments for processing them in most-recently-scheduled order.

Quoted 6 lines

A main loop should have only a few characteristics. It should enable timeouts (based on select), it should enable fd event dispatch, and it should allow for idle functions to be registered. There should be no guarantees on when idle functions are executed other than they'll eventually be executed.

If you don't provide any guarantees, then surely processing them immediately must be an acceptable implementation. I don't believe there is a useful definition of "idle".

All existing uses of qemu_bh_schedule_idle are in fact poorly implemented periodic polling. Furthermore these should not be using periodic polling, they can and should be event driven. They only exist because noone has bothered to fix the old code properly. Having arbitrary 10ms latency on DMA transfers is just plain wrong.

Quoted 6 lines

The way we use "bottom halves" today should be implemented in terms of a relative timeout of 0 or an absolute timeout of now. The fact that we can't do that in our main loop is due to the very strange dependencies deep within various devices on io dispatch ordering. I would love to eliminate this but I've not been able to spend any time on this recently.

I don't see how this helps. A self-triggering event with a timeout of "now" is still an infinite loop. Any delay is a bugs in the dispatch loop. "idle" BHs are relying on this bug.

Paul

Very simply, without idle bottom halves, there's no way to implement polling with the main loop. If we dropped idle bottom halves, we would have to add explicit polling back to the main loop.

How would you implement polling?

Quoted 3 lines

I don't see how this helps. A self-triggering event with a timeout of "now" is still an infinite loop. Any delay is a bugs in the dispatch loop. "idle" BHs are relying on this bug.

The main point is that BHs should not be implemented in the actual main loop and that "idle" BHs are really the only type of BHs that should exist as far as the main loop is concerned. s/"idle" BHs/idle callbacks/g and I think we're at a more agreeable place.

Quoted 6 lines

Quoted 3 lines

Idle bottom halves (i.e. qemu_bh_schedule_idle) are just bugs waiting to happen, and should never be used for anything.

Idle bottom halves make considerable more sense than the normal bottom halves.

Quoted 8 lines

The fact that rescheduling a bottom half within a bottom half results in an infinite loop is absurd. It is equally absurd that bottoms halves alter the select timeout. The result of that is that if a bottom half schedules another bottom half, and that bottom half schedules the previous, you get a tight infinite loop. Since bottom halves are used often times deep within functions, the result is very subtle infinite loops (that we've absolutely encountered in the past).

I disagree. The "select timeout" is a completely irrelevant implementation detail. Anything that relies on it is just plain wrong. If you require a delay then you should be using a timer. If scheduling a BH directly then you should expect it to be processed without delay.

Quoted 12 lines · Paul Brook

Quoted 12 lines

Quoted 2 lines

Idle bottom halves (i.e. qemu_bh_schedule_idle) are just bugs waiting to happen, and should never be used for anything.

Idle bottom halves make considerable more sense than the normal bottom halves.

The fact that rescheduling a bottom half within a bottom half results in an infinite loop is absurd. It is equally absurd that bottoms halves alter the select timeout. The result of that is that if a bottom half schedules another bottom half, and that bottom half schedules the previous, you get a tight infinite loop. Since bottom halves are used often times deep within functions, the result is very subtle infinite loops (that we've absolutely encountered in the past).

Quoted 4 lines

I disagree. The "select timeout" is a completely irrelevant implementation detail. Anything that relies on it is just plain wrong. If you require a delay then you should be using a timer. If scheduling a BH directly then you should expect it to be processed without delay.

I agree. Further, once we fine-grain device threading, the iothread essentially disappears and is replaced by device-specific threads. There's no "idle" anymore.

Quoted 1 line

I agree. Further, once we fine-grain device threading, the iothread essentially disappears and is replaced by device-specific threads. There's no "idle" anymore.

That's a nice idea, but how is io dispatch handled? Is everything synchronous or do we continue to program asynchronously?

It's very difficult to mix concepts.

I personally don't anticipate per-device threading but rather anticipate re-entrant device models. I would expect all I/O to be dispatched within the I/O thread and the VCPU threads to be able to execute device models simultaneously with the I/O thread.

Quoted 7 lines · Anthony Liguori

Quoted 3 lines

I agree. Further, once we fine-grain device threading, the iothread essentially disappears and is replaced by device-specific threads. There's no "idle" anymore.

That's a nice idea, but how is io dispatch handled? Is everything synchronous or do we continue to program asynchronously?

Simple stuff can be kept asynchronous, complex stuff (like qcow2) ought to be made synchronous (it uses threads anyway, so we don't lose anything). Stuff like vnc can go either way.

Quoted 2 lines

It's very difficult to mix concepts.

We're complicated enough to have conflicting requirements and a large code base with its own inertia, so no choice really.

Quoted 4 lines

I personally don't anticipate per-device threading but rather anticipate re-entrant device models. I would expect all I/O to be dispatched within the I/O thread and the VCPU threads to be able to execute device models simultaneously with the I/O thread.

That means long-running operations on the iothread can lock out other completions.

Candidates for own threads are:
- live migration
- block format drivers (except linux-aio, perhaps have a thread for the aio completion handler)
- vnc
- sdl
- sound?

- hotplug, esp. memory

Each such thread could run the same loop as the iothread. Any pollable fd or timer would be associated with a thread, so things continue as normal more or less. Unassociated objects continue with the main iothread.

Quoted 4 lines · Anthony Liguori

Quoted 1 line

I agree. Further, once we fine-grain device threading, the iothread essentially disappears and is replaced by device-specific threads. There's no "idle" anymore.

That's a nice idea, but how is io dispatch handled? Is everything synchronous or do we continue to program asynchronously?

Simple stuff can be kept asynchronous, complex stuff (like qcow2) ought to be made synchronous (it uses threads anyway, so we don't lose anything). Stuff like vnc can go either way.

Quoted 2 lines

It's very difficult to mix concepts.

We're complicated enough to have conflicting requirements and a large code base with its own inertia, so no choice really.

Quoted 1 line

I personally don't anticipate per-device threading but rather anticipate re-entrant device models. I would expect all I/O to be dispatched within the I/O thread and the VCPU threads to be able to execute device models simultaneously with the I/O thread.

That means long-running operations on the iothread can lock out other completions.

Candidates for own threads are:
- live migration
- block format drivers (except linux-aio, perhaps have a thread for the aio completion handler)
- vnc
- sdl
- sound?
- hotplug, esp. memory

Each such thread could run the same loop as the iothread. Any pollable fd or timer would be associated with a thread, so things continue as normal more or less. Unassociated objects continue with the main iothread.

Quoted 7 lines · Avi Kivity

Quoted 4 lines · Anthony Liguori

Quoted 1 line

I agree. Further, once we fine-grain device threading, the iothread essentially disappears and is replaced by device-specific threads. There's no "idle" anymore.

That's a nice idea, but how is io dispatch handled? Is everything synchronous or do we continue to program asynchronously?

Simple stuff can be kept asynchronous, complex stuff (like qcow2) ought to be made synchronous (it uses threads anyway, so we don't lose anything). Stuff like vnc can go either way.

We've discussed this before and I still contend that threads do not make qcow2 any simpler.

Quoted 2 lines

Each such thread could run the same loop as the iothread. Any pollable fd or timer would be associated with a thread, so things continue as normal more or less. Unassociated objects continue with the main iothread.

Is the point latency or increasing available CPU resources?

If the device models are re-entrant, that reduces a ton of the demand on the qemu_mutex which means that IO thread can run uncontended. While we have evidence that the VCPU threads and IO threads are competing with each other today, I don't think we have any evidence to suggest that the IO thread is self-starving itself with long running events.

With the device model, I'd like to see us move toward a very well defined API for each device to use. Part of the reason for this is to limit the scope of the devices in such a way that we can enforce this at compile time. Then we can introduce locking within devices with some level of guarantee that we've covered the API devices are actually consuming.

For host services though, it's much more difficult to isolate them like this.

I'm not necessarily claiming that this will never be the right thing to do, but I don't think we really have the evidence today to suggest that we should focus on this in the short term.

qcow2 is still not fully asynchronous. All the other format drivers (except raw) are fully synchronous. If we had a threaded infrastructure, we could convert them all in a day. As it is, you can only use the other block format drivers in 'qemu-img convert'.

Quoted 5 lines

Quoted 3 lines

Each such thread could run the same loop as the iothread. Any pollable fd or timer would be associated with a thread, so things continue as normal more or less. Unassociated objects continue with the main iothread.

Is the point latency or increasing available CPU resources?

Yes.

Quoted 1 line

If the device models are re-entrant, that reduces a ton of the demand on the qemu_mutex which means that IO thread can run uncontended. While we have evidence that the VCPU threads and IO threads are competing with each other today, I don't think we have any evidence to suggest that the IO thread is self-starving itself with long running events.

I agree we have no evidence and that this is all speculation. But consider a 64-vcpu guest, it has a 1:64 ratio of vcpu time (initiations) to iothread time (completions). If each vcpu generates 5000 initiations per second, the iothread needs to handle 320,000 completions per second. At that rate you will see some internal competition. That thread will also have a hard time shuffling data since every completion's data will reside in the wrong cpu cache.

Note, an alternative to multiple iothreads is to move completion handling back to vcpus, provided we can steer the handler close to the guest completion handler.

Quoted 2 lines

For host services though, it's much more difficult to isolate them like this.

What do you mean by host services?

Quoted 1 line

I'm not necessarily claiming that this will never be the right thing to do, but I don't think we really have the evidence today to suggest that we should focus on this in the short term.

Agreed. We will start to see evidence (one way or the other) as fully loaded 64-vcpu guests are benchmarked. Another driver may be real-time guests; if a timer can be deferred by some block device initiation or completion, then we can say goodbye to any realtime guarantees we want to make.

Quoted 1 line · Avi Kivity

qcow2 is still not fully asynchronous. All the other format drivers (except raw) are fully synchronous. If we had a threaded infrastructure, we could convert them all in a day. As it is, you can only use the other block format drivers in 'qemu-img convert'.

I've got a healthy amount of scepticism that it's that easy. But I'm happy to consider patches :-)

Quoted 4 lines

Quoted 2 lines

If the device models are re-entrant, that reduces a ton of the demand on the qemu_mutex which means that IO thread can run uncontended. While we have evidence that the VCPU threads and IO threads are competing with each other today, I don't think we have any evidence to suggest that the IO thread is self-starving itself with long running events.

I agree we have no evidence and that this is all speculation. But consider a 64-vcpu guest, it has a 1:64 ratio of vcpu time (initiations) to iothread time (completions). If each vcpu generates 5000 initiations per second, the iothread needs to handle 320,000 completions per second. At that rate you will see some internal competition. That thread will also have a hard time shuffling data since every completion's data will reside in the wrong cpu cache.

Ultimately, it depends on what you're optimizing for. If you've got a 64-vcpu guest on a 128-way box, then sure, we want to have 64 IO threads because that will absolutely increase throughput.

But realistically, it's more likely that if you've got a 64-vcpu guest, you're on a 1024-way box and you've got 64 guests running at once. Having 64 IO threads per VM means you've got 4k threads floating. It's still just as likely that one completion will get delayed by something less important. Now with all of these threads on a box like this, you get nasty NUMA interactions too.

The difference between the two models is that with threads, we rely on pre-emption to enforce fairness and the Linux scheduler to perform scheduling. With a single IO thread, we're determining execution order and priority.

A lot of main loops have a notion of priority for timer and idle callbacks. For something that is latency sensitive, you absolutely could introduce the concept of priority for bottom halves. It would ensure that a +1 priority bottom half would get scheduled before handling any lower priority I/O/BHs.

Quoted 2 lines

Note, an alternative to multiple iothreads is to move completion handling back to vcpus, provided we can steer the handler close to the guest completion handler.

Looking at something like linux-aio, I think we might actually want to do that. We can submit the request from the VCPU thread and we can certainly program the signal to get delivered to that VCPU thread. Maintaining affinity for the request is likely a benefit.

Quoted 5 lines

Quoted 3 lines

For host services though, it's much more difficult to isolate them like this.

What do you mean by host services?

Things like VNC and live migration. Things that aren't directly related to a guest's activity. One model I can imagine is to continue to relegate these things to a single IO thread, but then move device driven callbacks either back to the originating thread or to a dedicated device callback thread. Host services generally have a much lower priority.

Quoted 4 lines

Quoted 2 lines

I'm not necessarily claiming that this will never be the right thing to do, but I don't think we really have the evidence today to suggest that we should focus on this in the short term.

Agreed. We will start to see evidence (one way or the other) as fully loaded 64-vcpu guests are benchmarked. Another driver may be real-time guests; if a timer can be deferred by some block device initiation or completion, then we can say goodbye to any realtime guarantees we want to make.

I'm wary of making decisions based on performance of a 64-vcpu guest. It's an important workload to characterize because it's an extreme case but I think 64 1-vcpu guests will continue to be significantly more important than 1 64-vcpu guest.