Re: [Xen-devel] [PATCH 18/19] xen: credit2: implement SMT support independent runq arrangement

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.06.16 at 01:13, <dario.faggioli@xxxxxxxxxx> wrote:
> +static inline
> +void smt_idle_mask_set(unsigned int cpu, cpumask_t *idlers, cpumask_t *mask)
> +{
> +    if ( cpumask_subset( per_cpu(cpu_sibling_mask, cpu), idlers) )
> +        cpumask_or(mask, mask, per_cpu(cpu_sibling_mask, cpu));
> +}

I think helpers like this should be made const-correct. Here idlers
is only an input.

Also I'm not sure the compiler can fold the redundant
per_cpu(cpu_sibling_mask, cpu) in all cases. Is it maybe worth
helping it by using a local variable here or moving the expression
into the caller's invocation expression?

And as a side note - there a stray space inside the cpumask_subset().

> @@ -945,6 +1034,7 @@ runq_tickle(const struct scheduler *ops, struct 
> csched2_vcpu *new, s_time_t now)
>                      (unsigned char *)&d);
>      }
>      __cpumask_set_cpu(ipid, &rqd->tickled);
> +    //smt_idle_mask_clear(ipid, &rqd->smt_idle); XXX
>      cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
>  }

With this, was the patch meant to be RFC?

> @@ -1435,13 +1525,15 @@ csched2_cpu_pick(const struct scheduler *ops, struct 
> vcpu *vc)
>  
>      if ( !read_trylock(&prv->lock) )
>      {
> -        /* We may be here because someon requested us to migrate */
> +        /* We may be here because someone requested us to migrate */

Please add the missing full stop at once.

Jan


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