Re: [libvirt PATCH 1/2] qemu: support kvm-poll-control performance hint

On Mon, 2020-11-16 at 14:27 +0100, Michal Privoznik wrote: > On 11/13/20 9:49 AM, Tim Wiederhake wrote: >> QEMU version 4.2 introduced a performance feature under commit >> d645e13287 ("kvm: i386: halt poll control MSR support"). >> >> This patch adds a new KVM feature 'poll-control' to set this >> performance >> hint for KVM guests. The feature is off by default. >> >> To enable this hint and have libvirt add "-cpu host,kvm-poll- >> control=on" >> to the QEMU command line, the following XML code needs to be added >> to the >> guest's domain description: >> >> <features> >> <kvm> >> <poll-control state='on'/> >> </kvm> >> </features> >> >> Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; >> --- >> docs/formatdomain.rst | 14 ++++++++------ >> docs/schemas/domaincommon.rng | 5 +++++ >> src/conf/domain_conf.c | 4 ++++ >> src/conf/domain_conf.h | 1 + >> src/qemu/qemu_command.c | 5 +++++ >> 5 files changed, 23 insertions(+), 6 deletions(-) >> >> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst >> index a6ab845f92..cd5d7aae56 100644 >> --- a/docs/formatdomain.rst >> +++ b/docs/formatdomain.rst >> @@ -1766,6 +1766,7 @@ Hypervisors may allow certain CPU / machine >> features to be toggled on/off. >> <kvm> >> <hidden state='on'/> >> <hint-dedicated state='on'/> >> + <poll-control='on'/> >> </kvm> >> <xen> >> <e820_host state='on'/> >> @@ -1848,12 +1849,13 @@ are: >> ``kvm`` >> Various features to change the behavior of the KVM hypervisor. >> >> - ============== >> =================================================================== >> === ======= ============================ >> - Feature Description >> Value Since >> - ============== >> =================================================================== >> === ======= ============================ >> - hidden Hide the KVM hypervisor from standard MSR based >> discovery on, off :since:`1.2.8 (QEMU 2.1.0)` >> - hint-dedicated Allows a guest to enable optimizations when >> running on dedicated vCPUs on, off :since:`5.7.0 (QEMU 2.12.0)` >> - ============== >> =================================================================== >> === ======= ============================ >> + ============== >> =================================================================== >> ========= ======= ============================ >> + Feature Description >> Value Since >> + ============== >> =================================================================== >> ========= ======= ============================ >> + hidden Hide the KVM hypervisor from standard MSR based >> discovery on, off :since:`1.2.8 (QEMU 2.1.0)` >> + hint-dedicated Allows a guest to enable optimizations when >> running on dedicated vCPUs on, off :since:`5.7.0 (QEMU >> 2.12.0)` >> + poll-control Decrease IO completion latency by introducing a >> grace period of busy waiting on, off :since:`6.11.0 (QEMU 4.2)` >> + ============== >> =================================================================== >> ========= ======= ============================ >> >> ``xen`` >> Various features to change the behavior of the Xen hypervisor. >> diff --git a/docs/schemas/domaincommon.rng >> b/docs/schemas/domaincommon.rng >> index e0d09f9c03..f86a854863 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -6512,6 +6512,11 @@ >> <ref name="featurestate"/> >> </element> >> </optional> >> + <optional> >> + <element name="poll-control"> >> + <ref name="featurestate"/> >> + </element> >> + </optional> >> </interleave> >> </element> >> </define> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 9199771dc0..2b56993845 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -208,6 +208,7 @@ VIR_ENUM_IMPL(virDomainKVM, >> VIR_DOMAIN_KVM_LAST, >> "hidden", >> "hint-dedicated", >> + "poll-control", >> ); >> >> VIR_ENUM_IMPL(virDomainXen, >> @@ -19823,6 +19824,7 @@ virDomainFeaturesDefParse(virDomainDefPtr >> def, >> switch ((virDomainKVM) feature) { >> case VIR_DOMAIN_KVM_HIDDEN: >> case VIR_DOMAIN_KVM_DEDICATED: >> + case VIR_DOMAIN_KVM_POLLCONTROL: >> if (!(tmp = virXMLPropString(nodes[i], >> "state"))) { >> virReportError(VIR_ERR_XML_ERROR, >> _("missing 'state' >> attribute for " >> @@ -23982,6 +23984,7 @@ >> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, >> switch ((virDomainKVM) i) { >> case VIR_DOMAIN_KVM_HIDDEN: >> case VIR_DOMAIN_KVM_DEDICATED: >> + case VIR_DOMAIN_KVM_POLLCONTROL: >> if (src->kvm_features[i] != dst->kvm_features[i]) >> { >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("State of KVM feature '%s' >> differs: " >> @@ -29695,6 +29698,7 @@ virDomainDefFormatFeatures(virBufferPtr >> buf, >> switch ((virDomainKVM) j) { >> case VIR_DOMAIN_KVM_HIDDEN: >> case VIR_DOMAIN_KVM_DEDICATED: >> + case VIR_DOMAIN_KVM_POLLCONTROL: >> if (def->kvm_features[j]) >> virBufferAsprintf(&childBuf, "<%s >> state='%s'/>\n", >> virDomainKVMTypeToStrin >> g(j), >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >> index 16c050a3ea..3e3d4bd002 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h >> @@ -1912,6 +1912,7 @@ typedef enum { >> typedef enum { >> VIR_DOMAIN_KVM_HIDDEN = 0, >> VIR_DOMAIN_KVM_DEDICATED, >> + VIR_DOMAIN_KVM_POLLCONTROL, >> >> VIR_DOMAIN_KVM_LAST >> } virDomainKVM; > > Until here, the change is okay. > >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index 0eec35da16..34b5746c1a 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -6464,6 +6464,11 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, >> virBufferAddLit(&buf, ",kvm-hint- >> dedicated=on"); >> break; >> >> + case VIR_DOMAIN_KVM_POLLCONTROL: >> + if (def->kvm_features[i] == >> VIR_TRISTATE_SWITCH_ON) >> + virBufferAddLit(&buf, ",kvm-poll-control=on"); >> + break; >> + >> /* coverity[dead_error_begin] */ >> case VIR_DOMAIN_KVM_LAST: >> break; >> > > But this should go into the next patch IMO. The reason is that if > somebody wants to backport only XML part they should not be forced > to > backport QEMU too. In this specific case it probably doesn't doesn't > matter that much, because no other driver uses KVM features. But I > think > it's still wort it (e.g. to gain the habit). > > In theory, src/conf/ (plus docs/rng change) can be viewed as > "frontend" > and driver implementation can be viewed as "backend". > What is also acceptable is having src/conf + xml2xml tests in one > patch, > because xml2xml tests do test the change made to src/conf. So in > this > specific case, it is also acceptable to have src/conf/ and docs/rng > change and tests/.../*.xml changes in one patch, qemu + *.args in the > other. > > I'm sorry for being picky. The change itself is okay. > > Michal Thanks for the review! I don't quite understand though, if you could please clarify what you mean: If the patch changing src/conf/domain_conf.{c,h} goes in first, we have one commit where "<poll-control state='on'/>" would be accepted but silently discarded, potentially messing with people git-bisect'ing in the future.