[PATCH 2/2] qemu: support dirty ring feature
Hyman
huangy81 at chinatelecom.cn
Mon Jun 21 12:34:05 UTC 2021
在 2021/6/21 15:29, Peter Krempa 写道:
> On Sat, Jun 19, 2021 at 17:53:04 +0800, huangy81 at chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
>>
>> QEMU has introduced a dirty ring feature, this patch add corresponding
>> feature named 'dirty-ring', which enable dirty ring feature when
>> starting vm.
>>
>> To enable the feature, libvirt add "-accel dirty-ring-size=xxx"
>> to QEMU command line, the following XML needs to be added to
>> the guest's domain description:
>>
>> <features>
>> <kvm>
>> <dirty-ring state='on' size='xxx'>
>> </kvm>
>> </features>
>>
>> If property "state=on" but property "size" not be configured, set
>> default ring size with 4096.
>>
>> Since dirty ring can only be enabled by specifying "-accel" option
>> and do not support the legacy style, it seems that there's no
>> other way to work around this, so we use "-accel" option to specify
>> accelerator instead of "-machine" when building qemu commandline.
>>
>> Details about the qemu "-accel" option:
>> https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f73850bd@redhat.com/
>>
>> Three things this patch has done:
>
> Note we require that patches are kept small and self contained. You
> describe 3 things this patch is doing ant thus it almost implies as
> it could be split 3-ways ...
>
>> 1. switch the option "-machine accel" to "-accel" when specifying
>> accelerator type once libvirt build QEMU command line.
>
> this definitely belongs to a separate patch.
> >>
>> 2. Since this patch change the output of building commandline, qemuxml2argvtest
>> should also been modified so that test cases can be executed
>> successfully, we modify test cases involved from qemu-2.9.0 to
>
> Note that the oldest supported qemu is now qemu-2.11
ok, i'll fix it.
>
>> qemu-6.1.0
>>
>> 3. Introduce dirty_ring_size in struct "_virDomainDef" to hold the
>> ring size configured by user, and pass dirty_ring_size when building qemu
>> commandline if dirty ring feature enabled.
>
> This requires at least a 2 way split.
> 1) patch adding the definition, docs and XML parser/formatter
> 2) actual qemu impl.
>
ok, i'll split it next version.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
>> ---
>
> [...]
>
>> @@ -17574,6 +17575,9 @@ virDomainFeaturesDefParse(virDomainDef *def,
>> if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) {
>> int feature;
>> virTristateSwitch value;
>> + xmlNodePtr node = NULL;
>> +
>> + node = ctxt->node;
>> if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) < 0)
>> return -1;
>>
>> @@ -17590,12 +17594,40 @@ virDomainFeaturesDefParse(virDomainDef *def,
>> case VIR_DOMAIN_KVM_HIDDEN:
>> case VIR_DOMAIN_KVM_DEDICATED:
>> case VIR_DOMAIN_KVM_POLLCONTROL:
>> + case VIR_DOMAIN_KVM_DIRTY_RING:
>
> Don't integrate this with other cases ... >
>> if (virXMLPropTristateSwitch(nodes[i], "state",
>> VIR_XML_PROP_REQUIRED,
>> &value) < 0)
>> return -1;
>
>
> Preferably rather duplicate this parsing ....
>
>
>
>>
>> def->kvm_features[feature] = value;
>> +
>> + /* only for dirty ring case */
>> + if (((virDomainKVM) feature) == VIR_DOMAIN_KVM_DIRTY_RING &&
>> + value == VIR_TRISTATE_SWITCH_ON) {
>
> To avoid this condition.
>
>> + ctxt->node = nodes[i];
>> +
>> + if (virXMLPropString(ctxt->node, "size")) {
>> + if (virXPathUInt("string(./@size)", ctxt,
>> + &def->dirty_ring_size) < 0) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("invalid number of dirty ring size"));
>> + return -1;
>> + }
>> +
>> + if ((def->dirty_ring_size & (def->dirty_ring_size - 1)) ||
>
> Zero checks on integers should be explicit.
>
>> + def->dirty_ring_size < 1024 ||
>> + def->dirty_ring_size > 65536) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("dirty ring must be power of 2 "
>> + "and ranges [1024, 65536]"));
>> + return -1;
>> + }
>> + } else {
>> + /* ring size not configured, set with default value 4096 */
>> + def->dirty_ring_size = 4096;
>
> Assigning defaults in the parser isn't current state of things.
> Preferably this belongs to a post-parse callback.
get it.
>
>> + }
>> + }
>> break;
>>
>> case VIR_DOMAIN_KVM_LAST:
>
> [...]
>
>> @@ -21627,6 +21660,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
>> case VIR_DOMAIN_KVM_HIDDEN:
>> case VIR_DOMAIN_KVM_DEDICATED:
>> case VIR_DOMAIN_KVM_POLLCONTROL:
>> + case VIR_DOMAIN_KVM_DIRTY_RING:
>> if (src->kvm_features[i] != dst->kvm_features[i]) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("State of KVM feature '%s' differs: "
>
> The abi-stability check should also check the size.
>
>> @@ -27614,6 +27648,15 @@ virDomainDefFormatFeatures(virBuffer *buf,
>> def->kvm_features[j]));
>> break;
>>
>> + case VIR_DOMAIN_KVM_DIRTY_RING:
>> + if (def->kvm_features[j])
>
> Make the comparison with VIR_TRISTATE_SWITCH_ABSENT explicit here.
>
>> + virBufferAsprintf(&childBuf, "<%s state='%s' size='%d'/>\n",
>> + virDomainKVMTypeToString(j),
>> + virTristateSwitchTypeToString(
>> + def->kvm_features[j]),
>
> Don't linebreak the argument to virTristateSwitchTypeToString even if it
> exceeds the line lenght. It's unreadable this way.
indeed, seems weird, i'll fix it
>
>> + def->dirty_ring_size);
>> + break;
>> +
>> case VIR_DOMAIN_KVM_LAST:
>> break;
>> }
>
> [...]
>
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index ea51369..80142b2 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -6587,6 +6587,9 @@ qemuBuildCpuCommandLine(virCommand *cmd,
>> virBufferAddLit(&buf, ",kvm-poll-control=on");
>> break;
>>
>> + case VIR_DOMAIN_KVM_DIRTY_RING:
>> + break;
>> +
>> case VIR_DOMAIN_KVM_LAST:
>> break;
>> }
>> @@ -6758,29 +6761,41 @@ qemuBuildNameCommandLine(virCommand *cmd,
>> return 0;
>> }
>>
>> +static void
>> +qemuBuildAccelCommandLineTcgOptions(void)
>> +{
>> + /* TODO: build command line tcg accelerator
>> + * property like tb-size */
>
> Is this patch incomplete? In case you don't want to support this for TCG
> you should forbid it in validation rather than silently ignoring it.
ok, i'll forbid it next version.
>
>> +}
>> +
>> +static void
>> +qemuBuildAccelCommandLineKvmOptions(virBuffer *buf,
>> + const virDomainDef *def)
>> +{
>> + if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON &&
>> + def->kvm_features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) {
>> + virBufferAsprintf(buf, ",dirty-ring-size=%d", def->dirty_ring_size);
>> + }
>> +}
>> +
>> static int
>> -qemuBuildMachineCommandLine(virCommand *cmd,
>> - virQEMUDriverConfig *cfg,
>> - const virDomainDef *def,
>> - virQEMUCaps *qemuCaps,
>> - qemuDomainObjPrivate *priv)
>> +qemuBuildAccelCommandLine(virCommand *cmd,
>> + const virDomainDef *def)
>> {
>> - virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
>> - virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
>> - virCPUDef *cpu = def->cpu;
>> + /* the '-machine' options for accelerator are legacy,
>> + * using the '-accel' options by default */
>> g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>> - size_t i;
>> -
>> - virCommandAddArg(cmd, "-machine");
>> - virBufferAdd(&buf, def->os.machine, -1);
>> + virCommandAddArg(cmd, "-accel");
>
> This MUST be a separate commit it's a fully separate change which
> requires separate justification.
ok.
>
>>
>> switch ((virDomainVirtType)def->virtType) {
>> case VIR_DOMAIN_VIRT_QEMU:
>> - virBufferAddLit(&buf, ",accel=tcg");
>> + virBufferAddLit(&buf, "tcg");
>> + qemuBuildAccelCommandLineTcgOptions();
>> break;
>>
>> case VIR_DOMAIN_VIRT_KVM:
>> - virBufferAddLit(&buf, ",accel=kvm");
>> + virBufferAddLit(&buf, "kvm");
>> + qemuBuildAccelCommandLineKvmOptions(&buf, def);
>> break;
>>
>> case VIR_DOMAIN_VIRT_KQEMU:
>> @@ -6808,6 +6823,61 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>> return -1;
>> }
>>
>> + virCommandAddArgBuffer(cmd, &buf);
>> + return 0;
>> +}
>> +
>> +static int
>> +qemuBuildMachineCommandLine(virCommand *cmd,
>> + virQEMUDriverConfig *cfg,
>> + const virDomainDef *def,
>> + virQEMUCaps *qemuCaps,
>> + qemuDomainObjPrivate *priv)
>> +{
>> + virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
>> + virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
>> + virCPUDef *cpu = def->cpu;
>> + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>> + size_t i;
>> +
>> + virCommandAddArg(cmd, "-machine");
>> + virBufferAdd(&buf, def->os.machine, -1);
>> +
>> + /* QEMU lower than 2.9.0 do not support '-accel'
>> + * fallback to '-machine accel' */
>
> We don't support those qemu versions. This is the reason why you must
> split the patch. A refactor like this has nothing to do with the feature
> addition and should be justified and reviewed separately.
ok. i'll post a patch just changing the way of build commandline before
the dirty ring feature patchset.
>
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL)) {
>> + switch ((virDomainVirtType)def->virtType) {
>> + case VIR_DOMAIN_VIRT_QEMU:
>> + virBufferAddLit(&buf, ",accel=tcg");
>> + break;
>> + case VIR_DOMAIN_VIRT_KVM:
>> + virBufferAddLit(&buf, ",accel=kvm");
>> + break;
>> + case VIR_DOMAIN_VIRT_KQEMU:
>
>
> [...]
>
>> @@ -10402,6 +10472,11 @@ qemuBuildCommandLine(virQEMUDriver *driver,
>> if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps, priv) < 0)
>> return NULL;
>>
>> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL)) {
>> + if (qemuBuildAccelCommandLine(cmd, def) < 0)
>> + return NULL;
>> + }
>> +
>> qemuBuildTSEGCommandLine(cmd, def);
>>
>> if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
>
More information about the libvir-list
mailing list