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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.12.12 at 20:50, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> On 12/12/12 10:04, Jan Beulich wrote:
>>>>> 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.
> 
> 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.

Any use of schedule_data, the type of which is declared in
sched-if.h, should be in sched-if.h - someone only including
sched.h can't make use of it anyway (and it's intended to be
used by scheduler code, i.e. shouldn't be visible to other
code).

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

It's bad practice imo; I have always understood this as
questionable attempts of people to avoid name clashes (which
is understandable only for variables declared locally inside a
macro definition).

Jan

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