Re: [Xen-devel] [PATCH v3 1/4] sched: credit2: respect per-vcpu hard affinity

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Wed, 2015-03-25 at 23:48 -1000, Justin T. Weaver wrote:
> by making sure that vcpus only run on the pcpu(s) they are allowed to
> run on based on their hard affinity cpu masks.
> 
And here I am again... reviewing this is really taking _ages_...
Sorry! :-(

> Signed-off-by: Justin T. Weaver <jtweaver@xxxxxxxxxx>
> ---
> Changes in v3:

>  * Added #define for VCPU2ONLINE (probably should be factored out of
>    schedule.c and here, and put into a common header)
>
+1 for moving to an header. I think xen/include/xen/sched.h is the best
bit.
  
>  * Moved vc->processor assignment in function csched2_vcpu_migrate to an else
>    block to execute only if current and destination run queues are the same;
>    Note: without the processor assignment here the vcpu might be assigned to a
>    processor it no longer is allowed to run on. In that case, function
>    runq_candidate may only get called for the vcpu's old processor, and
>    runq_candidate will no longer let a vcpu run on a processor that it's not
>    allowed to run on (because of the hard affinity check first introduced in
>    v1 of this patch).
>
As I think I said already, this deserves to be somewhere where it won't
disappear, which is either the changelog or a comment in the code. I'd
go for the latter.

> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 7581731..af716e4 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -176,6 +176,7 @@ integer_param("sched_credit2_migrate_resist", 
> opt_migrate_resist);
>  #define c2r(_ops, _cpu)     (CSCHED2_PRIV(_ops)->runq_map[(_cpu)])
>  /* CPU to runqueue struct macro */
>  #define RQD(_ops, _cpu)     (&CSCHED2_PRIV(_ops)->rqd[c2r(_ops, _cpu)])
> +#define VCPU2ONLINE(_v)     cpupool_online_cpumask((_v)->domain->cpupool)
>
As said above, move this in a .h

>  integer_param("credit2_balance_over", opt_overload_balance_tolerance);
>  
>  /*
> + * Use this to avoid having too many cpumask_t structs on the stack
> + */
> +static cpumask_t **scratch_mask = NULL;
> +#define csched2_cpumask scratch_mask[smp_processor_id()]
> +
We covered this already.

> +/*
>   * Per-runqueue data
>   */
>  struct csched2_runqueue_data {
> @@ -268,6 +275,32 @@ struct csched2_dom {
>      uint16_t nr_vcpus;
>  };
>  
> +/*
> + * When a hard affinity change occurs, we may not be able to check some or
> + * all of the other run queues for a valid new processor for the given vcpu
> + * because (in function choose_cpu) either the trylock on the private data
> + * failed or the trylock on each run queue with valid processor(s) for svc
> + * failed. In these cases, this function is used to pick a pcpu that svc can
> + * run on. It returns svc's current pcpu if valid, or a different pcpu from
> + * the run queue svc is currently assigned to, or if none of those are valid,
> + * it returns a pcpu from the intersection of svc's hard affinity and the
> + * domain's online cpumask.
>
I'd make the last part a bulleted list:

"It returns, in order of preference:
 * svc's current pcpu, if still valid;
 * another (valid) pcpu from the svc's current runq, if any;
 * just a valid pcpu, i.e., a pcpu that is online, in svc's domain's 
   cpupool, and in svc's hard affinity."

> + */
> +static int get_fallback_cpu(struct csched2_vcpu *svc)
> +{
> +    if ( likely(cpumask_test_cpu(svc->vcpu->processor,
> +        svc->vcpu->cpu_hard_affinity)) )
> +        return svc->vcpu->processor;
> +
> +    cpumask_and(csched2_cpumask, svc->vcpu->cpu_hard_affinity,
> +        &svc->rqd->active);
> +    if ( cpumask_empty(csched2_cpumask) )
> +        cpumask_and(csched2_cpumask, svc->vcpu->cpu_hard_affinity,
> +            VCPU2ONLINE(svc->vcpu));
> +    ASSERT( !cpumask_empty(csched2_cpumask) );
> +
> +    return cpumask_any(csched2_cpumask);
>
I like the idea of cpumask_any(), but I'm not sure paying the pice of
making this random is worth here... after all, it's a fallback, so I
think just cpumask_first() is enough.

> +}

Also, coding style: align with the relevant opening parenthesis:

    if ( likely(cpumask_test_cpu(svc->vcpu->processor,
                                 svc->vcpu->cpu_hard_affinity)) )
    ...
    cpumask_and(csched2_cpumask, svc->vcpu->cpu_hard_affinity,
                &svc->rqd->active);
    ...
    and the same everywhere else, of course.

All this being said, I think the variant here below is both easier to
understand (as it maps almost 1:1 with the bulleted list in the comment)
and a bit more efficient. Do you like it?

static int get_fallback_cpu(struct csched2_vcpu *svc)
{
    if ( likely(cpumask_test_cpu(svc->vcpu->processor,
                svc->vcpu->cpu_hard_affinity)) )
        return svc->vcpu->processor;

    cpumask_and(csched2_cpumask, svc->vcpu->cpu_hard_affinity,
                &svc->rqd->active);
    cpu = cpumask_first(csched2_cpumask);
    if ( likely(cpu < nr_cpu_ids) )
        return cpu;

    cpumask_and(csched2_cpumask, svc->vcpu->cpu_hard_affinity,
                VCPU2ONLINE(svc->vcpu));
    ASSERT( !cpumask_empty(csched2_cpumask) );
    return cpumask_first(csched2_cpumask);
}

> @@ -1223,7 +1272,12 @@ static void migrate(const struct scheduler *ops,
>              on_runq=1;
>          }
>          __runq_deassign(svc);
> -        svc->vcpu->processor = cpumask_any(&trqd->active);
> +
> +        cpumask_and(csched2_cpumask, svc->vcpu->cpu_hard_affinity,
> +            &trqd->active);
> +        svc->vcpu->processor = cpumask_any(csched2_cpumask);
> +        BUG_ON(svc->vcpu->processor >= nr_cpu_ids);
> +
>          __runq_assign(svc, trqd);
>          if ( on_runq )
>          {
> @@ -1237,6 +1291,20 @@ static void migrate(const struct scheduler *ops,
>      }
>  }
>  
> +/*
> + * Migration of vcpu svc to run queue rqd is a valid option if svc is not
>
"Migration of svc" is fine.

Also, "runqueue" seems the dominant variant (over "run queue"), so I'd
stick to that.

> + * already flagged to migrate and if svc is allowed to run on at least one of
> + * the pcpus assigned to rqd based on svc's hard affinity mask.
> + */
> +static bool_t valid_vcpu_migration(struct csched2_vcpu *svc,
> +                                   struct csched2_runqueue_data *rqd)
>
The name, I don't like it much. In credit1 we have
__csched_vcpu_is_migrateable(), which does pretty much the same. I don't
care about the "__csched" part, but I like vcpu_is_migrateable() better
than valid_vcpu_migration().

> +{
> +    if ( test_bit(__CSFLAG_runq_migrate_request, &svc->flags)
> +        || !cpumask_intersects(svc->vcpu->cpu_hard_affinity, &rqd->active) )
> +        return 0;
> +    else
> +        return 1;
> +}
>
I'd also make it "positive", i.e., I'd structure the condition so that
if it's true, function returns true (and, BTW, you should use 'true' and
'false'), as I find that easier to read.

Finally, the else is pointless. Do either:

static bool_tvcpu_is_migrateable(...)
{
    return <condition>;
}

or:

static bool_tvcpu_is_migrateable(...)
{
    if ( <condition> )
        return true;
    return false;
}

Right... As I'm sure you've noticed, there still are some changes needed
(at least IMO), but they're rather minor adjustments... So great work on
this so far, we're really getting there.

Thanks and sorry again for the latency.

Regards,
Dario

Attachment: signature.asc
Description: This is a digitally signed message part

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