From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 1 of 6 v2] xen: sched_credit: improve picking up the idlal CPU for a VCPU Date: Fri, 14 Dec 2012 19:50:48 +0000 Message-ID: <50CB8318.6050807@eu.citrix.com> References: <50C864D402000078000AFDB0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50C864D402000078000AFDB0@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Dario Faggioli , "Keir (Xen.org)" , Ian Campbell , xen-devel List-Id: xen-devel@lists.xenproject.org On 12/12/12 10:04, Jan Beulich wrote: >>>> On 12.12.12 at 03:52, Dario Faggioli 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. Hmm, it looks like there are a number of things that could live in either sched-if.h or sched.h; but I think this one probably most closely links with thins like vcpu_is_runnable() and cpu_is_haltable(), both of which are in sched.h; so sched.h is where I'd put it. > 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"? I think the underscore is pretty standard in macros. There's certainly no need for double parentheses though. -George