Re: [Xen-devel] [PATCH 1 of 6 v2] xen: sched_credit: improve picking up the idlal CPU for a VCPU

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Wed, 2012-12-12 at 10:04 +0000, Jan Beulich wrote: 
> > -        weight_cpu = cpumask_weight(&cpu_idlers);
> > -        weight_nxt = cpumask_weight(&nxt_idlers);
> > +        nr_idlers_cpu = cpumask_weight(&cpu_idlers);
> > +        nr_idlers_nxt = cpumask_weight(&nxt_idlers);
> >          /* smt_power_savings: consolidate work rather than spreading it */
> >          if ( sched_smt_power_savings ?
> > -             weight_cpu > weight_nxt :
> > -             weight_cpu * migrate_factor < weight_nxt )
> > +             nr_idlers_cpu > nr_idlers_nxt :
> > +             nr_idlers_cpu * migrate_factor < nr_idlers_nxt )
> >          {
> >              cpumask_and(&nxt_idlers, &cpus, &nxt_idlers);
> >              spc = CSCHED_PCPU(nxt);
> 
> Despite you mentioning this in the description, these last two hunks
> are, afaict, only renaming variables (and that's even debatable, as
> the current names aren't really misleading imo), and hence I don't
> think belong in a patch that clearly has the potential for causing
> (performance) regressions.
> 
Ok, I think I can live with the current names too... Just a matter of
taste. :-)

> That said - I don't think it will (and even more, I'm agreeable to the
> change done).
> 
It has been benchmarked, together with the next change, and the results
are in the changelog of 2/6. Numbers there show that the combination of
those two changes are much more an improvement than anything else, at
least for the workloads I considered (which includes sysbench and
specjbb2005).

Anyway, I think I see your point, and I can either move the remane
somewhere else or kill it entirely.

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -396,6 +396,9 @@ extern struct vcpu *idle_vcpu[NR_CPUS];
> >  #define is_idle_domain(d) ((d)->domain_id == DOMID_IDLE)
> >  #define is_idle_vcpu(v)   (is_idle_domain((v)->domain))
> >  
> > +#define current_on_cpu(_c) \
> > +  ( (per_cpu(schedule_data, _c).curr) )
> > +
> 
> This, imo, really belings into sched-if.h.
> 
Ok.

> Plus - what's the point of double parentheses, when in fact none
> at all would be needed?
> 
> And finally, why "_c" and not just "c"?
> 
Nothing particular, just "personal macro style", I guess, which I can
convert to what you ask and resend.

Thanks,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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

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