On Mon, 2016-06-20 at 02:26 -0600, Jan Beulich wrote:
> > > > On 18.06.16 at 01:13, <dario.faggioli@citrix.com> 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.
> 
Ok.

> 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?
> 
Agreed too.

> > @@ -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?
> 
No, it's me that should have removed this line after the last round of
testing, but forgot. Apologies. :-/

> > @@ -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.
> 
Yep.

Thanks and 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)