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

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

On 31.05.19 16:24, Dario Faggioli wrote:
> Weather or not the main reason is that one, it fixes an (albeit not too
> terrible) layering/encapsulation violation, as things used only by
> Credit, should live in Credit code.
Encapsulation violation was the main reason to have this patch though ;)

> It also makes (although only slightly) `struct vcpu` smaller, if one
> doesn't use Credit at all.
>
> But sure, let's have just a few more words about the motivation for the
> change in the commit message, as George is asking.
>
> > If it’s the first, a quick note to that effect will help put
> > archaeologist’s minds at ease. :-)  This could probably be added on
> > commit.  (I’ll do a full review it in a day or two if Dario doesn’t
> > beat me to it.)
> I've looked at it, and there's only one thing that bothers me a little
> bit. In fact, here:
>
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -175,6 +175,9 @@ struct csched_vcpu {
>       atomic_t credit;
>       unsigned int residual;
>   
> +    /* last time when vCPU is scheduled out */
> +    s_time_t last_run_time;
> +
>   #ifdef CSCHED_STATS
>       struct {
>           int credit_last;
>
> The comment is not accurate. And I'm afraid that it could be misleading
> for someone reading it, but then realizing that the code does something
> slightly different, and hence not being able to tell which one of the
> two things is correct.
Well, I copy-pasted that. And was wrong here. Me actually against the text 
comments inlined into the code. It happens that code changes faster than 
comments and in result comments become misleading very often.
I'd rather drop the comment at all.

> So, either we change the comment (but I'm not capable, right now, of
> finding something that is short and, at the same time, clear enough),
> or we change how the variable is using.
As per the current code I'd rename the member to `last_sched_time`. To reflect 
that it is the time when the vcpu went through scheduling path.


--
Sincerely,
Andrii Anisov.

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