[libvirt] [PATCH] libxl: don't hardcode scheduler weight

Jim Fehlig posted 1 patch 6 years, 1 month ago
Failed in applying to current master (apply log)
Test syntax-check passed
src/libxl/libxl_conf.c                             |  4 +-
tests/libxlxml2domconfigdata/basic-hvm.json        |  2 +-
tests/libxlxml2domconfigdata/basic-pv.json         |  2 +-
tests/libxlxml2domconfigdata/cpu-shares-hvm.json   | 89 ++++++++++++++++++++++
tests/libxlxml2domconfigdata/cpu-shares-hvm.xml    | 39 ++++++++++
tests/libxlxml2domconfigdata/moredevs-hvm.json     |  2 +-
tests/libxlxml2domconfigdata/multiple-ip.json      |  2 +-
.../libxlxml2domconfigdata/variable-clock-hvm.json |  2 +-
tests/libxlxml2domconfigdata/vnuma-hvm.json        |  2 +-
tests/libxlxml2domconfigtest.c                     |  1 +
10 files changed, 138 insertions(+), 7 deletions(-)
[libvirt] [PATCH] libxl: don't hardcode scheduler weight
Posted by Jim Fehlig 6 years, 1 month ago
Long ago in commit dfa1e1dd53 the scheduler weight was accidentally
hardcoded to 1000. Weight is a setting with no unit since it is
relative to the weight of other domains. If no weight is specified,
libxl defaults to 256.

Instead of hardcoding the weight to 1000, honor any <shares> specified
in <cputune>. libvirt's notion of shares is synonomous to libxl's
scheduler weight setting. If shares is unspecified, defer default
weight setting to libxl.

Removing the hardcoded weight required some test fixup. While at it,
add an explicit test for <shares> conversion to scheduler weight.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

Honoring <shares> specified by the user is certainly desirable,
but I'm not sure about changing the default weight. One problematic
scenario that came to mind: Several domains started by pre-patch
libvirtd with weight=1000, update libvirtd with patch, start more
domains with weight=256. The pre-patch domains unknowingly have
a weight nearly 4 times that of post-patch domains.

 src/libxl/libxl_conf.c                             |  4 +-
 tests/libxlxml2domconfigdata/basic-hvm.json        |  2 +-
 tests/libxlxml2domconfigdata/basic-pv.json         |  2 +-
 tests/libxlxml2domconfigdata/cpu-shares-hvm.json   | 89 ++++++++++++++++++++++
 tests/libxlxml2domconfigdata/cpu-shares-hvm.xml    | 39 ++++++++++
 tests/libxlxml2domconfigdata/moredevs-hvm.json     |  2 +-
 tests/libxlxml2domconfigdata/multiple-ip.json      |  2 +-
 .../libxlxml2domconfigdata/variable-clock-hvm.json |  2 +-
 tests/libxlxml2domconfigdata/vnuma-hvm.json        |  2 +-
 tests/libxlxml2domconfigtest.c                     |  1 +
 10 files changed, 138 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index dcd46873c..e40145cfa 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -366,7 +366,9 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
         }
     }
 
-    b_info->sched_params.weight = 1000;
+    if (def->cputune.sharesSpecified)
+        b_info->sched_params.weight = def->cputune.shares;
+
     b_info->max_memkb = virDomainDefGetMemoryInitial(def);
     b_info->target_memkb = def->mem.cur_balloon;
     if (hvm) {
diff --git a/tests/libxlxml2domconfigdata/basic-hvm.json b/tests/libxlxml2domconfigdata/basic-hvm.json
index 6fa41f34f..ccd585385 100644
--- a/tests/libxlxml2domconfigdata/basic-hvm.json
+++ b/tests/libxlxml2domconfigdata/basic-hvm.json
@@ -19,7 +19,7 @@
         "device_model_version": "qemu_xen",
         "device_model": "/bin/true",
         "sched_params": {
-            "weight": 1000
+
         },
         "type.hvm": {
             "pae": "True",
diff --git a/tests/libxlxml2domconfigdata/basic-pv.json b/tests/libxlxml2domconfigdata/basic-pv.json
index 7e1abd3d6..0f846daab 100644
--- a/tests/libxlxml2domconfigdata/basic-pv.json
+++ b/tests/libxlxml2domconfigdata/basic-pv.json
@@ -15,7 +15,7 @@
         "max_memkb": 524288,
         "target_memkb": 524288,
         "sched_params": {
-            "weight": 1000
+
         },
         "type.pv": {
             "bootloader": "pygrub"
diff --git a/tests/libxlxml2domconfigdata/cpu-shares-hvm.json b/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
new file mode 100644
index 000000000..2e647eada
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
@@ -0,0 +1,89 @@
+{
+    "c_info": {
+        "type": "hvm",
+        "name": "test-hvm",
+        "uuid": "2147d599-9cc6-c0dc-92ab-4064b5446e9b"
+    },
+    "b_info": {
+        "max_vcpus": 4,
+        "avail_vcpus": [
+            0,
+            1,
+            2,
+            3
+        ],
+        "max_memkb": 1048576,
+        "target_memkb": 1048576,
+        "video_memkb": 8192,
+        "shadow_memkb": 12288,
+        "device_model_version": "qemu_xen",
+        "device_model": "/bin/true",
+        "sched_params": {
+            "weight": 1500
+        },
+        "type.hvm": {
+            "pae": "True",
+            "apic": "True",
+            "acpi": "True",
+            "vga": {
+                "kind": "cirrus"
+            },
+            "vnc": {
+                "enable": "True",
+                "listen": "0.0.0.0",
+                "findunused": "False"
+            },
+            "sdl": {
+                "enable": "False"
+            },
+            "spice": {
+
+            },
+            "boot": "c",
+            "rdm": {
+
+            }
+        },
+        "arch_arm": {
+
+        }
+    },
+    "disks": [
+        {
+            "pdev_path": "/var/lib/xen/images/test-hvm.img",
+            "vdev": "hda",
+            "backend": "qdisk",
+            "format": "raw",
+            "removable": 1,
+            "readwrite": 1
+        }
+    ],
+    "nics": [
+        {
+            "devid": 0,
+            "mac": "00:16:3e:66:12:b4",
+            "bridge": "br0",
+            "script": "/etc/xen/scripts/vif-bridge",
+            "nictype": "vif_ioemu"
+        }
+    ],
+    "vfbs": [
+        {
+	    "devid": -1,
+            "vnc": {
+                "enable": "True",
+                "listen": "0.0.0.0",
+                "findunused": "False"
+            },
+            "sdl": {
+                "enable": "False"
+            }
+        }
+    ],
+    "vkbs": [
+        {
+            "devid": -1
+        }
+    ],
+    "on_reboot": "restart"
+}
diff --git a/tests/libxlxml2domconfigdata/cpu-shares-hvm.xml b/tests/libxlxml2domconfigdata/cpu-shares-hvm.xml
new file mode 100644
index 000000000..49085b4b7
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/cpu-shares-hvm.xml
@@ -0,0 +1,39 @@
+<domain type='xen'>
+  <name>test-hvm</name>
+  <description>None</description>
+  <uuid>2147d599-9cc6-c0dc-92ab-4064b5446e9b</uuid>
+  <memory>1048576</memory>
+  <currentMemory>1048576</currentMemory>
+  <vcpu>4</vcpu>
+  <cputune>
+    <shares>1500</shares>
+  </cputune>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <clock offset='utc'/>
+  <os>
+    <type>hvm</type>
+    <loader>/usr/lib/xen/boot/hvmloader</loader>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <apic/>
+    <acpi/>
+    <pae/>
+  </features>
+  <devices>
+    <emulator>/bin/true</emulator>
+    <disk type='file' device='disk'>
+      <driver name='qemu'/>
+      <source file='/var/lib/xen/images/test-hvm.img'/>
+      <target dev='hda'/>
+    </disk>
+    <interface type='bridge'>
+      <source bridge='br0'/>
+      <mac address='00:16:3e:66:12:b4'/>
+      <script path='/etc/xen/scripts/vif-bridge'/>
+    </interface>
+    <graphics type='vnc' port='-1' autoport='yes' listen='0.0.0.0'/>
+  </devices>
+</domain>
diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.json b/tests/libxlxml2domconfigdata/moredevs-hvm.json
index 8f861f34e..7bfd68bd6 100644
--- a/tests/libxlxml2domconfigdata/moredevs-hvm.json
+++ b/tests/libxlxml2domconfigdata/moredevs-hvm.json
@@ -21,7 +21,7 @@
         "device_model_version": "qemu_xen",
         "device_model": "/bin/true",
         "sched_params": {
-            "weight": 1000
+
         },
         "type.hvm": {
             "pae": "True",
diff --git a/tests/libxlxml2domconfigdata/multiple-ip.json b/tests/libxlxml2domconfigdata/multiple-ip.json
index 4aff61d4b..80dca8261 100644
--- a/tests/libxlxml2domconfigdata/multiple-ip.json
+++ b/tests/libxlxml2domconfigdata/multiple-ip.json
@@ -15,7 +15,7 @@
         "max_memkb": 524288,
         "target_memkb": 524288,
         "sched_params": {
-            "weight": 1000
+
         },
         "type.pv": {
             "bootloader": "pygrub"
diff --git a/tests/libxlxml2domconfigdata/variable-clock-hvm.json b/tests/libxlxml2domconfigdata/variable-clock-hvm.json
index 49d0a4330..69be9c64c 100644
--- a/tests/libxlxml2domconfigdata/variable-clock-hvm.json
+++ b/tests/libxlxml2domconfigdata/variable-clock-hvm.json
@@ -21,7 +21,7 @@
         "device_model_version": "qemu_xen",
         "device_model": "/bin/true",
         "sched_params": {
-            "weight": 1000
+
         },
         "type.hvm": {
             "pae": "True",
diff --git a/tests/libxlxml2domconfigdata/vnuma-hvm.json b/tests/libxlxml2domconfigdata/vnuma-hvm.json
index 3a5071e14..3b2fc5f40 100644
--- a/tests/libxlxml2domconfigdata/vnuma-hvm.json
+++ b/tests/libxlxml2domconfigdata/vnuma-hvm.json
@@ -107,7 +107,7 @@
         "device_model_version": "qemu_xen",
         "device_model": "/bin/true",
         "sched_params": {
-            "weight": 1000
+
         },
         "type.hvm": {
             "pae": "True",
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index ea3d2884e..6eec4c752 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -192,6 +192,7 @@ mymain(void)
 
     DO_TEST("basic-pv");
     DO_TEST("basic-hvm");
+    DO_TEST("cpu-shares-hvm");
     DO_TEST("variable-clock-hvm");
     DO_TEST("moredevs-hvm");
     DO_TEST("vnuma-hvm");
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: don't hardcode scheduler weight
Posted by John Ferlan 6 years ago

On 02/22/2018 05:20 PM, Jim Fehlig wrote:
> Long ago in commit dfa1e1dd53 the scheduler weight was accidentally
> hardcoded to 1000. Weight is a setting with no unit since it is
> relative to the weight of other domains. If no weight is specified,
> libxl defaults to 256.
> 
> Instead of hardcoding the weight to 1000, honor any <shares> specified
> in <cputune>. libvirt's notion of shares is synonomous to libxl's
> scheduler weight setting. If shares is unspecified, defer default
> weight setting to libxl.
> 
> Removing the hardcoded weight required some test fixup. While at it,
> add an explicit test for <shares> conversion to scheduler weight.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> Honoring <shares> specified by the user is certainly desirable,
> but I'm not sure about changing the default weight. One problematic
> scenario that came to mind: Several domains started by pre-patch
> libvirtd with weight=1000, update libvirtd with patch, start more
> domains with weight=256. The pre-patch domains unknowingly have
> a weight nearly 4 times that of post-patch domains.

Put 'em all on a diet.  Don't try to do it overnight though because that
never works and it makes them all hangry...  Slowly try to have them
lose weight over the course a few months and in no time they'll be slim,
trim, and less cumbersome. Whether they'll be happy - who knows ;-)

> 
>  src/libxl/libxl_conf.c                             |  4 +-
>  tests/libxlxml2domconfigdata/basic-hvm.json        |  2 +-
>  tests/libxlxml2domconfigdata/basic-pv.json         |  2 +-
>  tests/libxlxml2domconfigdata/cpu-shares-hvm.json   | 89 ++++++++++++++++++++++
>  tests/libxlxml2domconfigdata/cpu-shares-hvm.xml    | 39 ++++++++++
>  tests/libxlxml2domconfigdata/moredevs-hvm.json     |  2 +-
>  tests/libxlxml2domconfigdata/multiple-ip.json      |  2 +-
>  .../libxlxml2domconfigdata/variable-clock-hvm.json |  2 +-
>  tests/libxlxml2domconfigdata/vnuma-hvm.json        |  2 +-
>  tests/libxlxml2domconfigtest.c                     |  1 +
>  10 files changed, 138 insertions(+), 7 deletions(-)
> 

Perhaps this is a "news" worthy type change along with the caveat about
those more weighty domains that were allowed to feed at the trough
without being limited.

In any case, this all seems reasonable to me, but it also seems you have
a self inflicted merge conflict.

Without compiling, via visual inspection:

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: don't hardcode scheduler weight
Posted by Jim Fehlig 6 years ago
On 03/02/2018 02:38 PM, John Ferlan wrote:
> 
> 
> On 02/22/2018 05:20 PM, Jim Fehlig wrote:
>> Long ago in commit dfa1e1dd53 the scheduler weight was accidentally
>> hardcoded to 1000. Weight is a setting with no unit since it is
>> relative to the weight of other domains. If no weight is specified,
>> libxl defaults to 256.
>>
>> Instead of hardcoding the weight to 1000, honor any <shares> specified
>> in <cputune>. libvirt's notion of shares is synonomous to libxl's
>> scheduler weight setting. If shares is unspecified, defer default
>> weight setting to libxl.
>>
>> Removing the hardcoded weight required some test fixup. While at it,
>> add an explicit test for <shares> conversion to scheduler weight.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>
>> Honoring <shares> specified by the user is certainly desirable,
>> but I'm not sure about changing the default weight. One problematic
>> scenario that came to mind: Several domains started by pre-patch
>> libvirtd with weight=1000, update libvirtd with patch, start more
>> domains with weight=256. The pre-patch domains unknowingly have
>> a weight nearly 4 times that of post-patch domains.
> 
> Put 'em all on a diet.  Don't try to do it overnight though because that
> never works and it makes them all hangry...  Slowly try to have them
> lose weight over the course a few months and in no time they'll be slim,
> trim, and less cumbersome. Whether they'll be happy - who knows ;-)

Heh, funny :-).

I'll manage this downstream by ensuring the patch only lands in a new product 
release, where no domains will be running when libvirt is installed.

>>
>>   src/libxl/libxl_conf.c                             |  4 +-
>>   tests/libxlxml2domconfigdata/basic-hvm.json        |  2 +-
>>   tests/libxlxml2domconfigdata/basic-pv.json         |  2 +-
>>   tests/libxlxml2domconfigdata/cpu-shares-hvm.json   | 89 ++++++++++++++++++++++
>>   tests/libxlxml2domconfigdata/cpu-shares-hvm.xml    | 39 ++++++++++
>>   tests/libxlxml2domconfigdata/moredevs-hvm.json     |  2 +-
>>   tests/libxlxml2domconfigdata/multiple-ip.json      |  2 +-
>>   .../libxlxml2domconfigdata/variable-clock-hvm.json |  2 +-
>>   tests/libxlxml2domconfigdata/vnuma-hvm.json        |  2 +-
>>   tests/libxlxml2domconfigtest.c                     |  1 +
>>   10 files changed, 138 insertions(+), 7 deletions(-)
>>
> 
> Perhaps this is a "news" worthy type change along with the caveat about
> those more weighty domains that were allowed to feed at the trough
> without being limited.

I've sent a patch to add a news entry, including a short description of the 
potential problem + mitigation

https://www.redhat.com/archives/libvir-list/2018-March/msg01336.html

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list