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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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()"

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;
> +        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, and is never updated for the idle vcpu.

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?

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).

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