* [PATCH v3 1/2] virtio: make seg_max virtqueue size dependent
2019-11-12 11:13 [PATCH v3 0/2] virtio: make seg_max virtqueue size dependent Denis Plotnikov
@ 2019-11-12 11:13 ` Denis Plotnikov
2019-11-12 11:13 ` [PATCH v3 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test Denis Plotnikov
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Denis Plotnikov @ 2019-11-12 11:13 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, fam, ehabkost, mst, mreitz, stefanha, pbonzini, den
Before the patch, seg_max parameter was immutable and hardcoded
to 126 (128 - 2) without respect to queue size. This has two negative effects:
1. when queue size is < 128, we have Virtio 1.1 specfication violation:
(2.6.5.3.1 Driver Requirements) seq_max must be <= queue_size.
This violation affects the old Linux guests (ver < 4.14). These guests
crash on these queue_size setups.
2. when queue_size > 128, as was pointed out by Denis Lunev <den@virtuozzo.com>,
seg_max restrics guest's block request length which affects guests'
performance making them issues more block request than needed.
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg03721.html
To mitigate this two effects, the patch adds the property adjusting seg_max
to queue size automaticaly. Since seg_max is a guest visible parameter,
the property is machine type managable and allows to choose between
old (seg_max = 126 always) and new (seg_max = queue_size - 2) behaviors.
Not to change the behavior of the older VMs, prevent setting the default
seg_max_adjust value for older machine types.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
hw/block/virtio-blk.c | 9 ++++++++-
hw/core/machine.c | 3 +++
hw/scsi/vhost-scsi.c | 2 ++
hw/scsi/virtio-scsi.c | 10 +++++++++-
include/hw/virtio/virtio-blk.h | 1 +
include/hw/virtio/virtio-scsi.h | 1 +
6 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4c357d2928..f118b29c7c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -908,7 +908,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
blk_get_geometry(s->blk, &capacity);
memset(&blkcfg, 0, sizeof(blkcfg));
virtio_stq_p(vdev, &blkcfg.capacity, capacity);
- virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
+ virtio_stl_p(vdev, &blkcfg.seg_max,
+ s->conf.seg_max_adjust ? s->conf.queue_size - 2 : 128 - 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);
@@ -1131,6 +1132,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
error_setg(errp, "num-queues property must be larger than 0");
return;
}
+ if (conf->queue_size <= 2) {
+ error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
+ "must be > 2", conf->queue_size);
+ return;
+ }
if (!is_power_of_2(conf->queue_size) ||
conf->queue_size > VIRTQUEUE_MAX_SIZE) {
error_setg(errp, "invalid queue-size property (%" PRIu16 "), "
@@ -1260,6 +1266,7 @@ static Property virtio_blk_properties[] = {
true),
DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1),
DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
+ DEFINE_PROP_BOOL("seg-max-adjust", VirtIOBlock, conf.seg_max_adjust, true),
DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
IOThread *),
DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3bf8..3e7df94620 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -29,6 +29,9 @@
GlobalProperty hw_compat_4_1[] = {
{ "virtio-pci", "x-pcie-flr-init", "off" },
+ { "virtio-blk-device", "seg-max-adjust", "off"},
+ { "virtio-scsi-device", "seg_max_adjust", "off"},
+ { "vhost-blk-device", "seg_max_adjust", "off"},
};
const size_t hw_compat_4_1_len = G_N_ELEMENTS(hw_compat_4_1);
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c693fc748a..26f710d3ec 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -275,6 +275,8 @@ static Property vhost_scsi_properties[] = {
DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1),
DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size,
128),
+ DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust,
+ true),
DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
0xFFFF),
DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e8b2b64d09..405cb6c953 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -654,7 +654,8 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
- virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
+ virtio_stl_p(vdev, &scsiconf->seg_max,
+ s->conf.seg_max_adjust ? s->conf.virtqueue_size - 2 : 128 - 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));
@@ -893,6 +894,11 @@ void virtio_scsi_common_realize(DeviceState *dev,
virtio_cleanup(vdev);
return;
}
+ if (s->conf.virtqueue_size <= 2) {
+ error_setg(errp, "invalid virtqueue_size property (= %" PRIu16 "), "
+ "must be > 2", s->conf.virtqueue_size);
+ return;
+ }
s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues);
s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
@@ -949,6 +955,8 @@ 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),
+ 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,
0xFFFF),
DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI, parent_obj.conf.cmd_per_lun,
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index cddcfbebe9..2934426050 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -38,6 +38,7 @@ struct VirtIOBlkConf
uint32_t request_merging;
uint16_t num_queues;
uint16_t queue_size;
+ bool seg_max_adjust;
uint32_t max_discard_sectors;
uint32_t max_write_zeroes_sectors;
};
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 122f7c4b6f..24e768909d 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -48,6 +48,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig;
struct VirtIOSCSIConf {
uint32_t num_queues;
uint32_t virtqueue_size;
+ bool seg_max_adjust;
uint32_t max_sectors;
uint32_t cmd_per_lun;
#ifdef CONFIG_VHOST_SCSI
--
2.17.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v3 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test
2019-11-12 11:13 [PATCH v3 0/2] virtio: make seg_max virtqueue size dependent Denis Plotnikov
2019-11-12 11:13 ` [PATCH v3 1/2] " Denis Plotnikov
@ 2019-11-12 11:13 ` Denis Plotnikov
2019-11-12 11:39 ` [PATCH v3 0/2] virtio: make seg_max virtqueue size dependent Michael S. Tsirkin
2019-11-21 13:33 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Denis Plotnikov @ 2019-11-12 11:13 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, fam, ehabkost, mst, mreitz, stefanha, pbonzini, den
It tests proper seg_max_adjust settings for all machine types except
'none', 'isapc', 'microvm'
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
tests/acceptance/virtio_seg_max_adjust.py | 135 ++++++++++++++++++++++
1 file changed, 135 insertions(+)
create mode 100755 tests/acceptance/virtio_seg_max_adjust.py
diff --git a/tests/acceptance/virtio_seg_max_adjust.py b/tests/acceptance/virtio_seg_max_adjust.py
new file mode 100755
index 0000000000..17e70efc1a
--- /dev/null
+++ b/tests/acceptance/virtio_seg_max_adjust.py
@@ -0,0 +1,135 @@
+#!/usr/bin/env python
+#
+# Test virtio-scsi and virtio-blk queue settings for all machine types
+#
+# Copyright (c) 2019 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import sys
+import os
+import re
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.machine import QEMUMachine
+from avocado_qemu import Test
+
+#list of machine types and virtqueue properties to test
+VIRTIO_SCSI_PROPS = {'seg_max_adjust': 'seg_max_adjust'}
+VIRTIO_BLK_PROPS = {'seg_max_adjust': 'seg-max-adjust'}
+
+DEV_TYPES = {'virtio-scsi-pci': VIRTIO_SCSI_PROPS,
+ 'virtio-blk-pci': VIRTIO_BLK_PROPS}
+
+VM_DEV_PARAMS = {'virtio-scsi-pci': ['-device', 'virtio-scsi-pci,id=scsi0'],
+ 'virtio-blk-pci': ['-device',
+ 'virtio-blk-pci,id=scsi0,drive=drive0',
+ '-drive',
+ 'driver=null-co,id=drive0,if=none']}
+
+
+class VirtioMaxSegSettingsCheck(Test):
+ @staticmethod
+ def make_pattern(props):
+ pattern_items = ['{0} = \w+'.format(prop) for prop in props]
+ return '|'.join(pattern_items)
+
+ def query_virtqueue(self, vm, dev_type_name):
+ query_ok = False
+ error = None
+ props = None
+
+ output = vm.command('human-monitor-command',
+ command_line = 'info qtree')
+ props_list = DEV_TYPES[dev_type_name].values();
+ pattern = self.make_pattern(props_list)
+ res = re.findall(pattern, output)
+
+ if len(res) != len(props_list):
+ props_list = set(props_list)
+ res = set(res)
+ not_found = props_list.difference(res)
+ not_found = ', '.join(not_found)
+ error = '({0}): The following properties not found: {1}'\
+ .format(dev_type_name, not_found)
+ else:
+ query_ok = True
+ props = dict()
+ for prop in res:
+ p = prop.split(' = ')
+ props[p[0]] = p[1]
+ return query_ok, props, error
+
+ def check_mt(self, mt, dev_type_name):
+ with QEMUMachine(self.qemu_bin) as vm:
+ vm.set_machine(mt["name"])
+ for s in VM_DEV_PARAMS[dev_type_name]:
+ vm.add_args(s)
+ vm.launch()
+ query_ok, props, error = self.query_virtqueue(vm, dev_type_name)
+
+ if not query_ok:
+ self.fail('machine type {0}: {1}'.format(mt['name'], error))
+
+ for prop_name, prop_val in props.items():
+ expected_val = mt[prop_name]
+ self.assertEqual(expected_val, prop_val)
+
+ @staticmethod
+ def seg_max_adjust_enabled(mt):
+ # machine types > 4.1 should have seg_max_adjust = true
+ # others seg_max_adjust = false
+ mt = mt.split("-")
+
+ # machine types with one line name and name like pc-x.x
+ if len(mt) <= 2:
+ return False
+
+ # machine types like pc-<chip_name>-x.x[.x]
+ ver = mt[2]
+ ver = ver.split(".");
+
+ # all versions greater than 4.1 goes with seg_max_adjust enabled
+ major = int(ver[0])
+ minor = int(ver[1])
+
+ if major > 4 or (major == 4 and minor > 1):
+ return True
+ return False
+
+ def test_machine_types(self):
+ # collect all machine types except 'none', 'isapc', 'microvm'
+ with QEMUMachine(self.qemu_bin) as vm:
+ vm.launch()
+ machines = [m['name'] for m in vm.command('query-machines')]
+ vm.shutdown()
+ machines.remove('none')
+ machines.remove('isapc')
+ machines.remove('microvm')
+
+ for dev_type in DEV_TYPES:
+ # create the list of machine types and their parameters.
+ mtypes = list()
+ for m in machines:
+ if self.seg_max_adjust_enabled(m):
+ enabled = 'true'
+ else:
+ enabled = 'false'
+ mtypes.append({'name': m,
+ DEV_TYPES[dev_type]['seg_max_adjust']: enabled})
+
+ # test each machine type for a device type
+ for mt in mtypes:
+ self.check_mt(mt, dev_type)
--
2.17.0
^ permalink raw reply related [flat|nested] 5+ messages in thread