Message ID | 20200129140702.5411-2-dplotnikov@virtuozzo.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Increase default virtqueue size to improve performance | expand |
On 1/29/20 3:06 PM, Denis Plotnikov wrote: > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> typo VIRTQUEUE_DEFUALT_SIZE -> VIRTQUEUE_DEFAULT_SIZE in subject With subject fixed: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/block/virtio-blk.c | 6 ++++-- > hw/scsi/virtio-scsi.c | 5 +++-- > include/hw/virtio/virtio.h | 1 + > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 09f46ed85f..72f935033f 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > memset(&blkcfg, 0, sizeof(blkcfg)); > virtio_stq_p(vdev, &blkcfg.capacity, capacity); > virtio_stl_p(vdev, &blkcfg.seg_max, > - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); > + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : > + VIRTQUEUE_DEFAULT_SIZE - 2); > virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); > virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); > virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); > @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, > true), > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, > + VIRTQUEUE_DEFAULT_SIZE), > DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > IOThread *), > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 3b61563609..36f66046ae 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, > > virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); > virtio_stl_p(vdev, &scsiconf->seg_max, > - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); > + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : > + VIRTQUEUE_DEFAULT_SIZE - 2); > virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); > virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); > virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); > @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) > static Property virtio_scsi_properties[] = { > DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), > DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, > - parent_obj.conf.virtqueue_size, 128), > + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), > DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, > parent_obj.conf.seg_max_adjust, true), > DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index b69d517496..a66ea2368b 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, > typedef struct VirtQueue VirtQueue; > > #define VIRTQUEUE_MAX_SIZE 1024 > +#define VIRTQUEUE_DEFAULT_SIZE 128 > > typedef struct VirtQueueElement > { >
On Wed, 29 Jan 2020 17:06:59 +0300 Denis Plotnikov <dplotnikov@virtuozzo.com> wrote: > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > --- > hw/block/virtio-blk.c | 6 ++++-- > hw/scsi/virtio-scsi.c | 5 +++-- > include/hw/virtio/virtio.h | 1 + > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 09f46ed85f..72f935033f 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > memset(&blkcfg, 0, sizeof(blkcfg)); > virtio_stq_p(vdev, &blkcfg.capacity, capacity); > virtio_stl_p(vdev, &blkcfg.seg_max, > - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); > + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : > + VIRTQUEUE_DEFAULT_SIZE - 2); > virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); > virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); > virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); > @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, > true), > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, > + VIRTQUEUE_DEFAULT_SIZE), > DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > IOThread *), > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 3b61563609..36f66046ae 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, > > virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); > virtio_stl_p(vdev, &scsiconf->seg_max, > - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); > + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : > + VIRTQUEUE_DEFAULT_SIZE - 2); > virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); > virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); > virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); > @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) > static Property virtio_scsi_properties[] = { > DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), > DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, > - parent_obj.conf.virtqueue_size, 128), > + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), > DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, > parent_obj.conf.seg_max_adjust, true), > DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index b69d517496..a66ea2368b 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, > typedef struct VirtQueue VirtQueue; > > #define VIRTQUEUE_MAX_SIZE 1024 > +#define VIRTQUEUE_DEFAULT_SIZE 128 Going from the header only, this looks like a value that is supposed to be used for every virtqueue... but from the users, this is only for blk and scsi. I don't think adding a default for everything makes sense, even if the same value makes sense for blk and scsi. > > typedef struct VirtQueueElement > {
On Wed, Jan 29, 2020 at 05:06:59PM +0300, Denis Plotnikov wrote: > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> I'm not sure what the point is. It's more or less an accident that these two devices share the queue size, this constance makes no sense to me. > --- > hw/block/virtio-blk.c | 6 ++++-- > hw/scsi/virtio-scsi.c | 5 +++-- > include/hw/virtio/virtio.h | 1 + > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 09f46ed85f..72f935033f 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > memset(&blkcfg, 0, sizeof(blkcfg)); > virtio_stq_p(vdev, &blkcfg.capacity, capacity); > virtio_stl_p(vdev, &blkcfg.seg_max, > - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); > + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : > + VIRTQUEUE_DEFAULT_SIZE - 2); > virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); > virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); > virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); > @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, > true), > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, > + VIRTQUEUE_DEFAULT_SIZE), > DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > IOThread *), > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 3b61563609..36f66046ae 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, > > virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); > virtio_stl_p(vdev, &scsiconf->seg_max, > - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); > + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : > + VIRTQUEUE_DEFAULT_SIZE - 2); > virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); > virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); > virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); > @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) > static Property virtio_scsi_properties[] = { > DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), > DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, > - parent_obj.conf.virtqueue_size, 128), > + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), > DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, > parent_obj.conf.seg_max_adjust, true), > DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index b69d517496..a66ea2368b 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, > typedef struct VirtQueue VirtQueue; > > #define VIRTQUEUE_MAX_SIZE 1024 > +#define VIRTQUEUE_DEFAULT_SIZE 128 > > typedef struct VirtQueueElement > { > -- > 2.17.0
On Wed, Jan 29, 2020 at 06:55:18PM +0100, Cornelia Huck wrote: > On Wed, 29 Jan 2020 17:06:59 +0300 > Denis Plotnikov <dplotnikov@virtuozzo.com> wrote: > > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > > --- > > hw/block/virtio-blk.c | 6 ++++-- > > hw/scsi/virtio-scsi.c | 5 +++-- > > include/hw/virtio/virtio.h | 1 + > > 3 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 09f46ed85f..72f935033f 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > > memset(&blkcfg, 0, sizeof(blkcfg)); > > virtio_stq_p(vdev, &blkcfg.capacity, capacity); > > virtio_stl_p(vdev, &blkcfg.seg_max, > > - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); > > + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : > > + VIRTQUEUE_DEFAULT_SIZE - 2); > > virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); > > virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); > > virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); > > @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { > > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, > > true), > > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > > - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, > > + VIRTQUEUE_DEFAULT_SIZE), > > DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > IOThread *), > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > > index 3b61563609..36f66046ae 100644 > > --- a/hw/scsi/virtio-scsi.c > > +++ b/hw/scsi/virtio-scsi.c > > @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, > > > > virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); > > virtio_stl_p(vdev, &scsiconf->seg_max, > > - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); > > + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : > > + VIRTQUEUE_DEFAULT_SIZE - 2); > > virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); > > virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); > > virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); > > @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) > > static Property virtio_scsi_properties[] = { > > DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), > > DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, > > - parent_obj.conf.virtqueue_size, 128), > > + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), > > DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, > > parent_obj.conf.seg_max_adjust, true), > > DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > index b69d517496..a66ea2368b 100644 > > --- a/include/hw/virtio/virtio.h > > +++ b/include/hw/virtio/virtio.h > > @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, > > typedef struct VirtQueue VirtQueue; > > > > #define VIRTQUEUE_MAX_SIZE 1024 > > +#define VIRTQUEUE_DEFAULT_SIZE 128 > > Going from the header only, this looks like a value that is supposed to > be used for every virtqueue... but from the users, this is only for blk > and scsi. > > I don't think adding a default for everything makes sense, even if the > same value makes sense for blk and scsi. Agreed, this value is too general. VIRTIO_BLK_VQ_DEFAULT_SIZE and VIRTIO_SCSI_VQ_DEFAULT_SIZE would make sense to me. Stefan
On 30.01.2020 17:56, Stefan Hajnoczi wrote: > On Wed, Jan 29, 2020 at 06:55:18PM +0100, Cornelia Huck wrote: >> On Wed, 29 Jan 2020 17:06:59 +0300 >> Denis Plotnikov <dplotnikov@virtuozzo.com> wrote: >> >>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >>> --- >>> hw/block/virtio-blk.c | 6 ++++-- >>> hw/scsi/virtio-scsi.c | 5 +++-- >>> include/hw/virtio/virtio.h | 1 + >>> 3 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >>> index 09f46ed85f..72f935033f 100644 >>> --- a/hw/block/virtio-blk.c >>> +++ b/hw/block/virtio-blk.c >>> @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) >>> memset(&blkcfg, 0, sizeof(blkcfg)); >>> virtio_stq_p(vdev, &blkcfg.capacity, capacity); >>> virtio_stl_p(vdev, &blkcfg.seg_max, >>> - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); >>> + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : >>> + VIRTQUEUE_DEFAULT_SIZE - 2); >>> virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); >>> virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); >>> virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); >>> @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { >>> DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, >>> true), >>> DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), >>> - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), >>> + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, >>> + VIRTQUEUE_DEFAULT_SIZE), >>> DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), >>> DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, >>> IOThread *), >>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >>> index 3b61563609..36f66046ae 100644 >>> --- a/hw/scsi/virtio-scsi.c >>> +++ b/hw/scsi/virtio-scsi.c >>> @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, >>> >>> virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); >>> virtio_stl_p(vdev, &scsiconf->seg_max, >>> - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); >>> + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : >>> + VIRTQUEUE_DEFAULT_SIZE - 2); >>> virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); >>> virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); >>> virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); >>> @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) >>> static Property virtio_scsi_properties[] = { >>> DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), >>> DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, >>> - parent_obj.conf.virtqueue_size, 128), >>> + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), >>> DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, >>> parent_obj.conf.seg_max_adjust, true), >>> DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>> index b69d517496..a66ea2368b 100644 >>> --- a/include/hw/virtio/virtio.h >>> +++ b/include/hw/virtio/virtio.h >>> @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, >>> typedef struct VirtQueue VirtQueue; >>> >>> #define VIRTQUEUE_MAX_SIZE 1024 >>> +#define VIRTQUEUE_DEFAULT_SIZE 128 >> Going from the header only, this looks like a value that is supposed to >> be used for every virtqueue... but from the users, this is only for blk >> and scsi. >> >> I don't think adding a default for everything makes sense, even if the >> same value makes sense for blk and scsi. > Agreed, this value is too general. VIRTIO_BLK_VQ_DEFAULT_SIZE and > VIRTIO_SCSI_VQ_DEFAULT_SIZE would make sense to me. > > Stefan agree, that would be better. Will redo and resend the series. Denis
On 30.01.2020 16:38, Michael S. Tsirkin wrote: > On Wed, Jan 29, 2020 at 05:06:59PM +0300, Denis Plotnikov wrote: >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > > I'm not sure what the point is. It's more or less an accident that > these two devices share the queue size, this constance > makes no sense to me. Ok, then let's just make a separate queue length constant for each type. (Will redo and send in the next series) Thanks! Denis > >> --- >> hw/block/virtio-blk.c | 6 ++++-- >> hw/scsi/virtio-scsi.c | 5 +++-- >> include/hw/virtio/virtio.h | 1 + >> 3 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 09f46ed85f..72f935033f 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) >> memset(&blkcfg, 0, sizeof(blkcfg)); >> virtio_stq_p(vdev, &blkcfg.capacity, capacity); >> virtio_stl_p(vdev, &blkcfg.seg_max, >> - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); >> + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : >> + VIRTQUEUE_DEFAULT_SIZE - 2); >> virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); >> virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); >> virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); >> @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { >> DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, >> true), >> DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), >> - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), >> + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, >> + VIRTQUEUE_DEFAULT_SIZE), >> DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), >> DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, >> IOThread *), >> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >> index 3b61563609..36f66046ae 100644 >> --- a/hw/scsi/virtio-scsi.c >> +++ b/hw/scsi/virtio-scsi.c >> @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, >> >> virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); >> virtio_stl_p(vdev, &scsiconf->seg_max, >> - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); >> + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : >> + VIRTQUEUE_DEFAULT_SIZE - 2); >> virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); >> virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); >> virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); >> @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) >> static Property virtio_scsi_properties[] = { >> DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), >> DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, >> - parent_obj.conf.virtqueue_size, 128), >> + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), >> DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, >> parent_obj.conf.seg_max_adjust, true), >> DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index b69d517496..a66ea2368b 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, >> typedef struct VirtQueue VirtQueue; >> >> #define VIRTQUEUE_MAX_SIZE 1024 >> +#define VIRTQUEUE_DEFAULT_SIZE 128 >> >> typedef struct VirtQueueElement >> { >> -- >> 2.17.0
On Mon, Feb 03, 2020 at 03:17:07PM +0300, Denis Plotnikov wrote: > > > On 30.01.2020 16:38, Michael S. Tsirkin wrote: > > On Wed, Jan 29, 2020 at 05:06:59PM +0300, Denis Plotnikov wrote: > > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > > > > I'm not sure what the point is. It's more or less an accident that > > these two devices share the queue size, this constance > > makes no sense to me. > Ok, then let's just make a separate queue length constant for each type. it's just a number, I don't think we need a constant here. If you feel it needs documentation, add a comment! > (Will redo and send in the next series) > Thanks! > > Denis > > > > > --- > > > hw/block/virtio-blk.c | 6 ++++-- > > > hw/scsi/virtio-scsi.c | 5 +++-- > > > include/hw/virtio/virtio.h | 1 + > > > 3 files changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > index 09f46ed85f..72f935033f 100644 > > > --- a/hw/block/virtio-blk.c > > > +++ b/hw/block/virtio-blk.c > > > @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > > > memset(&blkcfg, 0, sizeof(blkcfg)); > > > virtio_stq_p(vdev, &blkcfg.capacity, capacity); > > > virtio_stl_p(vdev, &blkcfg.seg_max, > > > - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); > > > + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : > > > + VIRTQUEUE_DEFAULT_SIZE - 2); > > > virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); > > > virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); > > > virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); > > > @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { > > > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, > > > true), > > > DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), > > > - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), > > > + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, > > > + VIRTQUEUE_DEFAULT_SIZE), > > > DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), > > > DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, > > > IOThread *), > > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > > > index 3b61563609..36f66046ae 100644 > > > --- a/hw/scsi/virtio-scsi.c > > > +++ b/hw/scsi/virtio-scsi.c > > > @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, > > > virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); > > > virtio_stl_p(vdev, &scsiconf->seg_max, > > > - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); > > > + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : > > > + VIRTQUEUE_DEFAULT_SIZE - 2); > > > virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); > > > virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); > > > virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); > > > @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) > > > static Property virtio_scsi_properties[] = { > > > DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), > > > DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, > > > - parent_obj.conf.virtqueue_size, 128), > > > + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), > > > DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, > > > parent_obj.conf.seg_max_adjust, true), > > > DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > > index b69d517496..a66ea2368b 100644 > > > --- a/include/hw/virtio/virtio.h > > > +++ b/include/hw/virtio/virtio.h > > > @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, > > > typedef struct VirtQueue VirtQueue; > > > #define VIRTQUEUE_MAX_SIZE 1024 > > > +#define VIRTQUEUE_DEFAULT_SIZE 128 > > > typedef struct VirtQueueElement > > > { > > > -- > > > 2.17.0
On 03.02.2020 15:51, Michael S. Tsirkin wrote: > On Mon, Feb 03, 2020 at 03:17:07PM +0300, Denis Plotnikov wrote: >> >> On 30.01.2020 16:38, Michael S. Tsirkin wrote: >>> On Wed, Jan 29, 2020 at 05:06:59PM +0300, Denis Plotnikov wrote: >>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >>> I'm not sure what the point is. It's more or less an accident that >>> these two devices share the queue size, this constance >>> makes no sense to me. >> Ok, then let's just make a separate queue length constant for each type. > it's just a number, I don't think we need a constant here. > If you feel it needs documentation, add a comment! I just thought that the meaningful name for the number would be better for the code understanding. Anyway, If doesn't improve anything I'll just change the number and add a comment what it means. Denis > >> (Will redo and send in the next series) >> Thanks! >> >> Denis >>>> --- >>>> hw/block/virtio-blk.c | 6 ++++-- >>>> hw/scsi/virtio-scsi.c | 5 +++-- >>>> include/hw/virtio/virtio.h | 1 + >>>> 3 files changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >>>> index 09f46ed85f..72f935033f 100644 >>>> --- a/hw/block/virtio-blk.c >>>> +++ b/hw/block/virtio-blk.c >>>> @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) >>>> memset(&blkcfg, 0, sizeof(blkcfg)); >>>> virtio_stq_p(vdev, &blkcfg.capacity, capacity); >>>> virtio_stl_p(vdev, &blkcfg.seg_max, >>>> - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); >>>> + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : >>>> + VIRTQUEUE_DEFAULT_SIZE - 2); >>>> virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); >>>> virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); >>>> virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); >>>> @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { >>>> DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, >>>> true), >>>> DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), >>>> - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), >>>> + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, >>>> + VIRTQUEUE_DEFAULT_SIZE), >>>> DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), >>>> DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, >>>> IOThread *), >>>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >>>> index 3b61563609..36f66046ae 100644 >>>> --- a/hw/scsi/virtio-scsi.c >>>> +++ b/hw/scsi/virtio-scsi.c >>>> @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, >>>> virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); >>>> virtio_stl_p(vdev, &scsiconf->seg_max, >>>> - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); >>>> + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : >>>> + VIRTQUEUE_DEFAULT_SIZE - 2); >>>> virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); >>>> virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); >>>> virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); >>>> @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) >>>> static Property virtio_scsi_properties[] = { >>>> DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), >>>> DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, >>>> - parent_obj.conf.virtqueue_size, 128), >>>> + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), >>>> DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, >>>> parent_obj.conf.seg_max_adjust, true), >>>> DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, >>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>>> index b69d517496..a66ea2368b 100644 >>>> --- a/include/hw/virtio/virtio.h >>>> +++ b/include/hw/virtio/virtio.h >>>> @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, >>>> typedef struct VirtQueue VirtQueue; >>>> #define VIRTQUEUE_MAX_SIZE 1024 >>>> +#define VIRTQUEUE_DEFAULT_SIZE 128 >>>> typedef struct VirtQueueElement >>>> { >>>> -- >>>> 2.17.0
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 09f46ed85f..72f935033f 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -914,7 +914,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) memset(&blkcfg, 0, sizeof(blkcfg)); virtio_stq_p(vdev, &blkcfg.capacity, capacity); virtio_stl_p(vdev, &blkcfg.seg_max, - s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 2); + s->conf.seg_max_adjust ? s->conf.queue_size - 2 : + VIRTQUEUE_DEFAULT_SIZE - 2); virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls); virtio_stl_p(vdev, &blkcfg.blk_size, blk_size); virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size); @@ -1272,7 +1273,8 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), - DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, + VIRTQUEUE_DEFAULT_SIZE), DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true), DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, IOThread *), diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 3b61563609..36f66046ae 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -660,7 +660,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev, virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues); virtio_stl_p(vdev, &scsiconf->seg_max, - s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 2); + s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : + VIRTQUEUE_DEFAULT_SIZE - 2); virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors); virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun); virtio_stl_p(vdev, &scsiconf->event_info_size, sizeof(VirtIOSCSIEvent)); @@ -965,7 +966,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp) static Property virtio_scsi_properties[] = { DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, - parent_obj.conf.virtqueue_size, 128), + parent_obj.conf.virtqueue_size, VIRTQUEUE_DEFAULT_SIZE), DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, parent_obj.conf.seg_max_adjust, true), DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors, diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index b69d517496..a66ea2368b 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -48,6 +48,7 @@ size_t virtio_feature_get_config_size(VirtIOFeature *features, typedef struct VirtQueue VirtQueue; #define VIRTQUEUE_MAX_SIZE 1024 +#define VIRTQUEUE_DEFAULT_SIZE 128 typedef struct VirtQueueElement {
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- hw/block/virtio-blk.c | 6 ++++-- hw/scsi/virtio-scsi.c | 5 +++-- include/hw/virtio/virtio.h | 1 + 3 files changed, 8 insertions(+), 4 deletions(-)