Patchwork [2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

login
register
mail settings
Submitter Laszlo Ersek
Date March 4, 2013, 10:19 p.m.
Message ID <1362435597-20018-3-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/224848/
State New
Headers show

Comments

Laszlo Ersek - March 4, 2013, 10:19 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)
Michael Roth - March 5, 2013, 8:03 p.m.
On Mon, Mar 04, 2013 at 11:19:56PM +0100, Laszlo Ersek wrote:
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1ad231a..d4b6bdc 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -15,6 +15,9 @@
>  #include <sys/types.h>
>  #include <sys/ioctl.h>
>  #include <sys/wait.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <stdio.h>
>  #include "qga/guest-agent-core.h"
>  #include "qga-qmp-commands.h"
>  #include "qapi/qmp/qerror.h"
> @@ -1083,9 +1086,93 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
>  }
>  #endif
> 
> +#if defined(__linux__)

There's a section in commands-posix.c set aside under "linux-specific
implementations" for these, and another underneath for stubs so we can
avoid having too many ifdef's.

> +#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
> +
> +static long sysconf_exact(int name, const char *name_str, Error **err)
> +{
> +    long ret;
> +
> +    errno = 0;
> +    ret = sysconf(name);
> +    if (ret == -1) {
> +        if (errno == 0) {
> +            error_setg(err, "sysconf(%s): value indefinite", name_str);
> +        } else {
> +            error_setg_errno(err, errno, "sysconf(%s)", name_str);
> +        }
> +    }
> +    return ret;
> +}
> +
> +/*
> + * Store a VCPU structure under the link, and return the link to store into
> + * at the next time.
> + */
> +static GuestLogicalProcessorList **
> +append_vcpu(int64_t logical_id, bool online, GuestLogicalProcessorList **link)
> +{
> +    GuestLogicalProcessor *vcpu;
> +    GuestLogicalProcessorList *entry;
> +
> +    vcpu = g_malloc0(sizeof *vcpu);
> +    vcpu->logical_id = logical_id;
> +    vcpu->online = online;
> +
> +    entry = g_malloc0(sizeof *entry);
> +    entry->value = vcpu;
> +
> +    *link = entry;
> +    return &entry->next;
> +}
> +#endif
> +
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> +#if defined(__linux__)
> +    long current;
> +    GuestLogicalProcessorList **link, *head;
> +    long sc_max;
> +    Error *local_err = NULL;
> +
> +    current = 0;
> +    link = append_vcpu(current++, true, &head);
> +
> +    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
> +    while (local_err == NULL && current < sc_max) {
> +        char *buf;
> +        FILE *f;
> +
> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> +                              current);
> +        f = fopen(buf, "r");
> +        if (f == NULL) {
> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
> +        } else {
> +            unsigned online;
> +
> +            if (fscanf(f, "%u", &online) != 1) {

On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
values for online/offline/etc, but instead just a 'global' entry at
/sys/devices/system/cpu/{online,offline} that provides a range. This is
what's currently described in
linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.

Is that file also available on the distro you're testing with? Hopefully
there's a single interfaces we can rely on.

> +                error_setg(&local_err, "failed to read or parse \"%s\"", buf);
> +            } else {
> +                link = append_vcpu(current++, online != 0, link);
> +            }
> +
> +            if (fclose(f) == EOF && local_err == NULL) {
> +                error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
> +            }
> +        }
> +        g_free(buf);
> +    }
> +
> +    if (local_err == NULL) {
> +        return head;
> +    }
> +
> +    qapi_free_GuestLogicalProcessorList(head);
> +    error_propagate(errp, local_err);
> +#else
>      error_set(errp, QERR_UNSUPPORTED);
> +#endif
>      return NULL;
>  }
> 
> -- 
> 1.7.1
> 
>
Eric Blake - March 5, 2013, 8:22 p.m.
On 03/05/2013 01:03 PM, mdroth wrote:

>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>> +        } else {
>> +            unsigned online;
>> +
>> +            if (fscanf(f, "%u", &online) != 1) {
> 
> On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
> values for online/offline/etc, but instead just a 'global' entry at
> /sys/devices/system/cpu/{online,offline} that provides a range. This is
> what's currently described in
> linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.

Actually, there is both.  Here's what I have on my 2-cpu laptop, running
Fedora 18:

# find /sys/devices/system/cpu/ -name online
/sys/devices/system/cpu/cpu1/online
/sys/devices/system/cpu/online

Notice that there is NO /sys/devices/system/cpu/cpu0/online, because
this particular laptop's chipset requires that cpu0 ALWAYS be online.
The per-cpu online file exists only for cpus that can safely be
offlined; if it does not exist, then you must leave that cpu on.

> 
> Is that file also available on the distro you're testing with? Hopefully
> there's a single interfaces we can rely on.

Libvirt also relies on the per-cpu online files, and hasn't had any
complaints across the distros.
Eric Blake - March 5, 2013, 8:25 p.m.
On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> +#if defined(__linux__)

> +
> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> +                              current);
> +        f = fopen(buf, "r");
> +        if (f == NULL) {
> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);

NACK to this portion.  If the file doesn't exist, but the
/sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
treated as always online, and not an error.  In fact, on many machines,
cpu0 does not have an online file precisely because it cannot be taken
offline, even if the rest of the cpus can.  It is also the case that on
older kernels that did not support offline cpus (such as RHEL 5), there
will be no per-cpu online file; but again, such kernels cannot support
hot unplug, so all per-cpu directories imply which cpus are online.  In
other words, you need a sane fallback if the online file does not exist
but the per-cpu directory does exist.
Michael Roth - March 5, 2013, 8:45 p.m.
On Tue, Mar 05, 2013 at 01:22:03PM -0700, Eric Blake wrote:
> On 03/05/2013 01:03 PM, mdroth wrote:
> 
> >> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> >> +                              current);
> >> +        f = fopen(buf, "r");
> >> +        if (f == NULL) {
> >> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
> >> +        } else {
> >> +            unsigned online;
> >> +
> >> +            if (fscanf(f, "%u", &online) != 1) {
> > 
> > On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
> > values for online/offline/etc, but instead just a 'global' entry at
> > /sys/devices/system/cpu/{online,offline} that provides a range. This is
> > what's currently described in
> > linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.
> 
> Actually, there is both.  Here's what I have on my 2-cpu laptop, running
> Fedora 18:
> 
> # find /sys/devices/system/cpu/ -name online
> /sys/devices/system/cpu/cpu1/online
> /sys/devices/system/cpu/online
> 
> Notice that there is NO /sys/devices/system/cpu/cpu0/online, because

Ahh, didn't think to check the others. This seems to be the case for me
as well. I agree on your later note about special casing this though.

> this particular laptop's chipset requires that cpu0 ALWAYS be online.
> The per-cpu online file exists only for cpus that can safely be
> offlined; if it does not exist, then you must leave that cpu on.
> 
> > 
> > Is that file also available on the distro you're testing with? Hopefully
> > there's a single interfaces we can rely on.
> 
> Libvirt also relies on the per-cpu online files, and hasn't had any
> complaints across the distros.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Laszlo Ersek - March 5, 2013, 9:05 p.m.
On 03/05/13 21:03, mdroth wrote:> On Mon, Mar 04, 2013 at 11:19:56PM +0100, Laszlo Ersek wrote:

>> +#if defined(__linux__)
>
> There's a section in commands-posix.c set aside under "linux-specific
> implementations" for these, and another underneath for stubs so we can
> avoid having too many ifdef's.

I'll check it, thanks.

>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>> +        } else {
>> +            unsigned online;
>> +
>> +            if (fscanf(f, "%u", &online) != 1) {
>
> On Fedora 18 and Ubuntu 12.04 at least there doesn't seem to be per-cpu
> values for online/offline/etc, but instead just a 'global' entry at
> /sys/devices/system/cpu/{online,offline} that provides a range. This is
> what's currently described in
> linux/Documentation/ABI/testing/sysfs-devices-system-cpu as well.
>
> Is that file also available on the distro you're testing with? Hopefully
> there's a single interfaces we can rely on.

On RHEL-6, both "/sys/devices/system/cpu/cpu*/online" and
"/sys/devices/system/cpu/online " are available.

On RHEL-5, only "/sys/devices/system/cpu/cpu*/online" is available.

"/sys/devices/system/cpu/cpu*/online" is documented in
"Documentation/cpu-hotplug.txt", in all of RHEL-5, RHEL-6, and upstream
Linux.

            #pwd
            #/sys/devices/system/cpu
            #ls -l
            total 0
            drwxr-xr-x  10 root root 0 Sep 19 07:44 .
            drwxr-xr-x  13 root root 0 Sep 19 07:45 ..
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu0
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu1
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu2
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu3
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu4
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu5
            drwxr-xr-x   3 root root 0 Sep 19 07:44 cpu6
            drwxr-xr-x   3 root root 0 Sep 19 07:48 cpu7

    Under each directory you would find an "online" file which is the control
    file to logically online/offline a processor.

    Q: Does hot-add/hot-remove refer to physical add/remove of cpus?
    A: The usage of hot-add/remove may not be very consistently used in the code.
    CONFIG_HOTPLUG_CPU enables logical online/offline capability in the kernel.
    To support physical addition/removal, one would need some BIOS hooks and
    the platform should have something like an attention button in PCI hotplug.
    CONFIG_ACPI_HOTPLUG_CPU enables ACPI support for physical add/remove of CPUs.

The kernels you mention may not have CONFIG_HOTPLUG_CPU enabled
(consequently they would probably not support the functionality either).

... I just checked "/boot/config-3.8.2-201.fc18.x86_64" in
"kernel-3.8.2-201.fc18.x86_64.rpm" and all required config options seem
to be set. I checked on a much older F18 guest as well
(3.6.10-4.fc18.x86_64), and the per-cpu "online" files are there. (Not
for cpu0, but I'll address that in response to Eric's review.) I'm not
sure why you don't get them under F18.

Thanks
Laszlo
Laszlo Ersek - March 5, 2013, 9:34 p.m.
On 03/05/13 21:25, Eric Blake wrote:

> On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>  {
>> +#if defined(__linux__)
>
>> +
>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>
> NACK to this portion.  If the file doesn't exist, but the
> /sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
> treated as always online, and not an error.  In fact, on many machines,
> cpu0 does not have an online file precisely because it cannot be taken
> offline, even if the rest of the cpus can.

The code always reports cpu0 online (without looking at the sysfs) and
never attempts to remove it (refuses any such requests), but see below.

> It is also the case that on
> older kernels that did not support offline cpus (such as RHEL 5), there
> will be no per-cpu online file; but again, such kernels cannot support
> hot unplug, so all per-cpu directories imply which cpus are online.  In
> other words, you need a sane fallback if the online file does not exist
> but the per-cpu directory does exist.

In upstream "Documentation/cpu-hotplug.txt", there's

    Q: Why can't i remove CPU0 on some systems?
    A: Some architectures may have some special dependency on a certain
    CPU.

    For e.g in IA64 platforms we have ability to sent platform
    interrupts to the OS. a.k.a Corrected Platform Error Interrupts
    (CPEI). In current ACPI specifications, we didn't have a way to
    change the target CPU. Hence if the current ACPI version doesn't
    support such re-direction, we disable that CPU by making it
    not-removable.

    In such cases you will also notice that the online file is missing
    under cpu0.

    Q: Is CPU0 removable on X86?
    A: Yes. If kernel is compiled with CONFIG_BOOTPARAM_HOTPLUG_CPU0=y,
    CPU0 is removable by default. Otherwise, CPU0 is also removable by
    kernel option cpu0_hotplug.

    But some features depend on CPU0. Two known dependencies are:

    1. Resume from hibernate/suspend depends on CPU0. Hibernate/suspend
    will fail if CPU0 is offline and you need to online CPU0 before
    hibernate/suspend can continue.
    2. PIC interrupts also depend on CPU0. CPU0 can't be removed if a
    PIC interrupt is detected.

    It's said poweroff/reboot may depend on CPU0 on some machines
    although I haven't seen any poweroff/reboot failure so far after
    CPU0 is offline on a few tested machines.

    Please let me know if you know or see any other dependencies of
    CPU0.

    If the dependencies are under your control, you can turn on CPU0
    hotplug feature either by CONFIG_BOOTPARAM_HOTPLUG_CPU0 or by kernel
    parameter cpu0_hotplug.

    --Fenghua Yu <fenghua.yu@intel.com>

I got this wrong in the series. The get method always reports an online
CPU#0 (without even looking at sysfs), plus 'set' always refuses an
attempt to offline it (even if the guest would support it; ie. 'set' too
omits looking at the sysfs for cpu#0). This matches the common specific
case (exactly cpu#0 is fixed online), but it is wrong in general.

I think I should introduce another boolean flag in the element structure
(only for the get method) that would say whether you can attempt to
offline the VCPU. Then the caller of the set method can either comply or
ignore the hint, and in the latter case it would be smacked down
rightfully.

Thanks!
Laszlo
Michael Roth - March 5, 2013, 10:06 p.m.
On Tue, Mar 05, 2013 at 10:34:08PM +0100, Laszlo Ersek wrote:
> On 03/05/13 21:25, Eric Blake wrote:
> 
> > On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
> >>  {
> >> +#if defined(__linux__)
> >
> >> +
> >> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
> >> +                              current);
> >> +        f = fopen(buf, "r");
> >> +        if (f == NULL) {
> >> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
> >
> > NACK to this portion.  If the file doesn't exist, but the
> > /sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
> > treated as always online, and not an error.  In fact, on many machines,
> > cpu0 does not have an online file precisely because it cannot be taken
> > offline, even if the rest of the cpus can.
> 
> The code always reports cpu0 online (without looking at the sysfs) and
> never attempts to remove it (refuses any such requests), but see below.
> 
> > It is also the case that on
> > older kernels that did not support offline cpus (such as RHEL 5), there
> > will be no per-cpu online file; but again, such kernels cannot support
> > hot unplug, so all per-cpu directories imply which cpus are online.  In
> > other words, you need a sane fallback if the online file does not exist
> > but the per-cpu directory does exist.
> 
> In upstream "Documentation/cpu-hotplug.txt", there's
> 
>     Q: Why can't i remove CPU0 on some systems?
>     A: Some architectures may have some special dependency on a certain
>     CPU.
> 
>     For e.g in IA64 platforms we have ability to sent platform
>     interrupts to the OS. a.k.a Corrected Platform Error Interrupts
>     (CPEI). In current ACPI specifications, we didn't have a way to
>     change the target CPU. Hence if the current ACPI version doesn't
>     support such re-direction, we disable that CPU by making it
>     not-removable.
> 
>     In such cases you will also notice that the online file is missing
>     under cpu0.
> 
>     Q: Is CPU0 removable on X86?
>     A: Yes. If kernel is compiled with CONFIG_BOOTPARAM_HOTPLUG_CPU0=y,
>     CPU0 is removable by default. Otherwise, CPU0 is also removable by
>     kernel option cpu0_hotplug.
> 
>     But some features depend on CPU0. Two known dependencies are:
> 
>     1. Resume from hibernate/suspend depends on CPU0. Hibernate/suspend
>     will fail if CPU0 is offline and you need to online CPU0 before
>     hibernate/suspend can continue.
>     2. PIC interrupts also depend on CPU0. CPU0 can't be removed if a
>     PIC interrupt is detected.
> 
>     It's said poweroff/reboot may depend on CPU0 on some machines
>     although I haven't seen any poweroff/reboot failure so far after
>     CPU0 is offline on a few tested machines.
> 
>     Please let me know if you know or see any other dependencies of
>     CPU0.
> 
>     If the dependencies are under your control, you can turn on CPU0
>     hotplug feature either by CONFIG_BOOTPARAM_HOTPLUG_CPU0 or by kernel
>     parameter cpu0_hotplug.
> 
>     --Fenghua Yu <fenghua.yu@intel.com>
> 
> I got this wrong in the series. The get method always reports an online
> CPU#0 (without even looking at sysfs), plus 'set' always refuses an
> attempt to offline it (even if the guest would support it; ie. 'set' too
> omits looking at the sysfs for cpu#0). This matches the common specific
> case (exactly cpu#0 is fixed online), but it is wrong in general.
> 
> I think I should introduce another boolean flag in the element structure
> (only for the get method) that would say whether you can attempt to
> offline the VCPU. Then the caller of the set method can either comply or
> ignore the hint, and in the latter case it would be smacked down
> rightfully.

I think that seems reasonable.

> 
> Thanks!
> Laszlo
>

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1ad231a..d4b6bdc 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -15,6 +15,9 @@ 
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdio.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
@@ -1083,9 +1086,93 @@  void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
+#if defined(__linux__)
+#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
+
+static long sysconf_exact(int name, const char *name_str, Error **err)
+{
+    long ret;
+
+    errno = 0;
+    ret = sysconf(name);
+    if (ret == -1) {
+        if (errno == 0) {
+            error_setg(err, "sysconf(%s): value indefinite", name_str);
+        } else {
+            error_setg_errno(err, errno, "sysconf(%s)", name_str);
+        }
+    }
+    return ret;
+}
+
+/*
+ * Store a VCPU structure under the link, and return the link to store into
+ * at the next time.
+ */
+static GuestLogicalProcessorList **
+append_vcpu(int64_t logical_id, bool online, GuestLogicalProcessorList **link)
+{
+    GuestLogicalProcessor *vcpu;
+    GuestLogicalProcessorList *entry;
+
+    vcpu = g_malloc0(sizeof *vcpu);
+    vcpu->logical_id = logical_id;
+    vcpu->online = online;
+
+    entry = g_malloc0(sizeof *entry);
+    entry->value = vcpu;
+
+    *link = entry;
+    return &entry->next;
+}
+#endif
+
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
+#if defined(__linux__)
+    long current;
+    GuestLogicalProcessorList **link, *head;
+    long sc_max;
+    Error *local_err = NULL;
+
+    current = 0;
+    link = append_vcpu(current++, true, &head);
+
+    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
+    while (local_err == NULL && current < sc_max) {
+        char *buf;
+        FILE *f;
+
+        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
+                              current);
+        f = fopen(buf, "r");
+        if (f == NULL) {
+            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
+        } else {
+            unsigned online;
+
+            if (fscanf(f, "%u", &online) != 1) {
+                error_setg(&local_err, "failed to read or parse \"%s\"", buf);
+            } else {
+                link = append_vcpu(current++, online != 0, link);
+            }
+
+            if (fclose(f) == EOF && local_err == NULL) {
+                error_setg_errno(&local_err, errno, "fclose(\"%s\")", buf);
+            }
+        }
+        g_free(buf);
+    }
+
+    if (local_err == NULL) {
+        return head;
+    }
+
+    qapi_free_GuestLogicalProcessorList(head);
+    error_propagate(errp, local_err);
+#else
     error_set(errp, QERR_UNSUPPORTED);
+#endif
     return NULL;
 }