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

[Ben Guthro <Ben.Guthro@xxxxxxxxxx>]

This had the wrong description text pasted from my last patch submission last 
week. 

Apologies. 

The code is sound. 

On Apr 17, 2013, at 5:16 PM, "Ben Guthro" <Ben.Guthro@xxxxxxxxxx> wrote:

> 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@xxxxxxxxxx>
> ---
> 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;
> -- 
> 1.7.9.5
> 

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