On Tue, 2019-04-23 at 10:44 +0100, Andrew Cooper wrote:
> On 20/04/2019 16:24, Dario Faggioli wrote:
>
> Definitely +1 to algorithm changes which avoid its use entirely.
> 
> A few cosmetic observations.
> 
Ok...

> > diff --git a/xen/common/sched_credit2.c
> > b/xen/common/sched_credit2.c
> > index 6958b265fc..7034325243 100644
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -466,6 +466,7 @@ struct csched2_runqueue_data {
> >      spinlock_t lock;           /* Lock for this
> > runqueue                     */
> >  
> >      struct list_head runq;     /* Ordered list of runnable
> > vms               */
> > +    int nr_cpus;               /* How many CPUs are sharing this
> > runqueue    */
> 
> Unsigned int.
> 
Yep.

> > @@ -3944,8 +3951,12 @@ csched2_deinit_pdata(const struct scheduler
> > *ops, void *pcpu, int cpu)
> >      __cpumask_clear_cpu(cpu, &rqd->smt_idle);
> >      __cpumask_clear_cpu(cpu, &rqd->active);
> >  
> > -    if ( cpumask_empty(&rqd->active) )
> > +    rqd->nr_cpus--;
> > +    ASSERT(cpumask_weight(&rqd->active) == rqd->nr_cpus);
> > +
> > +    if ( rqd->nr_cpus == 0 )
> >      {
> > +	ASSERT(cpumask_empty(&rqd->active));
> 
> And here.
> 
Right. Not sure how they ended in there. Will remove them.

> Is it really worth having both asserts?  The second is redundant.
> 
I think I agree. I'll keep the first one only.

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)