[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:
> >From 950424a5e6a481dd1b2b4fb2645e7a852c199b7f Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Mon, 13 May 2013 11:55:42 -0400
> Subject: [PATCH] piix4acpi, xen, hotplug: Fix race with ACPI AML code and
>  hotplug.
> 
> 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.
> 
> [v1: Use stack for the 'attr' instead of malloc/free]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>


>  xenstore.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/xenstore.c b/xenstore.c
> index c861d36..b0d6f77 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -22,6 +22,7 @@
>  #include "pci.h"
>  #include "qemu-timer.h"
>  #include "qemu-xen.h"
> +#include "xen_backend.h"
> 
>  struct xs_handle *xsh = NULL;
>  static char *media_filename[MAX_DRIVES+1];
> @@ -999,25 +1000,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 +1028,61 @@ 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 get to vCPU that is in the event.
> +     */
> +    rc = xenstore_process_one_vcpu_set_event(path);
> +    if (rc > 0)
> +        changed = 1;
> +    /*
> +     * 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[XEN_BUFSIZE];
> +
> +        /* Construct "/local/domain/<domid>/cpu" (path) with <vcpu> (attr),
> +         * and "availability" with '/' sprinkled around. */
> +        snprintf(attr, XEN_BUFSIZE, "%s/%s/%s", path, dir[i],  
> "availability");
> +        rc = xenstore_process_one_vcpu_set_event(attr);
> +
> +        if (rc > 0)
> +            changed = 1;
> +        if (rc < 0) /* say xs_read failed */
> +            break;
> +    }
> +    free (dir);
> +    if (changed > 0) {
> +        fprintf(stderr, "Notifying OS about CPU hotplug changes.\n");
>          qemu_cpu_notify();
> +    }
>      return;
>  }


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.