Re: [PATCH] x86-64: don't use xmalloc_array() for allocation of the (per-CPU) IDTs

[Keir Fraser <keir@xen.org>]

On 13/10/2011 08:21, "Jan Beulich" <JBeulich@suse.com> wrote:

> The IDTs being exactly a page in size, using xmalloc() here is rather
> inefficient, as this requires double the amount to be allocated (with
> almost an entire page wasted). For hot plugged CPUs, this at once
> eliminates one more non-order-zero runtime allocation.
> 
> For x86-32, however, the IDT is exactly half a page, so allocating a
> full page seems wasteful here, so it continues to use xmalloc() as
> before.
> 
> With most of the affected functions' bodies now being inside #ifdef-s,
> it might be reasonable to split those parts out into subarch-specific
> code...

Well, there is also opportunity for code merging. There is generally no
reason for x86-64 to use alloc_domheap_pages where x86-32 uses
alloc_xenheap_pages (e.g., for allocating per_cpu(gdt_table)) -- both
architectures can use alloc_xenheap_pages in this case.

Also we might get rid of some further ifdef'ery if we added an alloc/free
interface where *both* functions take a size parameter. We could then decide
whether to use xmalloc or alloc_xenheap_pages dynamically based on that
parameter. Knowing size on free would allow us to easily free such
allocations too.

Finally, I'm not sure about more x86-64 code movement: I'd like to kill
x86-32 entirely really.

Anyway, despite all this...

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -639,9 +639,6 @@ static void cpu_smpboot_free(unsigned in
>  {
>      unsigned int order;
>  
> -    xfree(idt_tables[cpu]);
> -    idt_tables[cpu] = NULL;
> -
>      order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
>  #ifdef __x86_64__
>      if ( per_cpu(compat_gdt_table, cpu) )
> @@ -650,10 +647,15 @@ static void cpu_smpboot_free(unsigned in
>          free_domheap_pages(virt_to_page(per_cpu(compat_gdt_table, cpu)),
>                             order);
>      per_cpu(compat_gdt_table, cpu) = NULL;
> +    order = get_order_from_bytes(IDT_ENTRIES * sizeof(**idt_tables));
> +    if ( idt_tables[cpu] )
> +        free_domheap_pages(virt_to_page(idt_tables[cpu]), order);
>  #else
>      free_xenheap_pages(per_cpu(gdt_table, cpu), order);
> +    xfree(idt_tables[cpu]);
>  #endif
>      per_cpu(gdt_table, cpu) = NULL;
> +    idt_tables[cpu] = NULL;
>  
>      if ( stack_base[cpu] != NULL )
>      {
> @@ -691,19 +693,25 @@ static int cpu_smpboot_alloc(unsigned in
>      if ( !page )
>          goto oom;
>      per_cpu(gdt_table, cpu) = gdt = page_to_virt(page);
> +    order = get_order_from_bytes(IDT_ENTRIES * sizeof(**idt_tables));
> +    page = alloc_domheap_pages(NULL, order,
> +                               MEMF_node(cpu_to_node(cpu)));
> +    if ( !page )
> +        goto oom;
> +    idt_tables[cpu] = page_to_virt(page);
>  #else
>      per_cpu(gdt_table, cpu) = gdt = alloc_xenheap_pages(order, 0);
>      if ( !gdt )
>          goto oom;
> +    idt_tables[cpu] = xmalloc_array(idt_entry_t, IDT_ENTRIES);
> +    if ( idt_tables[cpu] == NULL )
> +        goto oom;
>  #endif
>      memcpy(gdt, boot_cpu_gdt_table,
>             NR_RESERVED_GDT_PAGES * PAGE_SIZE);
>      BUILD_BUG_ON(NR_CPUS > 0x10000);
>      gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
>  
> -    idt_tables[cpu] = xmalloc_array(idt_entry_t, IDT_ENTRIES);
> -    if ( idt_tables[cpu] == NULL )
> -        goto oom;
>      memcpy(idt_tables[cpu], idt_table,
>             IDT_ENTRIES*sizeof(idt_entry_t));
>  
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel