Re: [Xen-devel] [PATCH] x86: clean up physid_mask_t handling

[Keir Fraser <keir@xxxxxxx>]

On 14/10/2011 09:57, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> This eliminates passing and returning by value of this type (making it
> unnecessary for the compiler to create temporary variables for doing so
> on the stack), thus dramatically reducing the stack frame sizes of a
> couple of functions (was in one case over 12k for a 4095-CPU build).
> 
> In one case a local variable gets converted to a static one, possible
> because the function gets called at most once (during early boot).
> 
> Some accessors get eliminated altogether as being unused or as being
> equally well open coded at the place of use.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Keir Fraser <keir@xxxxxxx>

> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1464,7 +1464,8 @@ int __init APIC_init_uniprocessor (void)
>  #ifdef CONFIG_CRASH_DUMP
>      boot_cpu_physical_apicid = get_apic_id();
>  #endif
> -    phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
> +    physids_clear(phys_cpu_present_map);
> +    physid_set(boot_cpu_physical_apicid, phys_cpu_present_map);
>  
>      setup_local_APIC();
>  
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1463,7 +1463,7 @@ void disable_IO_APIC(void)
>  static void __init setup_ioapic_ids_from_mpc(void)
>  {
>      union IO_APIC_reg_00 reg_00;
> -    physid_mask_t phys_id_present_map;
> +    static physid_mask_t __initdata phys_id_present_map;
>      int apic;
>      int i;
>      unsigned char old_id;
> @@ -1481,7 +1481,7 @@ static void __init setup_ioapic_ids_from
>       * This is broken; anything with a real cpu count has to
>       * circumvent this idiocy regardless.
>       */
> -    phys_id_present_map = ioapic_phys_id_map(phys_cpu_present_map);
> +    ioapic_phys_id_map(&phys_id_present_map);
>  
>      /*
>       * Set the IOAPIC ID to the value stored in the MPC table.
> @@ -1508,7 +1508,7 @@ static void __init setup_ioapic_ids_from
>           * system must have a unique ID or we get lots of nice
>           * 'stuck on smp_invalidate_needed IPI wait' messages.
>           */
> -        if (check_apicid_used(phys_id_present_map,
> +        if (check_apicid_used(&phys_id_present_map,
>                                mp_ioapics[apic].mpc_apicid)) {
>              printk(KERN_ERR "BIOS bug, IO-APIC#%d ID %d is already
> used!...\n",
>                     apic, mp_ioapics[apic].mpc_apicid);
> @@ -1519,17 +1519,13 @@ static void __init setup_ioapic_ids_from
>                  panic("Max APIC ID exceeded!\n");
>              printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
>                     i);
> -            physid_set(i, phys_id_present_map);
>              mp_ioapics[apic].mpc_apicid = i;
>          } else {
> -            physid_mask_t tmp;
> -            tmp = apicid_to_cpu_present(mp_ioapics[apic].mpc_apicid);
>              apic_printk(APIC_VERBOSE, "Setting %d in the "
>                          "phys_id_present_map\n",
>                          mp_ioapics[apic].mpc_apicid);
> -            physids_or(phys_id_present_map, phys_id_present_map, tmp);
>          }
> -
> +        set_apicid(mp_ioapics[apic].mpc_apicid, &phys_id_present_map);
>  
>          /*
>           * We need to adjust the IRQ routing table
> @@ -2195,7 +2191,6 @@ int __init io_apic_get_unique_id (int io
>  {
>      union IO_APIC_reg_00 reg_00;
>      static physid_mask_t __initdata apic_id_map = PHYSID_MASK_NONE;
> -    physid_mask_t tmp;
>      unsigned long flags;
>      int i = 0;
>  
> @@ -2209,7 +2204,7 @@ int __init io_apic_get_unique_id (int io
>       */
>  
>      if (physids_empty(apic_id_map))
> -        apic_id_map = ioapic_phys_id_map(phys_cpu_present_map);
> +        ioapic_phys_id_map(&apic_id_map);
>  
>      spin_lock_irqsave(&ioapic_lock, flags);
>      reg_00.raw = io_apic_read(ioapic, 0);
> @@ -2225,10 +2220,10 @@ int __init io_apic_get_unique_id (int io
>       * Every APIC in a system must have a unique ID or we get lots of nice
>       * 'stuck on smp_invalidate_needed IPI wait' messages.
>       */
> -    if (check_apicid_used(apic_id_map, apic_id)) {
> +    if (check_apicid_used(&apic_id_map, apic_id)) {
>  
>          for (i = 0; i < get_physical_broadcast(); i++) {
> -            if (!check_apicid_used(apic_id_map, i))
> +            if (!check_apicid_used(&apic_id_map, i))
>                  break;
>          }
>  
> @@ -2241,8 +2236,7 @@ int __init io_apic_get_unique_id (int io
>          apic_id = i;
>      } 
>  
> -    tmp = apicid_to_cpu_present(apic_id);
> -    physids_or(apic_id_map, apic_id_map, tmp);
> +    set_apicid(apic_id, &apic_id_map);
>  
>      if (reg_00.bits.ID != apic_id) {
>          reg_00.bits.ID = apic_id;
> --- a/xen/arch/x86/mpparse.c
> +++ b/xen/arch/x86/mpparse.c
> @@ -89,7 +89,6 @@ static int __devinit MP_processor_info_x
> u32 apicidx, bool_t hotplug)
>  {
> int ver, apicid, cpu = 0;
> - physid_mask_t phys_cpu;
> 
> if (!(m->mpc_cpuflag & CPU_ENABLED))
> return -EINVAL;
> @@ -114,8 +113,7 @@ static int __devinit MP_processor_info_x
> }
> apic_version[apicid] = ver;
>  
> - phys_cpu = apicid_to_cpu_present(apicid);
> - physids_or(phys_cpu_present_map, phys_cpu_present_map, phys_cpu);
> + set_apicid(apicid, &phys_cpu_present_map);
>  
> if (num_processors >= NR_CPUS) {
> printk(KERN_WARNING "WARNING: NR_CPUS limit of %i reached."
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -752,7 +752,8 @@ void __init smp_prepare_cpus(unsigned in
>      {
>          printk(KERN_NOTICE "SMP motherboard not detected.\n");
>      init_uniprocessor:
> -        phys_cpu_present_map = physid_mask_of_physid(0);
> +        physids_clear(phys_cpu_present_map);
> +        physid_set(0, phys_cpu_present_map);
>          if (APIC_init_uniprocessor())
>              printk(KERN_NOTICE "Local APIC not detected."
>                     " Using dummy APIC emulation.\n");
> @@ -767,7 +768,7 @@ void __init smp_prepare_cpus(unsigned in
>       * CPU too, but we do it for the sake of robustness anyway.
>       * Makes no sense to do this check in clustered apic mode, so skip it
>       */
> -    if ( !check_phys_apicid_present(boot_cpu_physical_apicid) )
> +    if ( !check_apicid_present(boot_cpu_physical_apicid) )
>      {
>          printk("weird, boot CPU (#%d) not listed by the BIOS.\n",
>                 boot_cpu_physical_apicid);
> --- a/xen/include/asm-x86/mach-generic/mach_apic.h
> +++ b/xen/include/asm-x86/mach-generic/mach_apic.h
> @@ -58,29 +58,24 @@ static inline int apic_id_registered(voi
>    phys_cpu_present_map);
>  }
>  
> -static inline physid_mask_t ioapic_phys_id_map(physid_mask_t phys_map)
> +static inline void ioapic_phys_id_map(physid_mask_t *map)
>  {
> - return phys_map;
> + *map = phys_cpu_present_map;
>  }
>  
> -static inline unsigned long check_apicid_used(physid_mask_t bitmap, int
> apicid)
> +static inline int check_apicid_used(const physid_mask_t *map, int apicid)
>  {
> - return physid_isset(apicid, bitmap);
> + return physid_isset(apicid, *map);
>  }
>  
> -static inline unsigned long check_apicid_present(int apicid)
> +static inline int check_apicid_present(int apicid)
>  {
> return physid_isset(apicid, phys_cpu_present_map);
>  }
>  
> -static inline int check_phys_apicid_present(int boot_cpu_physical_apicid)
> +static inline void set_apicid(int phys_apicid, physid_mask_t *map)
>  {
> - return physid_isset(boot_cpu_physical_apicid, phys_cpu_present_map);
> -}
> -
> -static inline physid_mask_t apicid_to_cpu_present(int phys_apicid)
> -{
> - return physid_mask_of_physid(phys_apicid);
> + physid_set(phys_apicid, *map);
>  }
>  
>  #endif /* __ASM_MACH_APIC_H */
> --- a/xen/include/asm-x86/mpspec.h
> +++ b/xen/include/asm-x86/mpspec.h
> @@ -50,23 +50,6 @@ typedef struct physid_mask physid_mask_t
>  #define physids_empty(map)   bitmap_empty((map).mask, MAX_APICS)
>  #define physids_equal(map1, map2)  bitmap_equal((map1).mask, (map2).mask,
> MAX_APICS)
>  #define physids_weight(map)   bitmap_weight((map).mask, MAX_APICS)
> -#define physids_shift_right(d, s, n)  bitmap_shift_right((d).mask, (s).mask,
> n, MAX_APICS)
> -#define physids_shift_left(d, s, n)  bitmap_shift_left((d).mask, (s).mask, n,
> MAX_APICS)
> -#define physids_coerce(map)   ((map).mask[0])
> -
> -#define physids_promote(physids)      \
> - ({         \
> -  physid_mask_t __physid_mask = PHYSID_MASK_NONE;   \
> -  __physid_mask.mask[0] = physids;    \
> -  __physid_mask;       \
> - })
> -
> -#define physid_mask_of_physid(physid)      \
> - ({         \
> -  physid_mask_t __physid_mask = PHYSID_MASK_NONE;   \
> -  physid_set(physid, __physid_mask);    \
> -  __physid_mask;       \
> - })
>  
>  #define PHYSID_MASK_ALL  { {[0 ... PHYSID_ARRAY_SIZE-1] = ~0UL} }
>  #define PHYSID_MASK_NONE { {[0 ... PHYSID_ARRAY_SIZE-1] = 0UL} }
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel