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

Michal Privoznik <mprivozn@xxxxxxxxxx> · Tue, 17 Nov 2020 14:37:09 +0100

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@xxxxxxxxxx>
---
   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







