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

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

On 03/16/2015 12:48 PM, Jan Beulich wrote:
>>>> On 13.03.15 at 20:13, <george.dunlap@eu.citrix.com> wrote:
>> On 03/13/2015 06:29 PM, Andrew Cooper wrote:
>>>> @@ -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.
> 
> Them returning garbage isn't what needs fixing. Instead the code
> here should use a different condition to check whether this is the
> boot CPU (e.g. looking at system_state). And that can very well be
> done directly in this patch.

What do you suggest, then?

 -George