Re: [Xen-devel] [PATCH 13/19] xen: credit2: make the code less experimental

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Sat, Jun 18, 2016 at 12:12 AM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> Mainly, almost all of the BUG_ON-s can be converted into
> ASSERTS, and the debug printk either removed or turned
> into tracing.
>
> The 'TODO' list, in a comment at the beginning of the file,
> was also stale, so remove items that were still there but
> are actually done.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

Overall looks good.  A couple of things...

> @@ -680,8 +677,8 @@ __update_svc_load(const struct scheduler *ops,
>          delta = now - svc->load_last_update;
>          if ( unlikely(delta < 0) )
>          {
> -            d2printk("%s: Time went backwards? now %"PRI_stime" llu 
> %"PRI_stime"\n",
> -                     __func__, now, svc->load_last_update);
> +            printk("WARNING: %s: Time went backwards? now %"PRI_stime" llu 
> %"PRI_stime"\n",
> +                   __func__, now, svc->load_last_update);

Hmm, I'm afraid this makes all Jan's comments from patch 7 which I
argued against since it was just a debugging message now valid.

> @@ -1540,9 +1536,26 @@ static void migrate(const struct scheduler *ops,
>                      struct csched2_runqueue_data *trqd,
>                      s_time_t now)
>  {
> -    if ( svc->flags & CSFLAG_scheduled )
> +    bool_t running = svc->flags & CSFLAG_scheduled;
> +    bool_t on_runq = __vcpu_on_runq(svc);

What's the point of having these variables here?  AFAICS 'running' is
used exactly once; and on_runq is only used inside the original else {
} clause where it was before.

> @@ -2069,12 +2076,13 @@ csched2_schedule(
>                  }
>              }
>          }
> -        printk("%s: pcpu %d rq %d, but scurr %pv assigned to "
> +        printk("DEBUG: %s: pcpu %d rq %d, but scurr %pv assigned to "
>                 "pcpu %d rq %d!\n",
>                 __func__,
>                 cpu, this_rqi,
>                 scurr->vcpu, scurr->vcpu->processor, other_rqi);
>      }
> +#endif

Do we need this path anymore? I think it was just there to help
debugging; but all this should have been sorted out a long time ago.
:-)

Thanks,
 -George

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