Re: [libvirt PATCH 7/8] cpu_x86: Penalize disabled features when computing CPU model

5 May 2022

On 5/4/22 18:54, Jiri Denemark wrote:
...
For finding the best matching CPU model for a given set of features
while we don't know the CPU signature (i.e., when computing a baseline
CPU model) we've been using a "shortest list of features" heuristics.
This works well if new CPU models are supersets of older models, but
that's not always the case. As a result it may actually select a new CPU
model as a baseline while removing some features from it to make it
compatible with older models. This is in general worse than using an old
CPU model with a bunch of added features as a guest OS or apps may crash
when using features that were disabled.
On the other hand we don't want to end up with a very old model which
would guarantee no disabled features as it could stop a guest OS or apps
from using some features provided by the CPU because they would not
expect them on such an old CPU.
This patch changes the heuristics to something in between. Enabled and
disabled features are counted separately so that a CPU model requiring
some features to be disabled looks worse than a model with fewer
disabled features even if its complete list of features is longer. The
penalty given for each additional disabled feature gets bigger to make
longer list of disabled features look even worse.
https://bugzilla.redhat.com/show_bug.cgi?id=1851227
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/cpu/cpu_x86.c                             | 44 ++++++++++++++++---
 .../x86_64-cpuid-Atom-D510-guest.xml          |  5 ++-
 .../x86_64-cpuid-Atom-N450-guest.xml          |  5 ++-
 .../x86_64-cpuid-Phenom-B95-json.xml          | 21 +++++----
 ...id-baseline-Broadwell-IBRS+Cascadelake.xml | 11 +++--
 ..._64-cpuid-baseline-Cascadelake+Icelake.xml | 13 +++---
 ...puid-baseline-Cascadelake+Skylake-IBRS.xml |  5 ++-
 ...6_64-cpuid-baseline-Cooperlake+Icelake.xml | 13 +++---
 .../x86_64-host+guest,models-result.xml       | 10 +++--
 .../domaincapsdata/qemu_3.1.0-tcg.x86_64.xml  | 35 +++++++++------
 .../domaincapsdata/qemu_4.0.0-tcg.x86_64.xml  | 36 ++++++++-------
 .../domaincapsdata/qemu_4.1.0-tcg.x86_64.xml  | 37 +++++++++-------
 .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml  | 37 +++++++++-------
 .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml  | 36 +++++++++------
 .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml  | 36 +++++++++------
 .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml  | 36 +++++++++------
 .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml  | 36 +++++++++------
 .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml  | 36 +++++++++------
 .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml  | 36 +++++++++------
 .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml  | 36 +++++++++------
 tests/qemuxml2argvdata/cpu-fallback.args      |  2 +-
 .../cpu-host-model-cmt.x86_64-4.0.0.args      |  2 +-
 .../cpu-host-model-fallback.args              |  2 +-
 23 files changed, 330 insertions(+), 200 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index fdee107ce9..3001fc2b8f 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1956,23 +1956,57 @@ virCPUx86Compare(virCPUDef *host,
 }
+/* Base penalty for disabled features. */
+#define BASE_PENALTY 2
+
 static int
 virCPUx86CompareCandidateFeatureList(virCPUDef *cpuCurrent,
                                      virCPUDef *cpuCandidate)
 {
     size_t current = cpuCurrent->nfeatures;
+    size_t enabledCurrent = current;
+    size_t disabledCurrent = 0;
     size_t candidate = cpuCandidate->nfeatures;
+    size_t enabled = candidate;
+    size_t disabled = 0;
+
+    if (cpuCandidate->type != VIR_CPU_TYPE_HOST) {
+        size_t i;
+        int penalty = BASE_PENALTY;
+
+        for (i = 0; i < enabledCurrent; i++) {
+            if (cpuCurrent->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
+                enabledCurrent--;
+                disabledCurrent += penalty;
+                penalty++;
+            }
+        }
+        current = enabledCurrent + disabledCurrent;
+
+        penalty = BASE_PENALTY;
+        for (i = 0; i < enabled; i++) {
+            if (cpuCandidate->features[i].policy == VIR_CPU_FEATURE_DISABLE) {
+                enabled--;
+                disabled += penalty;
+                penalty++;
+            }
+        }
+        candidate = enabled + disabled;
+    }
-    if (candidate < current) {
-        VIR_DEBUG("%s is better than %s: %zu < %zu",
+    if (candidate < current ||
+        (candidate == current && disabled < disabledCurrent)) {
+        VIR_DEBUG("%s is better than %s: %zu (%zu, %zu) < %zu (%zu, %zu)",
                   cpuCandidate->model, cpuCurrent->model,
-                  candidate, current);
+                  candidate, enabled, disabled,
+                  current, enabledCurrent, disabledCurrent);
         return 1;
     }
-    VIR_DEBUG("%s is not better than %s: %zu >= %zu",
+    VIR_DEBUG("%s is not better than %s: %zu (%zu, %zu) >= %zu (%zu, %zu)",
               cpuCandidate->model, cpuCurrent->model,
-              candidate, current);
+              candidate, enabled, disabled,
+              current, enabledCurrent, disabledCurrent);
     return 0;
What a nice algorithm you have here :-)

Michal

    

Re: [libvirt PATCH 7/8] cpu_x86: Penalize disabled features when computing CPU model

Michal Prívozník