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

Re: [libvirt] [PATCH RFC 3/3] qemu: Add support for paravirtual spinlocks in the guest



On Tue, Sep 24, 2013 at 3:43 AM, Peter Krempa <pkrempa redhat com> wrote:
> The linux kernel recently added support for paravirtual spinlock
> handling to avoid performance regressions on overcomitted hosts. This
> feature needs to be turned in the hypervisor so that the guest OS is
> notified about the possible support.
>
> This patch adds a new feature "paravirt-spinlock" to the XML and
> supporting code to enable the "kvm_pv_unhalt" pseudoCPU feature in qemu.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1008989
> ---
> This patch is RFC as qemu didn't add this to the upstream repo yet:
>
> Pull request:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg03385.html
>
> Original thread:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg03095.html
>
> I'm sending it to get review feedback but will push it only after qemu will
> have it.
>
>
>  docs/formatdomain.html.in                          |  7 ++++++
>  docs/schemas/domaincommon.rng                      | 10 ++++++++-
>  src/conf/domain_conf.c                             | 20 ++++++++++++++++-
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_command.c                            | 14 ++++++++++++
>  .../qemuxml2argv-pv-spinlock-disabled.args         |  5 +++++
>  .../qemuxml2argv-pv-spinlock-disabled.xml          | 26 ++++++++++++++++++++++
>  .../qemuxml2argv-pv-spinlock-enabled.args          |  5 +++++
>  .../qemuxml2argv-pv-spinlock-enabled.xml           | 26 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  2 ++
>  tests/qemuxml2xmltest.c                            |  2 ++
>  11 files changed, 116 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 3689399..aa0076c 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1182,6 +1182,7 @@
>        &lt;vapic state='on'/&gt;
>        &lt;spinlocks state='on' retries='4096'/&gt;
>      &lt;/hyperv&gt;
> +    &lt;paravirt-spinlock/&gt;
>
>    &lt;/features&gt;
>    ...</pre>
> @@ -1253,6 +1254,12 @@
>          </tr>
>        </table>
>        </dd>
> +      <dt><code>paravirtual-spinlock</code></dt>

You call it paravirt-spinlock everywhere else so this looks like a typo.

> +      <dd>Notify the guest that the host supports paravirtual spinlocks
> +          for example by exposing the pvticketlocks mechanism. This feature
> +          can be forced of by using a "state='off'" attribute.
> +      </dd>
> +
>      </dl>
>
>      <h3><a name="elementsTime">Time keeping</a></h3>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 94a72f8..4c494cb 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3560,7 +3560,7 @@
>    </define>
>    <!--
>        A set of optional features: PAE, APIC, ACPI,
> -      HyperV Enlightenment and HAP support
> +      HyperV Enlightenment, paravirtual spinlocks  and HAP support
>      -->
>    <define name="features">
>      <optional>
> @@ -3606,6 +3606,14 @@
>                <empty/>
>              </element>
>            </optional>
> +          <optional>
> +            <element name="paravirt-spinlock">
> +              <optional>
> +                <ref name="featurestate"/>
> +              </optional>
> +              <empty/>
> +            </element>
> +          </optional>
>          </interleave>
>        </element>
>      </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e1a1d6d..919adc1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -142,7 +142,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
>                "hap",
>                "viridian",
>                "privnet",
> -              "hyperv")
> +              "hyperv",
> +              "paravirt-spinlock")
>
>  VIR_ENUM_IMPL(virDomainFeatureState, VIR_DOMAIN_FEATURE_STATE_LAST,
>                "default",
> @@ -11394,6 +11395,22 @@ virDomainDefParseXML(xmlDocPtr xml,
>              def->features[val] = VIR_DOMAIN_FEATURE_STATE_ON;
>              break;
>
> +        case VIR_DOMAIN_FEATURE_PARAVIRT_SPINLOCK:
> +            node = ctxt->node;
> +            ctxt->node = nodes[i];
> +            if ((tmp = virXPathString("string(./@state)", ctxt))) {
> +                if ((def->features[val] = virDomainFeatureStateTypeFromString(tmp)) == -1) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                                   _("unknown state atribute '%s' of feature '%s'"),
> +                                   tmp, virDomainFeatureTypeToString(val));
> +                    goto error;
> +                }
> +            } else {
> +                def->features[val] = VIR_DOMAIN_FEATURE_STATE_ON;
> +            }
> +            ctxt->node = node;
> +            break;
> +
>          case VIR_DOMAIN_FEATURE_LAST:
>              break;
>          }
> @@ -16742,6 +16759,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>              case VIR_DOMAIN_FEATURE_HAP:
>              case VIR_DOMAIN_FEATURE_VIRIDIAN:
>              case VIR_DOMAIN_FEATURE_PRIVNET:
> +            case VIR_DOMAIN_FEATURE_PARAVIRT_SPINLOCK:
>                  switch ((enum virDomainFeatureState) def->features[i]) {
>                  case VIR_DOMAIN_FEATURE_STATE_ON:
>                      /* output just the element */
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 9ddd706..4d69c4f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1618,6 +1618,7 @@ enum virDomainFeature {
>      VIR_DOMAIN_FEATURE_VIRIDIAN,
>      VIR_DOMAIN_FEATURE_PRIVNET,
>      VIR_DOMAIN_FEATURE_HYPERV,
> +    VIR_DOMAIN_FEATURE_PARAVIRT_SPINLOCK,
>
>      VIR_DOMAIN_FEATURE_LAST
>  };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6f3b3cb..01de440 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6608,6 +6608,20 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver,
>          have_cpu = true;
>      }
>
> +    if (def->features[VIR_DOMAIN_FEATURE_PARAVIRT_SPINLOCK]) {
> +        char sign;
> +        if (def->features[VIR_DOMAIN_FEATURE_PARAVIRT_SPINLOCK] ==
> +            VIR_DOMAIN_FEATURE_STATE_ON)
> +            sign = '+';
> +        else
> +            sign = '-';
> +
> +        virBufferAsprintf(&buf, "%s,%ckvm_pv_unhalt",
> +                          have_cpu ? "" : default_model,
> +                          sign);
> +        have_cpu = true;
> +    }
> +
>      if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_DOMAIN_FEATURE_STATE_ON) {
>          if (!have_cpu) {
>              virBufferAdd(&buf, default_model, -1);
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args
> new file mode 100644
> index 0000000..80047f9
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args
> @@ -0,0 +1,5 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu -S -M pc \
> +-cpu qemu32,-kvm_pv_unhalt -m 214 -smp 6 -nographic -monitor \
> +unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial \
> +none -parallel none
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml
> new file mode 100644
> index 0000000..afb0b41
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml
> @@ -0,0 +1,26 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219100</memory>
> +  <currentMemory unit='KiB'>219100</currentMemory>
> +  <vcpu placement='static'>6</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='network'/>
> +  </os>
> +  <features>
> +    <acpi/>
> +    <pae/>
> +    <paravirt-spinlock state='off'/>
> +  </features>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args
> new file mode 100644
> index 0000000..70db173
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args
> @@ -0,0 +1,5 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu -S -M pc \
> +-cpu qemu32,+kvm_pv_unhalt -m 214 -smp 6 -nographic -monitor \
> +unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial \
> +none -parallel none
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml
> new file mode 100644
> index 0000000..f29cade
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml
> @@ -0,0 +1,26 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219100</memory>
> +  <currentMemory unit='KiB'>219100</currentMemory>
> +  <vcpu placement='static'>6</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='network'/>
> +  </os>
> +  <features>
> +    <acpi/>
> +    <pae/>
> +    <paravirt-spinlock/>
> +  </features>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index ec4a6e5..72b6b2a 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -435,6 +435,8 @@ mymain(void)
>              QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_SPICE, QEMU_CAPS_HDA_DUPLEX);
>      DO_TEST("eoi-disabled", NONE);
>      DO_TEST("eoi-enabled", NONE);
> +    DO_TEST("pv-spinlock-disabled", NONE);
> +    DO_TEST("pv-spinlock-enabled", NONE);
>      DO_TEST("kvmclock+eoi-disabled", QEMU_CAPS_ENABLE_KVM);
>
>      DO_TEST("hyperv", NONE);
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index c661573..0e83dcf 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -156,6 +156,8 @@ mymain(void)
>      DO_TEST("cpu-eoi-enabled");
>      DO_TEST("eoi-disabled");
>      DO_TEST("eoi-enabled");
> +    DO_TEST("pv-spinlock-disabled");
> +    DO_TEST("pv-spinlock-enabled");
>
>      DO_TEST("hyperv");
>      DO_TEST("hyperv-off");
> --
> 1.8.3.2
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Just one typo that in addition to Jan's review.

-- 
Doug Goldstein


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