Re: [PATCH v2 2/2] sched_credit2.c: runqueue_per_core code

[George Dunlap <george.dunlap@eu.citrix.com>]

On 03/13/2015 06:29 PM, Andrew Cooper wrote:

>> +#define CREDIT2_OPT_RUNQUEUE_CORE 1
>> +#define CREDIT2_OPT_RUNQUEUE_SOCKET 2
> 
> You can drop _OPT out of the name.  It serves only to make the constant
> longer.

I suggested _OPT so that nobody would be confused into thinking they
were used in the core mechanism.

> Also, this should probably be an enum as the exact numbers themselves
> are not interesting.

I also suggested #defines, since there are only 2, and the rest of the
code doesn't really use enums.

> 
>>  
>>  int opt_migrate_resist=500;
>>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
>> +char __initdata opt_credit2_runqueue_string[10] = "core";
>> +string_param("credit2_runqueue", opt_credit2_runqueue_string);
>> +int opt_credit2_runqueue = CREDIT2_OPT_RUNQUEUE_CORE;
>>  
>>  /*
>>   * Useful macros
>> @@ -1940,10 +1946,10 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>>  
>>      /* Figure out which runqueue to put it in */
>>      /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
>> -    if ( cpu == 0 )
>> -        rqi = 0;
>> +    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
>> +        rqi = cpu ? cpu_to_socket(cpu) : boot_cpu_to_socket();
> 
> This conditional is bogus.  If cpu0 is offlined and re-onlined, it must
> use cpu_to_core()
> 
> This entire hunk should probably be
> 
> rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ?
> cpu_to_socket(cpu) : cpu_to_core(cpu);
> 
> (with suitable alignment)

You're ignoring the fact that she's following suit from existing code;
and that that code is there for a reason: When this is first called for
cpu 0, cpu_to_socket() (and cpu_to_core()) return garbage since they
haven't been initialized yet.

That is something that needs to be fixed, but it's not Uma's job to fix it.

 -George