Re: [Xen-devel] [PATCH 2/2] xen/sched: don't disable scheduler on cpus during suspend

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Sat, 2019-03-16 at 14:14 +0100, Juergen Gross wrote:
> On 16/03/2019 04:01, Dario Faggioli wrote:

> > > So we can just omit the call of cpu_disable_scheduler() when
> > > offlining
> > > a cpu due to suspend and on resuming we can omit taking the
> > > schedule
> > > lock for selecting the new processor.
> > > 
> > Well, in theory, we should hold the lock when for using
> > 'cpumask_scratch_cpu(cpu)'. Not sure this is an actual problem in
> > this
> > case, but still...
> 
> I think there is no problem as there is no scheduling activity.
> 
I concur. I'm wondering whether it's worth adding a brief comment about
it. If it were me doing this, I most likely would.

> > > In restore_vcpu_affinity() we should be careful when applying
> > > affinity
> > > as the cpu might not have come back to life. 
> > > 
> > But restore_vcpu_affinity() is done, in a loop, for all vcpus of
> > all
> > domains, in thaw_domains(), which in turn comes after
> > enable_nonboot_cpus(), which I think is supposed to bring every CPU
> > back up.... What am I missing?
> 
> There is no guarantee all cpus will be up after suspend. Some might
> fail coming up.
> 
Ah, ok, that's the case we're trying to address. I didn't get it in the
first place.

And this, in fact, was what I was missing, and the reason why I was
finding some aspects of the patch a little weird. :-P

So, can we state this a bit better in the changelog, and also in a
comment? I'd like the reason why we break the affinity in a function
called "repair_affinity" to be more obvious. :-P

> > > diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> > > index e89bb67e71..7b5ce18426 100644
> > > --- a/xen/common/cpupool.c
> > > +++ b/xen/common/cpupool.c
> > > @@ -313,7 +313,7 @@ static long cpupool_unassign_cpu_helper(void
> > > *info)
> > >       * cpu_disable_scheduler(), and at the bottom of this
> > > function.
> > >       */
> > >      rcu_read_lock(&domlist_read_lock);
> > > -    ret = cpu_disable_scheduler(cpu);
> > > +    ret = (system_state == SYS_STATE_suspend) ? 0 :
> > > cpu_disable_scheduler(cpu);
> > > 
> > Mmm... How can this function be called with state = STATE_suspend ?
> 
> Meh, my bad. I masked the wrong call of cpu_disable_scheduler().
> Should
> have been that from __cpu_disable().
> 
Yes, that should be the one.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel