Re: [Xen-devel] [PATCH 1/2] x86: during boot, anticipate identifying the boot cpu

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

Hi,

Thanks Andrew for the quick review, and sorry for the HTML and for top posting.

Just letting know that I'm on vacation next week. I'll look at the comments when back on Sept 1st.

Regards, Dario

-------- Messaggio originale --------
Da: Andrew Cooper
Data:22/08/2014 19:28 (GMT+01:00)
A: Dario Faggioli ,xen-devel
Cc: George Dunlap ,"Keir (Xen.org)" ,Jan Beulich
Oggetto: Re: [PATCH 1/2] x86: during boot, anticipate identifying the boot cpu

On 22/08/14 18:15, Dario Faggioli wrote:
> and provide helpers to access the core and socket IDs,
> resulting from that identification phase.
>
> Also, initialize the socket ID of all the cpus to something
> invalid (-1), rather than leaving it as 0, which is at risk
> of being confused with "this CPU is on socket 0".
>
> All this is right now relevant to credit2 only, but may be
> useful in other schedulers, or elsewhere in general.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
>  xen/arch/x86/setup.c            |    8 ++++++--
>  xen/arch/x86/smpboot.c          |    3 ++-
>  xen/include/asm-x86/processor.h |    6 ++++--
>  3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 6a814cd..f39fdf0 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1262,6 +1262,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> 
>      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();
> 
>      trap_init();
> @@ -1272,8 +1278,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 84f2d25..16a7474 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 } };

This moves a huge chunk of data from .bss to .data.  Can it not be fixed
by enumerating topology on APs before setting scheduling up?

> 
>  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 a156e01..76d47de 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -213,8 +213,10 @@ extern void detect_extended_topology(struct cpuinfo_x86 *c);
> 
>  extern void detect_ht(struct cpuinfo_x86 *c);
> 
> -#define cpu_to_core(_cpu)   (cpu_data[_cpu].cpu_core_id)
> -#define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id)
> +#define cpu_to_core(_cpu)    (cpu_data[_cpu].cpu_core_id)
> +#define cpu_to_socket(_cpu)  (cpu_data[_cpu].phys_proc_id)
> +#define boot_cpu_to_core()   (boot_cpu_data.phys_core_id)
> +#define boot_cpu_to_socket() (boot_cpu_data.phys_proc_id)

This change is pointless.  Anything explicitly referencing the bsp
should just use boot_cpu_data directly, rather than obscuring the IDs
behind macros like this.

~Andrew

> 
>  unsigned int apicid_to_socket(unsigned int);
> 
>

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