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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

On Fri, 2019-05-31 at 10:26 +0000, George Dunlap wrote:
> > On May 30, 2019, at 3:15 PM, Andrii Anisov <andrii.anisov@xxxxxxxxx
> > > 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 structure to the credit scheduler
> > private
> > vcpu definition.
> 
> This seems like a useful change, and the commit message has a lot of
> good detail, thanks.  But I’m left wondering: Is the main idea here
> just to generally reduce code and data usage when not running the
> credit scheduler, or is there another reason?
> 
Yeah, I also think this change is a good one to have.

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.

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.

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.

Like, e.g., in csched_schedule(), we first set it to 0, and then we
update it with `now` for `prev` if `prev != next && !is_idle(prev)` (or
something like that)

The rest of the patch looks fine to me.

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

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