[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1 of 6 v2] xen: sched_credit: improve picking up the idlal CPU for a VCPU
>>> On 12.12.12 at 03:52, Dario Faggioli <dario.faggioli@xxxxxxxxxx> wrote: > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -59,6 +59,8 @@ > #define CSCHED_VCPU(_vcpu) ((struct csched_vcpu *) (_vcpu)->sched_priv) > #define CSCHED_DOM(_dom) ((struct csched_dom *) (_dom)->sched_priv) > #define RUNQ(_cpu) (&(CSCHED_PCPU(_cpu)->runq)) > +/* Is the first element of _cpu's runq its idle vcpu? */ > +#define IS_RUNQ_IDLE(_cpu) > (is_idle_vcpu(__runq_elem(RUNQ(_cpu)->next)->vcpu)) > > > /* > @@ -479,9 +481,14 @@ static int > * distinct cores first and guarantees we don't do something stupid > * like run two VCPUs on co-hyperthreads while there are idle cores > * or sockets. > + * > + * Notice that, when computing the "idleness" of cpu, we may want to > + * discount vc. That is, iff vc is the currently running and the only > + * runnable vcpu on cpu, we add cpu to the idlers. > */ > cpumask_and(&idlers, &cpu_online_map, CSCHED_PRIV(ops)->idlers); > - cpumask_set_cpu(cpu, &idlers); > + if ( current_on_cpu(cpu) == vc && IS_RUNQ_IDLE(cpu) ) > + cpumask_set_cpu(cpu, &idlers); > cpumask_and(&cpus, &cpus, &idlers); > cpumask_clear_cpu(cpu, &cpus); > > @@ -489,7 +496,7 @@ static int > { > cpumask_t cpu_idlers; > cpumask_t nxt_idlers; > - int nxt, weight_cpu, weight_nxt; > + int nxt, nr_idlers_cpu, nr_idlers_nxt; > int migrate_factor; > > nxt = cpumask_cycle(cpu, &cpus); > @@ -513,12 +520,12 @@ static int > cpumask_and(&nxt_idlers, &idlers, per_cpu(cpu_core_mask, nxt)); > } > > - 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. That said - I don't think it will (and even more, I'm agreeable to the change done). > --- 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. 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"? Jan > #define DOMAIN_DESTROYED (1<<31) /* assumes atomic_t is >= 32 bits */ > #define put_domain(_d) \ > if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |