[PATCH v3] schedule: move last_run_time to the credit scheduler privates

[PATCH v3] schedule: move last_run_time to the credit scheduler privates

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

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.
With this move we have slight changes in functionality:
 - last_run_time is not updated for an idle vcpu. But the idle vcpu is,
   in fact, a per-pcpu stub and never migrates so last_run_time is
   meaningless for it.
 - The value of last_run_time is updated on every schedule, even if the
   vcpu is not being changed. It is still ok, because last_run_time is
   only used for runnable vcpu migration decision, and we have it correct
   at that moment. Scheduling parameters and statistics are tracked by
   other entities.

While here, also:
  - turn last_run_time into s_time_t, which is more appropriate.
  - properly const-ify related argument of __csched_vcpu_is_cache_hot().

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---

Changes in
 v3:
    - commit message updated accordingly to [1]
 v2:
    - last_run_time type changed to s_time_t
    - scurr changed to svc
    - dropped stray blanks
    - pointers to const are used appropriately

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01201.html

---
 xen/common/sched_credit.c | 12 +++++++++---
 xen/common/schedule.c     |  1 -
 xen/include/xen/sched.h   |  3 ---
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 7b7facb..b097048 100644
--- 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;
@@ -701,10 +704,11 @@ static unsigned int vcpu_migration_delay_us;
 integer_param("vcpu_migration_delay", vcpu_migration_delay_us);
 
 static inline bool
-__csched_vcpu_is_cache_hot(const struct csched_private *prv, struct vcpu *v)
+__csched_vcpu_is_cache_hot(const struct csched_private *prv,
+                           const struct csched_vcpu *svc)
 {
     bool hot = prv->vcpu_migr_delay &&
-               (NOW() - v->last_run_time) < prv->vcpu_migr_delay;
+               (NOW() - svc->last_run_time) < prv->vcpu_migr_delay;
 
     if ( hot )
         SCHED_STAT_CRANK(vcpu_hot);
@@ -716,6 +720,7 @@ static inline int
 __csched_vcpu_is_migrateable(const struct csched_private *prv, struct vcpu *vc,
                              int dest_cpu, cpumask_t *mask)
 {
+    const struct csched_vcpu *svc = CSCHED_VCPU(vc);
     /*
      * Don't pick up work that's hot on peer PCPU, or that can't (or
      * would prefer not to) run on cpu.
@@ -725,7 +730,7 @@ __csched_vcpu_is_migrateable(const struct csched_private *prv, struct vcpu *vc,
      */
     ASSERT(!vc->is_running);
 
-    return !__csched_vcpu_is_cache_hot(prv, vc) &&
+    return !__csched_vcpu_is_cache_hot(prv, svc) &&
            cpumask_test_cpu(dest_cpu, mask);
 }
 
@@ -1870,6 +1875,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 66f1e26..4deacf6 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1493,7 +1493,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);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2201fac..7aa0be6 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -174,9 +174,6 @@ struct vcpu
     } runstate_guest; /* guest address */
 #endif
 
-    /* last time when vCPU is scheduled out */
-    uint64_t last_run_time;
-
     /* Has the FPU been initialised? */
     bool             fpu_initialised;
     /* Has the FPU been used since it was last saved? */
-- 
2.7.4


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

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

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

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.
With this move we have slight changes in functionality:
 - last_run_time is not updated for an idle vcpu. But the idle vcpu is,
   in fact, a per-pcpu stub and never migrates so last_run_time is
   meaningless for it.
 - The value of last_run_time is updated on every schedule, even if the
   vcpu is not being changed. It is still ok, because last_run_time is
   only used for runnable vcpu migration decision, and we have it correct
   at that moment. Scheduling parameters and statistics are tracked by
   other entities.

While here, also:
  - turn last_run_time into s_time_t, which is more appropriate.
  - properly const-ify related argument of __csched_vcpu_is_cache_hot().

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---

Changes in
 v3:
    - commit message updated accordingly to [1]
 v2:
    - last_run_time type changed to s_time_t
    - scurr changed to svc
    - dropped stray blanks
    - pointers to const are used appropriately

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01201.html

---
 xen/common/sched_credit.c | 12 +++++++++---
 xen/common/schedule.c     |  1 -
 xen/include/xen/sched.h   |  3 ---
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 7b7facb..b097048 100644
--- 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;
@@ -701,10 +704,11 @@ static unsigned int vcpu_migration_delay_us;
 integer_param("vcpu_migration_delay", vcpu_migration_delay_us);
 
 static inline bool
-__csched_vcpu_is_cache_hot(const struct csched_private *prv, struct vcpu *v)
+__csched_vcpu_is_cache_hot(const struct csched_private *prv,
+                           const struct csched_vcpu *svc)
 {
     bool hot = prv->vcpu_migr_delay &&
-               (NOW() - v->last_run_time) < prv->vcpu_migr_delay;
+               (NOW() - svc->last_run_time) < prv->vcpu_migr_delay;
 
     if ( hot )
         SCHED_STAT_CRANK(vcpu_hot);
@@ -716,6 +720,7 @@ static inline int
 __csched_vcpu_is_migrateable(const struct csched_private *prv, struct vcpu *vc,
                              int dest_cpu, cpumask_t *mask)
 {
+    const struct csched_vcpu *svc = CSCHED_VCPU(vc);
     /*
      * Don't pick up work that's hot on peer PCPU, or that can't (or
      * would prefer not to) run on cpu.
@@ -725,7 +730,7 @@ __csched_vcpu_is_migrateable(const struct csched_private *prv, struct vcpu *vc,
      */
     ASSERT(!vc->is_running);
 
-    return !__csched_vcpu_is_cache_hot(prv, vc) &&
+    return !__csched_vcpu_is_cache_hot(prv, svc) &&
            cpumask_test_cpu(dest_cpu, mask);
 }
 
@@ -1870,6 +1875,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 66f1e26..4deacf6 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1493,7 +1493,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);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2201fac..7aa0be6 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -174,9 +174,6 @@ struct vcpu
     } runstate_guest; /* guest address */
 #endif
 
-    /* last time when vCPU is scheduled out */
-    uint64_t last_run_time;
-
     /* Has the FPU been initialised? */
     bool             fpu_initialised;
     /* Has the FPU been used since it was last saved? */
-- 
2.7.4


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

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

[George Dunlap]

> On May 30, 2019, at 3:15 PM, Andrii Anisov <andrii.anisov@gmail.com> wrote:
> 
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> 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?

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

 -George

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

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

[George Dunlap]

> On May 30, 2019, at 3:15 PM, Andrii Anisov <andrii.anisov@gmail.com> wrote:
> 
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> 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?

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

 -George

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2685 bytes --]

On Fri, 2019-05-31 at 10:26 +0000, George Dunlap wrote:
> > On May 30, 2019, at 3:15 PM, Andrii Anisov <andrii.anisov@gmail.com
> > > wrote:
> > 
> > From: Andrii Anisov <andrii_anisov@epam.com>
> > 
> > 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2685 bytes --]

On Fri, 2019-05-31 at 10:26 +0000, George Dunlap wrote:
> > On May 30, 2019, at 3:15 PM, Andrii Anisov <andrii.anisov@gmail.com
> > > wrote:
> > 
> > From: Andrii Anisov <andrii_anisov@epam.com>
> > 
> > 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Andrii Anisov]

Hello George,

On 31.05.19 13:26, George Dunlap wrote:
> 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?

The initial intention was avoiding layering/encapsulation violation (as Dario mentioned). And it is stated in the commit message.
Structure size reduction is just a fine side effect FMPOV. If you like I can add a short notice about that into the message.

-- 
Sincerely,
Andrii Anisov.

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

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

[Andrii Anisov]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]

On Mon, 2019-06-10 at 18:16 +0300, Andrii Anisov wrote:
> On 31.05.19 16:24, Dario Faggioli wrote:
> > --- 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.
> 
Well, in general, I've mixed feelings on the subject.

About this specific case, if we find a suitable new name for the field,
I agree that the comment may very well become pretty useless.

> > 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.
> 
Ok, yes, this is something I personally can live with. :-)

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Andrii Anisov]

Hello Dario


On 11.06.19 13:01, Dario Faggioli wrote:

>> 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.
>>
> Ok, yes, this is something I personally can live with. :-)

Good.
I'll send v4 with no comment but renamed member.

-- 
Sincerely,
Andrii Anisov.

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