[1/2] xen/sched: fix locking in restore_vcpu_affinity()
diff mbox series

Message ID 20190723092056.15045-2-jgross@suse.com
State New
Headers show
Series
  • xen: fix/enhance temporary vcpu pinning
Related show

Commit Message

Jürgen Groß July 23, 2019, 9:20 a.m. UTC
Commit 0763cd2687897b55e7 ("xen/sched: don't disable scheduler on cpus
during suspend") removed a lock in restore_vcpu_affinity() which needs
to stay: cpumask_scratch_cpu() must be protected by the scheduler
lock. restore_vcpu_affinity() is being called by thaw_domains(), so
with multiple domains in the system another domain might already be
running and the scheduler might make use of cpumask_scratch_cpu()
already.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/schedule.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Dario Faggioli July 23, 2019, 10:55 a.m. UTC | #1
On Tue, 2019-07-23 at 11:20 +0200, Juergen Gross wrote:
> Commit 0763cd2687897b55e7 ("xen/sched: don't disable scheduler on
> cpus
> during suspend") removed a lock in restore_vcpu_affinity() which
> needs
> to stay: cpumask_scratch_cpu() must be protected by the scheduler
> lock. 
>
And indeed I recall the thing looked weird, and bringing it up... but
then I also managed to convince myself that it wasn't a problem, which
apparently is. :-(

Sorry for having overlooked that.

> restore_vcpu_affinity() is being called by thaw_domains(), so
> with multiple domains in the system another domain might already be
> running and the scheduler might make use of cpumask_scratch_cpu()
> already.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Thanks and Regards
Jan Beulich July 23, 2019, 12:08 p.m. UTC | #2
On 23.07.2019 11:20, Juergen Gross wrote:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -708,6 +708,8 @@ void restore_vcpu_affinity(struct domain *d)
>            * set v->processor of each of their vCPUs to something that will
>            * make sense for the scheduler of the cpupool in which they are in.
>            */
> +        lock = vcpu_schedule_lock_irq(v);
> +
>           cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
>                       cpupool_domain_cpumask(d));
>           if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
> @@ -731,6 +733,9 @@ void restore_vcpu_affinity(struct domain *d)
>   
>           v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
>   
> +        spin_unlock_irq(lock);
> +
> +	/* v->processor might have changed, so reacquire the lock. */

Whoever commits this (perhaps me, later today) will want to replace
the hard tab here.

Jan
Jürgen Groß July 23, 2019, 12:10 p.m. UTC | #3
On 23.07.19 14:08, Jan Beulich wrote:
> On 23.07.2019 11:20, Juergen Gross wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -708,6 +708,8 @@ void restore_vcpu_affinity(struct domain *d)
>>             * set v->processor of each of their vCPUs to something that will
>>             * make sense for the scheduler of the cpupool in which they are in.
>>             */
>> +        lock = vcpu_schedule_lock_irq(v);
>> +
>>            cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
>>                        cpupool_domain_cpumask(d));
>>            if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
>> @@ -731,6 +733,9 @@ void restore_vcpu_affinity(struct domain *d)
>>    
>>            v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
>>    
>> +        spin_unlock_irq(lock);
>> +
>> +	/* v->processor might have changed, so reacquire the lock. */
> 
> Whoever commits this (perhaps me, later today) will want to replace
> the hard tab here.

Oh, I'm sorry for that!

We really want checkpatch in Xen!


Juergen
Andrew Cooper July 23, 2019, 12:50 p.m. UTC | #4
On 23/07/2019 13:08, Jan Beulich wrote:
> On 23.07.2019 11:20, Juergen Gross wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -708,6 +708,8 @@ void restore_vcpu_affinity(struct domain *d)
>>            * set v->processor of each of their vCPUs to something that will
>>            * make sense for the scheduler of the cpupool in which they are in.
>>            */
>> +        lock = vcpu_schedule_lock_irq(v);
>> +
>>           cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
>>                       cpupool_domain_cpumask(d));
>>           if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
>> @@ -731,6 +733,9 @@ void restore_vcpu_affinity(struct domain *d)
>>   
>>           v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
>>   
>> +        spin_unlock_irq(lock);
>> +
>> +	/* v->processor might have changed, so reacquire the lock. */
> Whoever commits this (perhaps me, later today) will want to replace
> the hard tab here.

I've already committed this, and did fix up the tab.

~Andrew

Patch
diff mbox series

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 25f6ab388d..89bc259ae4 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -708,6 +708,8 @@  void restore_vcpu_affinity(struct domain *d)
          * set v->processor of each of their vCPUs to something that will
          * make sense for the scheduler of the cpupool in which they are in.
          */
+        lock = vcpu_schedule_lock_irq(v);
+
         cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
                     cpupool_domain_cpumask(d));
         if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
@@ -731,6 +733,9 @@  void restore_vcpu_affinity(struct domain *d)
 
         v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
 
+        spin_unlock_irq(lock);
+
+	/* v->processor might have changed, so reacquire the lock. */
         lock = vcpu_schedule_lock_irq(v);
         v->processor = sched_pick_cpu(vcpu_scheduler(v), v);
         spin_unlock_irq(lock);