Re: [PATCH v7 1/2] qemu: support dirty ring feature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 12/14/21 17:22, Michal Prívozník wrote:
On 11/23/21 15:36, huangy81@xxxxxxxxxxxxxxx wrote:
From: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>

Dirty ring feature was introduced in qemu-6.1.0, this patch
add the corresponding feature named 'dirty-ring', which enable
dirty ring feature when starting vm.

To implement the dirty-ring feature, dirty_ring_size in struct
"_virDomainDef" is introduced to hold the dirty ring size
configured in xml, and it will be used as dirty-ring-size
property of kvm accelerator when building qemu commandline,
it is something like "-accel dirty-ring-size=xxx".

To enable the feature, 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", property "size" must be specified, which
should be power of 2 and range in [1024, 65526].

Signed-off-by: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
---
  docs/formatdomain.rst         | 18 ++++++------
  docs/schemas/domaincommon.rng | 10 +++++++
  src/conf/domain_conf.c        | 54 +++++++++++++++++++++++++++++++++++
  src/conf/domain_conf.h        |  4 +++
  src/qemu/qemu_command.c       | 12 ++++++++
  5 files changed, 90 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index eb8c973cf1..ea69b61c70 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1843,6 +1843,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off.
         <hint-dedicated state='on'/>
         <poll-control state='on'/>
         <pv-ipi state='off'/>
+       <dirty-ring state='on' size='4096'/>

I was confused at first, what units is @size in but looking into the
qemu docs it's unit-less number:

   "[dirty-ring-size] it controls the size of the per-vCPU dirty page
    ring buffer (number of entries for each vCPU)."

Therefore I'm okay with having it as a plain attribute. Otherwise for
values with units (traditionally size units like KiB/MiB/...) I would
advise to go with an extra element.

       </kvm>
       <xen>
         <e820_host state='on'/>
@@ -1925,14 +1926,15 @@ 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)`
-   poll-control   Decrease IO completion latency by introducing a grace period of busy waiting on, off :since:`6.10.0 (QEMU 4.2)`
-   pv-ipi         Paravirtualized send IPIs                                                    on, off :since:`7.10.0 (QEMU 3.1)`
-   ============== ============================================================================ ======= ============================
+   ============== ============================================================================ ====================================================== ============================
+   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.10.0 (QEMU 4.2)`
+   pv-ipi         Paravirtualized send IPIs                                                    on, off                                                :since:`7.10.0 (QEMU 3.1)`
+   dirty-ring     Enable dirty ring feature                                                    on, off; size - must be power of 2, range [1024,65536] :since:`7.10.0 (QEMU 6.1)`
+   ============== ============================================================================ ====================================================== ============================
``xen``
     Various features to change the behavior of the Xen hypervisor.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f01b7a6470..5f9fe3cc58 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -7212,6 +7212,16 @@
              <ref name="featurestate"/>
            </element>
          </optional>
+        <optional>
+          <element name="dirty-ring">
+            <ref name="featurestate"/>
+            <optional>
+              <attribute name="size">
+                <data type="unsignedInt"/>
+              </attribute>
+            </optional>
+          </element>
+        </optional>
        </interleave>
      </element>
    </define>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 552d43b845..6f3c925b55 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -205,6 +205,7 @@ VIR_ENUM_IMPL(virDomainKVM,
                "hint-dedicated",
                "poll-control",
                "pv-ipi",
+              "dirty-ring",
  );
VIR_ENUM_IMPL(virDomainXen,
@@ -17589,6 +17590,25 @@ virDomainFeaturesKVMDefParse(virDomainDef *def,
def->kvm_features[feature] = value; + /* dirty ring feature should parse size property */
+        if (((virDomainKVM) feature == VIR_DOMAIN_KVM_DIRTY_RING) &&
+             (value == VIR_TRISTATE_SWITCH_ON))  {
+
+            if (virXMLPropUInt(node, "size", 0, VIR_XML_PROP_REQUIRED,
+                               &def->dirty_ring_size) < 0) {
+                return -1;
+            }
+
+            if ((def->dirty_ring_size != VIR_ROUND_UP_POWER_OF_TWO(def->dirty_ring_size)) ||

VIR_IS_POW2() which works even for other types than uint.

+                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;
+            }
+        }
+
          node = xmlNextElementSibling(node);
      }
@@ -21824,7 +21844,27 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
                                     virTristateSwitchTypeToString(dst->kvm_features[i]));
                      return false;
                  }
+                break;
+ 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: "
+                                     "source: '%s', destination: '%s'"),
+                                   virDomainKVMTypeToString(i),
+                                   virTristateSwitchTypeToString(src->kvm_features[i]),
+                                   virTristateSwitchTypeToString(dst->kvm_features[i]));
+                    return false;
+                }
+
+                if (src->dirty_ring_size != dst->dirty_ring_size) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                                   _("dirty ring size of KVM feature '%s' differs: "
+                                     "source: '%d', destination: '%d'"),
+                                   virDomainKVMTypeToString(i),
+                                   src->dirty_ring_size, dst->dirty_ring_size);
+                    return false;
+                }
                  break;
case VIR_DOMAIN_KVM_LAST:
@@ -27872,6 +27912,20 @@ virDomainDefFormatFeatures(virBuffer *buf,
                                                def->kvm_features[j]));
                      break;
+ case VIR_DOMAIN_KVM_DIRTY_RING:
+                    if (def->kvm_features[j] != VIR_TRISTATE_SWITCH_ABSENT) {
+                        virBufferAsprintf(&childBuf, "<%s state='%s'",
+                                          virDomainKVMTypeToString(j),
+                                          virTristateSwitchTypeToString(def->kvm_features[j]));
+                        if (def->dirty_ring_size) {
+                            virBufferAsprintf(&childBuf, " size='%d'/>\n",
+                                              def->dirty_ring_size);
+                        } else {
+                            virBufferAddLit(&childBuf, "/>\n");
+                        }
+                    }
+                    break;
+
                  case VIR_DOMAIN_KVM_LAST:
                      break;
                  }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 8634960313..026edde88f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2084,6 +2084,7 @@ typedef enum {
      VIR_DOMAIN_KVM_DEDICATED,
      VIR_DOMAIN_KVM_POLLCONTROL,
      VIR_DOMAIN_KVM_PVIPI,
+    VIR_DOMAIN_KVM_DIRTY_RING,
VIR_DOMAIN_KVM_LAST
  } virDomainKVM;
@@ -2933,6 +2934,9 @@ struct _virDomainDef {
                               should be re-run before starting */
unsigned int scsiBusMaxUnit;
+
+    /* size of dirty ring for each vcpu */
+    unsigned int dirty_ring_size;

This feels weird. I haven't followed previous versions that closely, but
what I did for TCG features is introducing a struct that lives close to
other features. This could then be something like:

typedef struct _virDomainFeatureKVM virDomainFeatureKVM;
struct _virDomainFeatureKVM {
     int features[VIR_DOMAIN_KVM_LAST];

     unsigned int dirty_ring_size; /* size of dirty ring for each vCPU, no units */
};

  struct _virDomainDef {
      ...
-    int kvm_features[VIR_DOMAIN_KVM_LAST];
+    virDomainFeatureKVM *kvm_features;
      ...
  };


So here's what I suggest doing - let me post a patch that changes 'int
kvm_features' into a separate struct. I would squash it into yours but
it turned out to be quite lengthy change. Then I'll do changes necessary
for your patch (which will be trivial after that).

Michal

Ok, i'll rebase the master once the changes get merged and test if the dirty ring still works.

--
Best Regards
Hyman Huang(黄勇)





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux