Re: [Xen-devel] [PATCH v2] schedule: move last_run_time to the credit scheduler privates

[Andrii Anisov <andrii_anisov@xxxxxxxx>]

On 12.09.18 20:27, Dario Faggioli wrote:
> On Wed, 2018-09-12 at 17:47 +0300, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> >
> > The structure member last_run_time is used by credit scheduler only.
> > So move it from a generic vcpu sctructure to the credit scheduler
> > private
> > vcpu definition.
> >
> > Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> > ---
> > Changes in v2:
> >      - last_run_time type changed to s_time_t
> >      - scurr changed to svc
> >      - dropped stray blanks
> >      - pointers to const are used appropriately
> Well, the fact that the type is changing, and that you're also
> fixing/improving const-ness, should IMO at least be quickly mentioned
> in the changelog.
>
> Something like:
> "While there, also turn it into s_time_t, which is more appropriate.
> Also, properly const-ify one of the argument of
> __csched_vcpu_is_cache_hot()"
OK.

> And I was right about to write that I would also like the changelog
> itself to contain the usual sentence:
> "No functional change intended"
>
> but then I saw this (sorry for not noticing it in v1):
>
> > @@ -1869,6 +1874,7 @@ csched_schedule(
> >           /* Update credits of a non-idle VCPU. */
> >           burn_credits(scurr, now);
> >           scurr->start_time -= now;
In the first (internal) version of the patch this line was commented as 
"No matter what happens next with the current VCPU, we are safe to set 
its last run time here"
> > +        scurr->last_run_time = now;
> >       }
> >       else
> >       {
> > diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> > index 05281d6..3c299ca 100644
> > @@ -1556,7 +1556,6 @@ static void schedule(void)
> >           ((prev->pause_flags & VPF_blocked) ? RUNSTATE_blocked :
> >            (vcpu_runnable(prev) ? RUNSTATE_runnable :
> > RUNSTATE_offline)),
> >           now);
> > -    prev->last_run_time = now;
> >   
> >       ASSERT(next->runstate.state != RUNSTATE_running);
> >       vcpu_runstate_change(next, RUNSTATE_running, now);
> Basically, right now last_run_time is updated only when, in
> csched_schedule() we chose a different vcpu than the one which was
> running, and it also was updated for the idle vcpu.
> With the patch, it looks to me that it is updated even when we continue
> to run the same vcpu,
Right you are. But while it is used for vcpu cache "temperature" 
measurement only, it is still OK. Scheduling parameters and statistics 
are tracked by other entities.
>   and is never updated for the idle vcpu.
Yep, but it has no meaning for the idle vcpu. It never migrates.

> Now, considering how the info it contains is used currently, I'd say
> that not updating the field for the idle vcpu is not a problem.
>
> The fact that we _always_ update it for non-idle vcpus means we're
> changing what the field contains for running vcpus. Which again does
> not look to matter much right now.
>
> Do you agree with this analysis?
Sure :)

> If yes, well, I think this is bearable. Or, at least, that the benefit
> of having the logic self-contained in Credit code, overweight this
> slight loss of coherency between the name of the field and its content
> (things remains well within Credit's standards wrt to time accounting!
> :-/).
>
> But I'd add a few words about all this either in the changelog, and
> probably also in a comment (even a very short one, like "last_run_time
> is only meaningful for non running tasks" close to where you update it,
> or something like that).
OK.

--

*Andrii Anisov*



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