|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/3] piix4acpi, xen, hotplug: Fix race with ACPI AML code and hotplug.
On Mon, 13 May 2013, Konrad Rzeszutek Wilk wrote:
> This is a race so the amount varies but on a 4PCPU box
> I seem to get only ~14 out of 16 vCPUs I want to online.
>
> The issue at hand is that QEMU xenstore.c hotplug code changes
> the vCPU array and triggers an ACPI SCI for each vCPU
> online/offline change. That means we modify the array of vCPUs
> as the guests ACPI AML code is reading it - resulting in
> the guest reading the data only once and not changing the
> CPU states appropiately.
>
> The fix is to seperate the vCPU array changes from the ACPI SCI
> notification. The code now will enumerate all of the vCPUs
> and change the vCPU array if there is a need for a change.
> If a change did occur then only _one_ ACPI SCI pulse is sent
> to the guest. The vCPU array at that point has the online/offline
> modified to what the user wanted to have.
>
> Specifically, if a user provided this command:
> xl vcpu-set latest 16
>
> (guest config has vcpus=1, maxvcpus=32) QEMU and the guest
> (in this case Linux) would do:
>
> QEMU: Guest OS:
> -xenstore_process_vcpu_set_event
> -> Gets an XenBus notification for CPU1
> -> Updates the gpe_state.cpus_state bitfield.
> -> Pulses the ACPI SCI
> - ACPI SCI kicks in
>
> -> Gets an XenBus notification for CPU2
> -> Updates the gpe_state.cpus_state bitfield.
> -> Pulses the ACPI SCI
>
> -> Gets an XenBus notification for CPU3
> -> Updates the gpe_state.cpus_state bitfield.
> -> Pulses the ACPI SCI
> ...
> - Method(PRST) invoked
>
> -> Gets an XenBus notification for CPU12
> -> Updates the gpe_state.cpus_state bitfield.
> -> Pulses the ACPI SCI
> - reads AF00 for CPU state
> [gets 0xff]
> - reads AF02 [gets 0x7f]
>
> -> Gets an XenBus notification for CPU13
> -> Updates the gpe_state.cpus_state bitfield.
> -> Pulses the ACPI SCI
>
> .. until VCPU 16
> - Method PRST updates
> PR01 through 13 FLG
> entry.
> - PR01->PR13 _MAD
> invoked.
>
> - Brings up 13 CPUs.
>
> While QEMU updates the rest of the cpus_state bitfields the ACPI AML
> only does the CPU hotplug on those it had read.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
> xenstore.c | 68
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/xenstore.c b/xenstore.c
> index c861d36..31dbbe5 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -999,25 +999,24 @@ void xenstore_record_dm_state(const char *state)
> {
> xenstore_record_dm("state", state);
> }
> -
> -static void xenstore_process_vcpu_set_event(char **vec)
> +static int xenstore_process_one_vcpu_set_event(char *node)
> {
> char *act = NULL;
> - char *vcpustr, *node = vec[XS_WATCH_PATH];
> + char *vcpustr;
> unsigned int vcpu, len;
> int changed = -EINVAL;
>
> vcpustr = strstr(node, "cpu/");
> if (!vcpustr) {
> fprintf(stderr, "vcpu-set: watch node error.\n");
> - return;
> + return changed;
> }
> sscanf(vcpustr, "cpu/%u", &vcpu);
>
> act = xs_read(xsh, XBT_NULL, node, &len);
> if (!act) {
> fprintf(stderr, "vcpu-set: no command yet.\n");
> - return;
> + return changed;
> }
>
> if (!strncmp(act, "online", len))
> @@ -1028,8 +1027,65 @@ static void xenstore_process_vcpu_set_event(char **vec)
> fprintf(stderr, "vcpu-set: command error.\n");
>
> free(act);
> - if (changed > 0)
> + return changed;
> +}
> +static void xenstore_process_vcpu_set_event(char **vec)
> +{
> + int changed = 0, rc, i, num = 0;
> + char *vcpu, **dir;
> + char *path = vec[XS_WATCH_PATH];
> +
> + /*
> + * Process the event right away in case the loop below fails
> + * to enumerate the correct vCPU.
> + */
> + rc = xenstore_process_one_vcpu_set_event(path);
> + if (rc > 0)
> + changed = 1;
I am not sure I follow you here: why the loop below would fail?
> + /*
> + * We get: /local/domain/<domid>/cpu/<vcpu>/availability or
> + * (at init) /local/domain/<domid>/cpu [ignore it] and need to
> + * iterate over /local/domain/<domid>/cpu/ directory.
> + */
> + vcpu = strstr(path, "cpu/");
> + if (!vcpu) {
> + fprintf(stderr,"[%s]: %s has no CPU!\n", __func__, path);
> + return;
> + }
> + /* Eliminate '/availability' */
> + vcpu[3] = '\0';
> + dir = xs_directory(xsh, XBT_NULL, path, &num);
> +
> + if (!dir) {
> + fprintf(stderr, "[%s]: directory %s has no dirs!\n", __func__, path);
> + return;
> + }
> + if (num != vcpus)
> + fprintf(stderr, "[%s]: %d (number of vcpu entries) != %d (maxvcpus)!
> "\
> + "Continuing on..\n", __func__, num, vcpus);
> +
> + for (i = 0; i < num; i++) {
> + char *attr;
> + ssize_t len = strlen(path) + strlen(dir[i]) + strlen("availability")
> +
> + 3 /* account for two '/' and '\0'*/;
> + attr = malloc(len);
just use XEN_BUFSIZE and put in on the stack
> +
> + /* Construct "/local/domain/<domid>/cpu" (path) with <vcpu> (attr),
> + * and "availability" with '/' sprinkled around. */
> + snprintf(attr, len, "%s/%s/%s", path, dir[i], "availability");
> + rc = xenstore_process_one_vcpu_set_event(attr);
> +
> + free(attr);
> + if (rc > 0)
> + changed = 1;
> + if (rc < 0)
> + break;
Given that the user could provide a mask of vcpus to enable/disable,
shouldn't we just process them all anyway?
> + }
> + free (dir);
> + if (changed > 0) {
> + fprintf(stderr, "Notifying OS about CPU hotplug changes.\n");
> qemu_cpu_notify();
> + }
> return;
> }
>
> --
> 1.8.1.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |