[PATCH 0/2] xen: Credit2: fix two issues from recently committed series.

[PATCH 0/2] xen: Credit2: fix two issues from recently committed series.

[Dario Faggioli]

Series committed yesterday, to be precise, and two (not too big, at least :-))
issues, have been found already (thanks Andrew!).

Thanks and Regards,
Dario
---
Dario Faggioli (2):
      xen: credit2: fix two s_time_t handling issues in load balancing
      xen: credit2: fix potential issues in csched2_cpu_pick with tracing enabled

 xen/common/sched_credit2.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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

[PATCH 1/2] xen: credit2: fix two s_time_t handling issues in load balancing

[Dario Faggioli]

both introduced in d205f8a7f48e2ec ("xen: credit2: rework
load tracking logic").

First, in __update_runq_load(), the ASSERT() was actually
useless. Let's instead check that the computed value of
the load has not overflowed (and hence gone negative).

While there, do that in __update_svc_load() as well.

Second, in balance_load(), cpus_max needs being extended
in order to be correctly shifted, and the result compared
with an s_time_t value, without risking loosing info.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
 xen/common/sched_credit2.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index b33ba7a..3d3e4ae 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -656,7 +656,8 @@ __update_runq_load(const struct scheduler *ops,
     rqd->load += change;
     rqd->load_last_update = now;
 
-    ASSERT(rqd->avgload <= STIME_MAX && rqd->b_avgload <= STIME_MAX);
+    /* Overflow, capable of making the load look negative, must not occur. */
+    ASSERT(rqd->avgload > 0 && rqd->b_avgload > 0);
 
     if ( unlikely(tb_init_done) )
     {
@@ -714,6 +715,9 @@ __update_svc_load(const struct scheduler *ops,
     }
     svc->load_last_update = now;
 
+    /* Overflow, capable of making the load look negative, must not occur. */
+    ASSERT(svc->avgload > 0);
+
     if ( unlikely(tb_init_done) )
     {
         struct {
@@ -1742,7 +1746,7 @@ retry:
          * If we're under 100% capacaty, only shift if load difference
          * is > 1.  otherwise, shift if under 12.5%
          */
-        if ( load_max < (cpus_max << prv->load_precision_shift) )
+        if ( load_max < ((s_time_t)cpus_max << prv->load_precision_shift) )
         {
             if ( st.load_delta < (1ULL << (prv->load_precision_shift +
                                            opt_underload_balance_tolerance)) )


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

Re: [PATCH 1/2] xen: credit2: fix two s_time_t handling issues in load balancing

[Dario Faggioli]

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

On Tue, 2016-07-19 at 14:07 +0200, Dario Faggioli wrote:
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -656,7 +656,8 @@ __update_runq_load(const struct scheduler *ops,
>      rqd->load += change;
>      rqd->load_last_update = now;
>  
> -    ASSERT(rqd->avgload <= STIME_MAX && rqd->b_avgload <=
> STIME_MAX);
> +    /* Overflow, capable of making the load look negative, must not
> occur. */
> +    ASSERT(rqd->avgload > 0 && rqd->b_avgload > 0);
>  
Wait! This quite obviously wants to be '>= 0' !!

Sorry for the glaring mistake. v2 coming...

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

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

[PATCH 2/2] xen: credit2: fix potential issues in csched2_cpu_pick with tracing enabled

[Dario Faggioli]

In fact, when not finding a suitable runqueue where to
place a vCPU, and hence using a fallback, we either:
 - don't issue any trace record (while we should),
 - risk underruning when accessing the runqueues
   array, while preparing the trace record.

Fix both issues and, while there, also a couple of style
problems found nearby.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
 xen/common/sched_credit2.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 3d3e4ae..7bfb24a 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1443,7 +1443,8 @@ csched2_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
     {
         /* We may be here because someone requested us to migrate. */
         __clear_bit(__CSFLAG_runq_migrate_request, &svc->flags);
-        return get_fallback_cpu(svc);
+        new_cpu = get_fallback_cpu(svc);
+        goto out;
     }
 
     /* First check to see if we're here because someone else suggested a place
@@ -1505,7 +1506,7 @@ csched2_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
         if ( rqd_avgload < min_avgload )
         {
             min_avgload = rqd_avgload;
-            min_rqi=i;
+            min_rqi = i;
         }
     }
 
@@ -1520,20 +1521,20 @@ csched2_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
         BUG_ON(new_cpu >= nr_cpu_ids);
     }
 
-out_up:
+ out_up:
     read_unlock(&prv->lock);
-
+ out:
     if ( unlikely(tb_init_done) )
     {
         struct {
             uint64_t b_avgload;
             unsigned vcpu:16, dom:16;
             unsigned rq_id:16, new_cpu:16;
-       } d;
-        d.b_avgload = prv->rqd[min_rqi].b_avgload;
+        } d;
         d.dom = vc->domain->domain_id;
         d.vcpu = vc->vcpu_id;
         d.rq_id = c2r(ops, new_cpu);
+        d.b_avgload = prv->rqd[d.rq_id].b_avgload;
         d.new_cpu = new_cpu;
         __trace_var(TRC_CSCHED2_PICKED_CPU, 1,
                     sizeof(d),


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

Re: [PATCH 0/2] xen: Credit2: fix two issues from recently committed series.

[Andrew Cooper]

On 19/07/16 13:06, Dario Faggioli wrote:
> Series committed yesterday, to be precise, and two (not too big, at least :-))
> issues, have been found already (thanks Andrew!).

I tend to put "Spotted by Coverity." in the commit message of these,
even when we can't provide CID numbers.

Its not like I spotted these from code inspection.

~Andrew

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

Re: [PATCH 0/2] xen: Credit2: fix two issues from recently committed series.

[Dario Faggioli]

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

On Tue, 2016-07-19 at 13:20 +0100, Andrew Cooper wrote:
> On 19/07/16 13:06, Dario Faggioli wrote:
> > 
> > Series committed yesterday, to be precise, and two (not too big, at
> > least :-))
> > issues, have been found already (thanks Andrew!).
> I tend to put "Spotted by Coverity." in the commit message of these,
> even when we can't provide CID numbers.
> 
> Its not like I spotted these from code inspection.
> 
Ok, I see. As you say, since I could not provide a meaningful CID, that
would not seem too useful to say either, to me.

But in any case, if you feel strong enough about it, I'm ok resending
with that, or about anyone committing the patches adding it.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

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

Re: [PATCH 0/2] xen: Credit2: fix two issues from recently committed series.

[Andrew Cooper]

On 19/07/16 13:26, Dario Faggioli wrote:
> On Tue, 2016-07-19 at 13:20 +0100, Andrew Cooper wrote:
>> On 19/07/16 13:06, Dario Faggioli wrote:
>>> Series committed yesterday, to be precise, and two (not too big, at
>>> least :-))
>>> issues, have been found already (thanks Andrew!).
>> I tend to put "Spotted by Coverity." in the commit message of these,
>> even when we can't provide CID numbers.
>>
>> Its not like I spotted these from code inspection.
>>
> Ok, I see. As you say, since I could not provide a meaningful CID, that
> would not seem too useful to say either, to me.
>
> But in any case, if you feel strong enough about it, I'm ok resending
> with that, or about anyone committing the patches adding it.

I am sure this can be tweaked on commit if there are no other issues.

I presume George will take care of committing in due course.

~Andrew

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