x86: identifying the boot cpu
PATCH 1/2 x86: identifying the boot cpu
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.
--- 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(-)
xen/arch/x86/setup.c +5 -2
@@ -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 )
xen/arch/x86/smpboot.c +2 -1
@@ -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 };
xen/include/asm-x86/processor.h +2 -0
@@ -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 );
Signature · 1 line
1.9.1
Please use git-send-email and thread your patch series properly. You are still submitting 3 independent emails.
Quoted 9 lines · 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 <[email protected]>
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.
Quoted 11 lines
--- 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(-)xen/arch/x86/setup.c +0
-0
xen/arch/x86/setup.c void __init noreturn __start_xen(unsigned long mbi_p) · line 1265
Quoted 7 lines
xen/arch/x86/setup.c +4
-0@@ -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
Quoted 41 lines
xen/arch/x86/setup.c +1
-2+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 )xen/arch/x86/smpboot.c +2
-1@@ -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 };xen/include/asm-x86/processor.h +2
-0@@ -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 );
Quoted 12 lines · Andrew Cooper
Please use git-send-email and thread your patch series properly. You are still submitting 3 independent emails.
Quoted 8 lines · 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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Uma Sharma <[email protected]>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
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 13.03.15 at 19:07, <[email protected]> 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.
Quoted 5 lines
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