Re: [Xen-devel] [PATCH v3 26/47] xen/sched: Change vcpu_migrate_*() to operate on schedule unit

[Jürgen Groß <jgross@xxxxxxxx>]

On 25.09.19 00:33, Dario Faggioli wrote:
> On Sat, 2019-09-14 at 10:52 +0200, Juergen Gross wrote:
> > Now that vcpu_migrate_start() and vcpu_migrate_finish() are used only
> > to ensure a vcpu is running on a suitable processor
> Is this sentence like this (I mean with that "Now" at the beginning)
> because it was --in previous versions of the series-- right after the
> patch that changed vcpu_force_reschedule()?
>
> If yes, then that is not the case any longer, so we may want to re-
> phrase.
Indeed.

> > they can be
> > switched to operate on schedule units instead of vcpus.
> >
> > While doing that rename them accordingly and make the _start()
> > variant
> > static.
> What does this mean/refer to?
>
> Wasn't vcpu_migrate_start() static already?
Wei cheated. He modified it with commit 9f8d606b4384408. :-)

> > As it is needed anyway call vcpu_sync_execstate() for each
> > vcpu of the unit when changing processors.
> Again, what do you mean with "As it is needed anyway"?
When moving from one cpu to another one the state must be saved in
struct vcpu (in contrast to being held partially in registers or on the
stack in case only the idle vcpu was scheduled afterwards on the old
cpu). This is done by vcpu_sync_execstate(). Without the explicit call
it would be done either when the vcpu is being scheduled on the new cpu
or if another non-idle vcpu is becoming active on the old cpu.

> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index 0bd9f0d278..70271cdea2 100644
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> >
> >   /*
> >    * Initiating migration
> >    *
> > - * In order to migrate, we need the vcpu in question to have stopped
> > + * In order to migrate, we need the unit in question to have stopped
> >    * running and had sched_sleep() called (to take it off any
> Might be me (not a native speaker), but this reads weird. "and have
> called sched_sleep()" or "and sched_sleep() to have been called" would
> sound better.
>
> It's, of course, not your fault, but I guess we can consider adjusting
> it, since we are touching the line above it.
>
> Or maybe you also want to queue it up for the cleanup series?
No, I'll change it.


Juergen

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