Re: [PATCH] x86/S3: Fix cpu pool scheduling after suspend/resume (v3)

["Jürgen Groß" <juergen.gross@ts.fujitsu.com>]

Am 17.04.2013 23:16, schrieb Ben Guthro:
> This review is another S3 scheduler problem with the system_state variable introduced with the following changeset:
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=269f543ea750ed567d18f2e819e5d5ce58eda5c5
>
> Specifically, the cpu_callback function that takes the CPU down during suspend, and back up during resume.
> We were seeing situations where, after S3, only CPU0 was in cpupool0. Guest performance suffered greatly, since all vcpus were only on a single pcpu. Guests under high CPU load showed the problem much more quickly than an idle guest.
>
> Removing this if condition forces the CPUs to go through the expected online/offline state, and be properly scheduled after S3.
>
> This also includes a necessary partial change proposed earlier by Tomasz Wroblewski here:
> http://lists.xen.org/archives/html/xen-devel/2013-01/msg02206.html
>
> It should also resolve the issues discussed in this thread:
> http://lists.xen.org/archives/html/xen-devel/2012-11/msg01801.html
>
> v2:
> Address concerns from Juergen Gross about the cpus not being put back into the pool they were in prior to suspend
>
> v3:
> Addressed leak of cpu_suspended, clean up hard tabs
>
> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>

Not tested, as I'm on vacation, but looks okay, so:

Acked-by: Juergen Gross <juergen.gross@ts.fujitsu.com>

> ---
>   xen/common/cpupool.c       |   57 ++++++++++++++++++++++++++++++++++----------
>   xen/include/xen/sched-if.h |    1 +
>   2 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 10b10f8..6b5a3db 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -40,17 +40,31 @@ DEFINE_PER_CPU(struct cpupool *, cpupool);
>   static struct cpupool *alloc_cpupool_struct(void)
>   {
>       struct cpupool *c = xzalloc(struct cpupool);
> +    if ( !c )
> +        return NULL;
>
> -    if ( c && zalloc_cpumask_var(&c->cpu_valid) )
> -        return c;
> -    xfree(c);
> -    return NULL;
> +    if ( !zalloc_cpumask_var(&c->cpu_suspended) )
> +    {
> +        xfree(c);
> +        return NULL;
> +    }
> +
> +    if ( !zalloc_cpumask_var(&c->cpu_valid) )
> +    {
> +        free_cpumask_var(c->cpu_suspended);
> +        xfree(c);
> +        return NULL;
> +    }
> +
> +    return c;
>   }
>
>   static void free_cpupool_struct(struct cpupool *c)
>   {
> -    if ( c )
> +    if ( c ) {
> +        free_cpumask_var(c->cpu_suspended);
>           free_cpumask_var(c->cpu_valid);
> +    }
>       xfree(c);
>   }
>
> @@ -417,14 +431,32 @@ void cpupool_rm_domain(struct domain *d)
>
>   /*
>    * called to add a new cpu to pool admin
> - * we add a hotplugged cpu to the cpupool0 to be able to add it to dom0
> + * we add a hotplugged cpu to the cpupool0 to be able to add it to dom0,
> + * unless we are resuming from S3, in which case we put the cpu back
> + * in the cpupool it was in prior to suspend.
>    */
>   static void cpupool_cpu_add(unsigned int cpu)
>   {
> +    struct cpupool **c;
>       spin_lock(&cpupool_lock);
> +
>       cpumask_clear_cpu(cpu, &cpupool_locked_cpus);
>       cpumask_set_cpu(cpu, &cpupool_free_cpus);
> -    cpupool_assign_cpu_locked(cpupool0, cpu);
> +
> +    if ( system_state == SYS_STATE_resume )
> +    {
> +        for_each_cpupool(c)
> +        {
> +            if ( cpumask_test_cpu(cpu, (*c)->cpu_suspended ) )
> +            {
> +                cpupool_assign_cpu_locked((*c), cpu);
> +                cpumask_clear_cpu(cpu, (*c)->cpu_suspended);
> +            }
> +        }
> +    }
> +
> +    if ( cpumask_test_cpu(cpu, &cpupool_free_cpus) )
> +        cpupool_assign_cpu_locked(cpupool0, cpu);
>       spin_unlock(&cpupool_lock);
>   }
>
> @@ -436,7 +468,7 @@ static void cpupool_cpu_add(unsigned int cpu)
>   static int cpupool_cpu_remove(unsigned int cpu)
>   {
>       int ret = 0;
> -	
> +
>       spin_lock(&cpupool_lock);
>       if ( !cpumask_test_cpu(cpu, cpupool0->cpu_valid))
>           ret = -EBUSY;
> @@ -630,12 +662,14 @@ void dump_runq(unsigned char key)
>   static int cpu_callback(
>       struct notifier_block *nfb, unsigned long action, void *hcpu)
>   {
> +    struct cpupool **c;
>       unsigned int cpu = (unsigned long)hcpu;
>       int rc = 0;
>
> -    if ( (system_state == SYS_STATE_suspend) ||
> -         (system_state == SYS_STATE_resume) )
> -        goto out;
> +    if ( system_state == SYS_STATE_suspend )
> +        for_each_cpupool(c)
> +            if ( cpumask_test_cpu(cpu, (*c)->cpu_valid ) )
> +                cpumask_set_cpu(cpu, (*c)->cpu_suspended);
>
>       switch ( action )
>       {
> @@ -650,7 +684,6 @@ static int cpu_callback(
>           break;
>       }
>
> -out:
>       return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>   }
>
> diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
> index 9ace22c..84bb13f 100644
> --- a/xen/include/xen/sched-if.h
> +++ b/xen/include/xen/sched-if.h
> @@ -201,6 +201,7 @@ struct cpupool
>   {
>       int              cpupool_id;
>       cpumask_var_t    cpu_valid;      /* all cpus assigned to pool */
> +    cpumask_var_t    cpu_suspended;  /* cpus in S3 that should be in this pool */
>       struct cpupool   *next;
>       unsigned int     n_dom;
>       struct scheduler *sched;
>