[libvirt PATCH 1/2] qemu: support kvm-poll-control performance hint
Michal Privoznik
mprivozn at redhat.com
Tue Nov 17 13:37:09 UTC 2020
On 11/17/20 2:26 PM, Tim Wiederhake wrote:
> 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 at redhat.com>
>>> ---
>>> 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.
Ah, we don't error out? Alright then, let's go with your patches as they
are. Sorry for the noise.
Michal
More information about the libvir-list
mailing list