| Message ID | 1406720388-18671-15-git-send-email-ming.lei@canonical.com |
|---|---|
| State | New |
| Headers | show |
Il 30/07/2014 13:39, Ming Lei ha scritto: > Now we only support multi vqs for dataplane case. > > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > hw/block/virtio-blk.c | 18 +++++++++++++++++- > include/hw/virtio/virtio-blk.h | 1 + > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index ab99156..44e9956 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -556,6 +556,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > blkcfg.physical_block_exp = get_physical_block_exp(s->conf); > blkcfg.alignment_offset = 0; > blkcfg.wce = bdrv_enable_write_cache(s->bs); > + stw_p(&blkcfg.num_queues, s->blk.num_queues); > memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); > } > > @@ -590,6 +591,12 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features) > if (bdrv_is_read_only(s->bs)) > features |= 1 << VIRTIO_BLK_F_RO; > > +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > + if (s->blk.num_queues > 1) { > + features |= 1 << VIRTIO_BLK_F_MQ; > + } > +#endif > + > return features; > } > > @@ -739,8 +746,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > Error *err = NULL; > #endif > + int i; > static int virtio_blk_id; > > +#ifndef CONFIG_VIRTIO_BLK_DATA_PLANE > + blk->num_queues = 1; > +#endif > + > if (!blk->conf.bs) { > error_setg(errp, "drive property not set"); > return; > @@ -765,7 +777,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > s->rq = NULL; > s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; > > - s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output); > + s->vqs = g_malloc0(sizeof(VirtQueue *) * blk->num_queues); > + for (i = 0; i < blk->num_queues; i++) > + s->vqs[i] = virtio_add_queue(vdev, 128, virtio_blk_handle_output); > + s->vq = s->vqs[0]; > s->complete_request = virtio_blk_complete_request; > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > virtio_blk_data_plane_create(vdev, blk, &s->dataplane, &err); > @@ -802,6 +817,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) > qemu_del_vm_change_state_handler(s->change); > unregister_savevm(dev, "virtio-blk", s); > blockdev_mark_auto_del(s->bs); > + g_free(s->vqs); > virtio_cleanup(vdev); > } > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index ad70c9a..b4609dd 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -132,6 +132,7 @@ typedef struct VirtIOBlock { > VirtIODevice parent_obj; > BlockDriverState *bs; > VirtQueue *vq; > + VirtQueue **vqs; > void *rq; > QEMUBH *bh; > BlockConf *conf; > Dataplane must not be a change to the guest ABI. If you implement this feature you have to implement it for both dataplane and non-dataplne. Paolo
On Wed, Jul 30, 2014 at 04:01:59PM +0200, Paolo Bonzini wrote: > Il 30/07/2014 13:39, Ming Lei ha scritto: > > Now we only support multi vqs for dataplane case. > > > > Signed-off-by: Ming Lei <ming.lei@canonical.com> > > --- > > hw/block/virtio-blk.c | 18 +++++++++++++++++- > > include/hw/virtio/virtio-blk.h | 1 + > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index ab99156..44e9956 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -556,6 +556,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > > blkcfg.physical_block_exp = get_physical_block_exp(s->conf); > > blkcfg.alignment_offset = 0; > > blkcfg.wce = bdrv_enable_write_cache(s->bs); > > + stw_p(&blkcfg.num_queues, s->blk.num_queues); > > memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); > > } > > > > @@ -590,6 +591,12 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features) > > if (bdrv_is_read_only(s->bs)) > > features |= 1 << VIRTIO_BLK_F_RO; > > > > +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > > + if (s->blk.num_queues > 1) { > > + features |= 1 << VIRTIO_BLK_F_MQ; > > + } > > +#endif > > + > > return features; > > } > > > > @@ -739,8 +746,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > > Error *err = NULL; > > #endif > > + int i; > > static int virtio_blk_id; > > > > +#ifndef CONFIG_VIRTIO_BLK_DATA_PLANE > > + blk->num_queues = 1; > > +#endif > > + > > if (!blk->conf.bs) { > > error_setg(errp, "drive property not set"); > > return; > > @@ -765,7 +777,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > > s->rq = NULL; > > s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; > > > > - s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output); > > + s->vqs = g_malloc0(sizeof(VirtQueue *) * blk->num_queues); > > + for (i = 0; i < blk->num_queues; i++) > > + s->vqs[i] = virtio_add_queue(vdev, 128, virtio_blk_handle_output); > > + s->vq = s->vqs[0]; > > s->complete_request = virtio_blk_complete_request; > > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > > virtio_blk_data_plane_create(vdev, blk, &s->dataplane, &err); > > @@ -802,6 +817,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) > > qemu_del_vm_change_state_handler(s->change); > > unregister_savevm(dev, "virtio-blk", s); > > blockdev_mark_auto_del(s->bs); > > + g_free(s->vqs); > > virtio_cleanup(vdev); > > } > > > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > > index ad70c9a..b4609dd 100644 > > --- a/include/hw/virtio/virtio-blk.h > > +++ b/include/hw/virtio/virtio-blk.h > > @@ -132,6 +132,7 @@ typedef struct VirtIOBlock { > > VirtIODevice parent_obj; > > BlockDriverState *bs; > > VirtQueue *vq; > > + VirtQueue **vqs; > > void *rq; > > QEMUBH *bh; > > BlockConf *conf; > > > > Dataplane must not be a change to the guest ABI. If you implement this > feature you have to implement it for both dataplane and non-dataplne. > > Paolo Actually, I think different backends at runtime should be allowed to change guest visible ABI. For example if you give qemu a read only backend this is guest visible right? However relying on ifdefs is wrong. It should be a runtime thing.
Il 30/07/2014 17:12, Michael S. Tsirkin ha scritto: >> > >> > Dataplane must not be a change to the guest ABI. If you implement this >> > feature you have to implement it for both dataplane and non-dataplne. >> > >> > Paolo > Actually, I think different backends at runtime should be allowed to > change guest visible ABI. For example if you give qemu a read only > backend this is guest visible right? Dataplane is not meant to be a different backend, it's just moving stuff out to a separate thread. It's only due to QEMU limitation that it doesn't share the vring code (and these patches include a lot of steps backwards where dataplane becomes != non-dataplane). Paolo
On Wed, Jul 30, 2014 at 11:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 30/07/2014 17:12, Michael S. Tsirkin ha scritto: >>> > >>> > Dataplane must not be a change to the guest ABI. If you implement this >>> > feature you have to implement it for both dataplane and non-dataplne. >>> > IMO, no matter if the feature is set or not, both old and new VM can support it well. Per virtio spec, only the feature is set, the VM can be allowed to access the 'num_queues' field, and I didn't see any problem from VM's view point. So could you explain why both dataplane and non-dataplane have to support the feature. I am doing so because there isn't performance advantage to take mq for non-dataplane. >>> > Paolo >> Actually, I think different backends at runtime should be allowed to >> change guest visible ABI. For example if you give qemu a read only >> backend this is guest visible right? > > Dataplane is not meant to be a different backend, it's just moving stuff > out to a separate thread. It's only due to QEMU limitation that it > doesn't share the vring code (and these patches include a lot of steps > backwards where dataplane becomes != non-dataplane). There won't lots of backwards steps, just the bypass co patch, which is just bypassing co in case of being unnecessary for raw image case, but it is still one code path. As I mentioned, all these steps are just for the performance regression from the commit 580b6b2aa2(dataplane: use the QEMU block layer for I/O). Thanks,
Il 31/07/2014 05:47, Ming Lei ha scritto: > On Wed, Jul 30, 2014 at 11:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 30/07/2014 17:12, Michael S. Tsirkin ha scritto: >>>>> >>>>> Dataplane must not be a change to the guest ABI. If you implement this >>>>> feature you have to implement it for both dataplane and non-dataplne. >>>>> > > IMO, no matter if the feature is set or not, both old and new VM > can support it well. > > Per virtio spec, only the feature is set, the VM can be allowed to > access the 'num_queues' field, and I didn't see any problem from > VM's view point. > > So could you explain why both dataplane and non-dataplane have > to support the feature. Because otherwise you change the guest ABI. > I am doing so because there isn't performance advantage to take mq > for non-dataplane. It doesn't matter, changing features between dataplane and non-dataplane is not something that you can do. >>> Actually, I think different backends at runtime should be allowed to >>> change guest visible ABI. For example if you give qemu a read only >>> backend this is guest visible right? >> >> Dataplane is not meant to be a different backend, it's just moving stuff >> out to a separate thread. It's only due to QEMU limitation that it >> doesn't share the vring code (and these patches include a lot of steps >> backwards where dataplane becomes != non-dataplane). > > There won't lots of backwards steps, just the bypass co patch, which > is just bypassing co in case of being unnecessary for raw image case, > but it is still one code path. Using the object pool for dataplane only is a backwards step, implementing multique for dataplane only is a backwards step, bypassing coroutines for dataplane only is a backwards step. Paolo
On Thu, Jul 31, 2014 at 4:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 31/07/2014 05:47, Ming Lei ha scritto: >> On Wed, Jul 30, 2014 at 11:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 30/07/2014 17:12, Michael S. Tsirkin ha scritto: >>>>>> >>>>>> Dataplane must not be a change to the guest ABI. If you implement this >>>>>> feature you have to implement it for both dataplane and non-dataplne. >>>>>> >> >> IMO, no matter if the feature is set or not, both old and new VM >> can support it well. >> >> Per virtio spec, only the feature is set, the VM can be allowed to >> access the 'num_queues' field, and I didn't see any problem from >> VM's view point. >> >> So could you explain why both dataplane and non-dataplane have >> to support the feature. > > Because otherwise you change the guest ABI. Sorry, I don't understand why the guest ABI is changed since VM only parses 'num_queues' iff the feature is set, and the DATAPLANE macro is determined during compiling. > >> I am doing so because there isn't performance advantage to take mq >> for non-dataplane. > > It doesn't matter, changing features between dataplane and non-dataplane > is not something that you can do. Actually it is easy to support multi virtqueue for non-dataplane, and it should be enough to remove two 'ifdef'. > >>>> Actually, I think different backends at runtime should be allowed to >>>> change guest visible ABI. For example if you give qemu a read only >>>> backend this is guest visible right? >>> >>> Dataplane is not meant to be a different backend, it's just moving stuff >>> out to a separate thread. It's only due to QEMU limitation that it >>> doesn't share the vring code (and these patches include a lot of steps >>> backwards where dataplane becomes != non-dataplane). >> >> There won't lots of backwards steps, just the bypass co patch, which >> is just bypassing co in case of being unnecessary for raw image case, >> but it is still one code path. > > Using the object pool for dataplane only is a backwards step, > implementing multique for dataplane only is a backwards step, This one isn't since no multique has been tried before for virtio-blk, :-) > bypassing > coroutines for dataplane only is a backwards step. OK, then all these backwards are positive steps too since they do improve performance, :-) The only side-effect I thought of is that bypassing coroutine may cause block layer a bit complicated. Thanks,
On Fri, Aug 1, 2014 at 11:09 AM, Ming Lei <ming.lei@canonical.com> wrote: > On Thu, Jul 31, 2014 at 4:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> It doesn't matter, changing features between dataplane and non-dataplane >> is not something that you can do. > > Actually it is easy to support multi virtqueue for non-dataplane, > and it should be enough to remove two 'ifdef'. Sorry, virtqueue_pop()/virtqueue_push() need changes too, but it won't be difficult too since 'qid' has been introduced inside VirtIOBlockReq already. Thanks,
Il 01/08/2014 05:09, Ming Lei ha scritto: >>> >> Per virtio spec, only the feature is set, the VM can be allowed to >>> >> access the 'num_queues' field, and I didn't see any problem from >>> >> VM's view point. >>> >> >>> >> So could you explain why both dataplane and non-dataplane have >>> >> to support the feature. >> > >> > Because otherwise you change the guest ABI. > Sorry, I don't understand why the guest ABI is changed since > VM only parses 'num_queues' iff the feature is set, and the DATAPLANE > macro is determined during compiling. Even recompiling the same version of QEMU should not change the guest ABI. Recompilation may affect which devices are present, but then the migration destination will not even start if something is broken. And as you pointed out, migration from dataplane to non-dataplane will break because you didn't convert callers of virtqueue_pop/push. Paolo
On Fri, Aug 1, 2014 at 2:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 01/08/2014 05:09, Ming Lei ha scritto: >>>> >> Per virtio spec, only the feature is set, the VM can be allowed to >>>> >> access the 'num_queues' field, and I didn't see any problem from >>>> >> VM's view point. >>>> >> >>>> >> So could you explain why both dataplane and non-dataplane have >>>> >> to support the feature. >>> > >>> > Because otherwise you change the guest ABI. >> Sorry, I don't understand why the guest ABI is changed since >> VM only parses 'num_queues' iff the feature is set, and the DATAPLANE >> macro is determined during compiling. > > Even recompiling the same version of QEMU should not change the guest > ABI. Recompilation may affect which devices are present, but then the > migration destination will not even start if something is broken. > > And as you pointed out, migration from dataplane to non-dataplane will > break because you didn't convert callers of virtqueue_pop/push. OK, I will convert non-dataplane to support multi virtqueues in V1, and the conversion is not difficult and straightforward. BTW, docs/migration.txt mentions that "QEMU has to be launched with the same arguments the two times", so can I understand that migration from dataplane to non-dataplane shouldn't have been allowed? Thanks,
Il 01/08/2014 09:35, Ming Lei ha scritto: > OK, I will convert non-dataplane to support multi virtqueues in V1, > and the conversion is not difficult and straightforward. > > BTW, docs/migration.txt mentions that "QEMU has to be launched > with the same arguments the two times", so can I understand that > migration from dataplane to non-dataplane shouldn't have been > allowed? You're right, that text gives a rule of thumb but it is not absolutely true. It is for example obviously okay to have different paths for drives in the two invocations. It is also okay to use different formats (raw vs. qcow2) if, for example, you are migrating disk contents (either with "migrate -b" or with the embedded NBD server). Dataplane is another such case where an option does not affect how the device behaves. It is a bit more clearer if you think of it as just "use an iothread different from the default one", which is what we're aiming at. Everything else is working around limitations in QEMU, and will disappear once these limitations are lifted. Paolo
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index ab99156..44e9956 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -556,6 +556,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) blkcfg.physical_block_exp = get_physical_block_exp(s->conf); blkcfg.alignment_offset = 0; blkcfg.wce = bdrv_enable_write_cache(s->bs); + stw_p(&blkcfg.num_queues, s->blk.num_queues); memcpy(config, &blkcfg, sizeof(struct virtio_blk_config)); } @@ -590,6 +591,12 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features) if (bdrv_is_read_only(s->bs)) features |= 1 << VIRTIO_BLK_F_RO; +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE + if (s->blk.num_queues > 1) { + features |= 1 << VIRTIO_BLK_F_MQ; + } +#endif + return features; } @@ -739,8 +746,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE Error *err = NULL; #endif + int i; static int virtio_blk_id; +#ifndef CONFIG_VIRTIO_BLK_DATA_PLANE + blk->num_queues = 1; +#endif + if (!blk->conf.bs) { error_setg(errp, "drive property not set"); return; @@ -765,7 +777,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) s->rq = NULL; s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; - s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output); + s->vqs = g_malloc0(sizeof(VirtQueue *) * blk->num_queues); + for (i = 0; i < blk->num_queues; i++) + s->vqs[i] = virtio_add_queue(vdev, 128, virtio_blk_handle_output); + s->vq = s->vqs[0]; s->complete_request = virtio_blk_complete_request; #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE virtio_blk_data_plane_create(vdev, blk, &s->dataplane, &err); @@ -802,6 +817,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) qemu_del_vm_change_state_handler(s->change); unregister_savevm(dev, "virtio-blk", s); blockdev_mark_auto_del(s->bs); + g_free(s->vqs); virtio_cleanup(vdev); } diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index ad70c9a..b4609dd 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -132,6 +132,7 @@ typedef struct VirtIOBlock { VirtIODevice parent_obj; BlockDriverState *bs; VirtQueue *vq; + VirtQueue **vqs; void *rq; QEMUBH *bh; BlockConf *conf;
Now we only support multi vqs for dataplane case. Signed-off-by: Ming Lei <ming.lei@canonical.com> --- hw/block/virtio-blk.c | 18 +++++++++++++++++- include/hw/virtio/virtio-blk.h | 1 + 2 files changed, 18 insertions(+), 1 deletion(-)