Re: [PATCH 01/19] xen: sched: leave CPUs doing tasklet work alone.

From: George Dunlap <george.dunlap@citrix.com>
To: Dario Faggioli <dario.faggioli@citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Anshul Makkar <anshul.makkar@citrix.com>,
	David Vrabel <david.vrabel@citrix.com>
Subject: Re: [PATCH 01/19] xen: sched: leave CPUs doing tasklet work alone.
Date: Wed, 6 Jul 2016 16:41:40 +0100	[thread overview]
Message-ID: <CAFLBxZZ2UHVXifwv+BkgUKcMpe5g4fF=W5dfLhyug0spxYg8+A@mail.gmail.com> (raw)
In-Reply-To: <146620508199.29766.7439697154591443704.stgit@Solace.fritz.box>

On Sat, Jun 18, 2016 at 12:11 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> In both Credit1 and Credit2, stop considering a pCPU idle,
> if the reason why the idle vCPU is being selected, is to
> do tasklet work.
>
> Not doing so means that the tickling and load balancing
> logic, seeing the pCPU as idle, considers it a candidate
> for picking up vCPUs. But the pCPU won't actually pick
> up or schedule any vCPU, which would then remain in the
> runqueue, which is bas, especially if there were other,

*bad

> truly idle pCPUs, that could execute it.
>
> The only drawback is that we can't assume that a pCPU is
> in always marked as idle when being removed from an
> instance of the Credit2 scheduler (csched2_deinit_pdata).
> In fact, if we are in stop-machine (i.e., during suspend
> or shutdown), the pCPUs are running the stopmachine_tasklet
> and hence are actually marked as busy. On the other hand,
> when removing a pCPU from a Credit2 pool, it will indeed
> be idle. The only thing we can do, therefore, is to
> remove the BUG_ON() check.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/common/sched_credit.c  |   12 ++++++------
>  xen/common/sched_credit2.c |   14 ++++++++++----
>  2 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index a38a63d..a6645a2 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1819,24 +1819,24 @@ csched_schedule(
>      else
>          snext = csched_load_balance(prv, cpu, snext, &ret.migrated);
>
> + out:

Sorry if I'm being a bit dense, but why is this moving up here?  As
far as I can tell the only time the 'out' label will be used, the
'idler' status of the cpu cannot change.

At very least moving it up here introduces a bug, since now...

>      /*
>       * Update idlers mask if necessary. When we're idling, other CPUs
>       * will tickle us when they get extra work.
>       */
> -    if ( snext->pri == CSCHED_PRI_IDLE )
> +    if ( tasklet_work_scheduled || snext->pri != CSCHED_PRI_IDLE )
>      {
> -        if ( !cpumask_test_cpu(cpu, prv->idlers) )
> -            cpumask_set_cpu(cpu, prv->idlers);
> +        if ( cpumask_test_cpu(cpu, prv->idlers) )
> +            cpumask_clear_cpu(cpu, prv->idlers);
>      }
> -    else if ( cpumask_test_cpu(cpu, prv->idlers) )
> +    else if ( !cpumask_test_cpu(cpu, prv->idlers) )
>      {
> -        cpumask_clear_cpu(cpu, prv->idlers);
> +        cpumask_set_cpu(cpu, prv->idlers);
>      }
>
>      if ( !is_idle_vcpu(snext->vcpu) )
>          snext->start_time += now;

...this will happen twice in the case (once in the if() clause, once
here).  (Although arguably the one in the if() clause should go away
and the out: label should be moved above this line anyway).

Other than that, looks good.

 -George

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