| Message ID | 1362435597-20018-3-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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.
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.
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 >
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
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
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 >
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; }
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- qga/commands-posix.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 87 insertions(+), 0 deletions(-)