[PATCH 1/2] x86: identifying the boot cpu

[PATCH 1/2] x86: identifying the boot cpu

[Uma Sharma]

Provide helpers to access the socket and core IDs, resulting from
identification phase.
Initialize socket and core ID to -1 i.e invalid instead of 0. Having
that field in all elements set to 0 would induce credit2 to think that
the pCPU have already been initialized, and that all are on socket 0
in case of credit2 socket scheduler and on core 0 in case of credit2
core scheduler.

Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
---
 xen/arch/x86/setup.c            | 7 +++++--
 xen/arch/x86/smpboot.c          | 3 ++-
 xen/include/asm-x86/processor.h | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 7593533..251840f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1259,6 +1259,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     microcode_grab_module(module_map, mbi, bootstrap_map);
 
     timer_init();
+    /*
+     * Identify the boot CPU, in case the scheduler initialization
+     * needs to know about it (e.g., topology, etc.)
+     */
+    identify_cpu(&boot_cpu_data);
 
     init_idle_domain();
 
@@ -1270,8 +1275,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     arch_init_memory();
 
-    identify_cpu(&boot_cpu_data);
-
     if ( cpu_has_fxsr )
         set_in_cr4(X86_CR4_OSFXSR);
     if ( cpu_has_xmm )
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 314e253..ac8c4c8 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -59,7 +59,8 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
 cpumask_t cpu_online_map __read_mostly;
 EXPORT_SYMBOL(cpu_online_map);
 
-struct cpuinfo_x86 cpu_data[NR_CPUS];
+struct cpuinfo_x86 cpu_data[NR_CPUS] =
+        { [0 ... NR_CPUS-1] = { .phys_proc_id = -1, .cpu_core_id = -1 } };
 
 u32 x86_cpu_to_apicid[NR_CPUS] __read_mostly =
 	{ [0 ... NR_CPUS-1] = BAD_APICID };
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 87d80ff..6ec9588 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -214,7 +214,9 @@ extern void detect_extended_topology(struct cpuinfo_x86 *c);
 
 extern void detect_ht(struct cpuinfo_x86 *c);
 
+#define boot_cpu_to_core()   (boot_cpu_data.cpu_core_id)
 #define cpu_to_core(_cpu)   (cpu_data[_cpu].cpu_core_id)
+#define boot_cpu_to_socket() (boot_cpu_data.phys_proc_id)
 #define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id)
 
 unsigned int apicid_to_socket(unsigned int);
-- 
1.9.1

Re: [PATCH 1/2] x86: identifying the boot cpu

[Andrew Cooper]

Please use git-send-email and thread your patch series properly.  You
are still submitting 3 independent emails.

On 13/03/15 18:07, Uma Sharma wrote:
> Provide helpers to access the socket and core IDs, resulting from
> identification phase.
> Initialize socket and core ID to -1 i.e invalid instead of 0. Having
> that field in all elements set to 0 would induce credit2 to think that
> the pCPU have already been initialized, and that all are on socket 0
> in case of credit2 socket scheduler and on core 0 in case of credit2
> core scheduler.
>
> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>

You have not addressed the cpu onlining/offlining problem which Jan
asked you about.

Furthermore, you don't make any justification as to why it is safe to
change the defaults under all the other users of cpu_data.

> ---
>  xen/arch/x86/setup.c            | 7 +++++--
>  xen/arch/x86/smpboot.c          | 3 ++-
>  xen/include/asm-x86/processor.h | 2 ++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 7593533..251840f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1259,6 +1259,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      microcode_grab_module(module_map, mbi, bootstrap_map);
>  
>      timer_init();
> +    /*
> +     * Identify the boot CPU, in case the scheduler initialization
> +     * needs to know about it (e.g., topology, etc.)
> +     */

I wouldn't bother with the comment here.  This is a long line of
initialisation, and everyone has to pay close attention to the order of
things.

~Andrew

> +    identify_cpu(&boot_cpu_data);
>  
>      init_idle_domain();
>  
> @@ -1270,8 +1275,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      arch_init_memory();
>  
> -    identify_cpu(&boot_cpu_data);
> -
>      if ( cpu_has_fxsr )
>          set_in_cr4(X86_CR4_OSFXSR);
>      if ( cpu_has_xmm )
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 314e253..ac8c4c8 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -59,7 +59,8 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
>  cpumask_t cpu_online_map __read_mostly;
>  EXPORT_SYMBOL(cpu_online_map);
>  
> -struct cpuinfo_x86 cpu_data[NR_CPUS];
> +struct cpuinfo_x86 cpu_data[NR_CPUS] =
> +        { [0 ... NR_CPUS-1] = { .phys_proc_id = -1, .cpu_core_id = -1 } };
>  
>  u32 x86_cpu_to_apicid[NR_CPUS] __read_mostly =
>  	{ [0 ... NR_CPUS-1] = BAD_APICID };
> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index 87d80ff..6ec9588 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -214,7 +214,9 @@ extern void detect_extended_topology(struct cpuinfo_x86 *c);
>  
>  extern void detect_ht(struct cpuinfo_x86 *c);
>  
> +#define boot_cpu_to_core()   (boot_cpu_data.cpu_core_id)
>  #define cpu_to_core(_cpu)   (cpu_data[_cpu].cpu_core_id)
> +#define boot_cpu_to_socket() (boot_cpu_data.phys_proc_id)
>  #define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id)
>  
>  unsigned int apicid_to_socket(unsigned int);

Re: [PATCH 1/2] x86: identifying the boot cpu

[George Dunlap]

On 03/13/2015 06:16 PM, Andrew Cooper wrote:
> Please use git-send-email and thread your patch series properly.  You
> are still submitting 3 independent emails.
> 
> On 13/03/15 18:07, Uma Sharma wrote:
>> Provide helpers to access the socket and core IDs, resulting from
>> identification phase.
>> Initialize socket and core ID to -1 i.e invalid instead of 0. Having
>> that field in all elements set to 0 would induce credit2 to think that
>> the pCPU have already been initialized, and that all are on socket 0
>> in case of credit2 socket scheduler and on core 0 in case of credit2
>> core scheduler.
>>
>> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
> 
> You have not addressed the cpu onlining/offlining problem which Jan
> asked you about.
> 
> Furthermore, you don't make any justification as to why it is safe to
> change the defaults under all the other users of cpu_data.

That's because Dario said these were his patches and that he would be
doing the revision based on comments.

That said, Uma, if it was your intention not to actually suggest this be
accepted now, then you probably should have put "RFC" in the subject
line, and mentioned that this is a patch that Dario is going to work on
a replacement for, so that people know not to review it.

 -George

Re: [PATCH 1/2] x86: identifying the boot cpu

[Jan Beulich]

>>> On 13.03.15 at 19:07, <uma.sharma523@gmail.com> wrote:
> Provide helpers to access the socket and core IDs, resulting from
> identification phase.

Such helpers already exist, as can be seen from the context of the
hunk changing xen/include/asm-x86/processor.h. What you add
are accessors specifically for the boot CPU.

> Initialize socket and core ID to -1 i.e invalid instead of 0. Having
> that field in all elements set to 0 would induce credit2 to think that
> the pCPU have already been initialized, and that all are on socket 0
> in case of credit2 socket scheduler and on core 0 in case of credit2
> core scheduler.

So how would the scheduler initialization work better should it
happen to encounter a -1 here? I.e. how is the change you
describe an improvement? In the end - aiui - you want to make
sure these fields get initialized properly on all CPUs before the
scheduler first looks at them. And if you achieve that, I can't
see how changing the initializers would make any difference. As
a side note - if this was really important, I'm sure you would also
want to add respective initializers for boot_cpu_data. Plus it
would remain to be explained why compute_unit_id wouldn't
also want to be non-zero-initialized.

Assuming this my understanding of the goal of the patch is
correct, then the title still misses the word "earlier" (or some
equivalent) to properly describe what all this is about, and
the description still fails to explicitly state the reason for this
(one can now guess that, I admit).

Jan