Re: [RFC 8/8] x86_64: Support for new UV apic

[Andi Kleen <andi@firstfloor.org>]

Jack Steiner <steiner@sgi.com> writes:

>  unsigned int get_apic_id(void)
>  {
> -	return (apic_read(APIC_ID) >> 24) & 0xFFu;
> +	unsigned int id;
> +
> +	preempt_disable();
> +	id = apic_read(APIC_ID);
> +	if (uv_system_type >= UV_X2APIC)
> +		id  |= __get_cpu_var(x2apic_extra_bits);
> +	else
> +		id = (id >> 24) & 0xFFu;;
> +	preempt_enable();
> +	return id;

Really caller should have done preempt_disable(), otherwise
the value can be wrong as soon as you return.

Better probably to just WARN_ON if preemption is on

(just be careful it does not trigger in oopses and machine checks)

> +
> +DEFINE_PER_CPU(struct uv_hub_info_s, __uv_hub_info);
> +EXPORT_PER_CPU_SYMBOL(__uv_hub_info);

GPL export too?

> +
> +struct uv_blade_info *uv_blade_info;
> +EXPORT_SYMBOL_GPL(uv_blade_info);
> +
> +short *uv_node_to_blade;
> +EXPORT_SYMBOL_GPL(uv_node_to_blade);
> +
> +short *uv_cpu_to_blade;
> +EXPORT_SYMBOL_GPL(uv_cpu_to_blade);
> +
> +short uv_possible_blades;
> +EXPORT_SYMBOL_GPL(uv_possible_blades);
> +
> +/* Start with all IRQs pointing to boot CPU.  IRQ balancing will shift them. */
> +/* Probably incorrect for UV  ZZZ */

Actually it should be correct. Except for UV you likely really need a
NUMA aware irqbalanced. I used to have some old very hackish patches
to implement that in irqbalanced, but never pushed it because the
systems I was working on didn't really need it.


> +
> +static void uv_send_IPI_one(int cpu, int vector)
> +{
> +	unsigned long val, apicid;
> +	int nasid;
> +
> +	apicid = per_cpu(x86_cpu_to_apicid, cpu); /* ZZZ - cache node-local ? */

Instead of doing that it might be better to implement __read_mostly per CPU variables
(should not be very hard) 

> +static void uv_send_IPI_mask(cpumask_t mask, int vector)
> +{
> +	unsigned long flags;
> +	unsigned int cpu;
> +
> +	local_irq_save(flags);
> +	for (cpu = 0; cpu < NR_CPUS; ++cpu)
> +		if (cpu_isset(cpu, mask))
> +			uv_send_IPI_one(cpu, vector);
> +	local_irq_restore(flags);

This could disable interrupts for a long time could't it?  Really needed?


> +	bytes = sizeof(struct uv_blade_info) * uv_num_possible_blades();
> +	uv_blade_info = alloc_bootmem_pages(bytes);
> +	memset(uv_blade_info, 255, bytes);

255?  Strange poison value.

> ===================================================================
> --- linux.orig/arch/x86/kernel/Makefile	2008-03-21 15:36:35.000000000 -0500
> +++ linux/arch/x86/kernel/Makefile	2008-03-21 15:49:38.000000000 -0500
> @@ -90,7 +90,7 @@ scx200-y			+= scx200_32.o
>  ###
>  # 64 bit specific files
>  ifeq ($(CONFIG_X86_64),y)
> -        obj-y				+= genapic_64.o genapic_flat_64.o
> +        obj-y				+= genapic_64.o genapic_flat_64.o genx2apic_uv_x.o

Definitely should be a CONFIG

> @@ -418,6 +419,9 @@ static int __cpuinit wakeup_secondary_vi
>  	unsigned long send_status, accept_status = 0;
>  	int maxlvt, num_starts, j;
>  
> +	if (get_uv_system_type() == UV_NON_UNIQUE_APIC)
> +		return uv_wakeup_secondary(phys_apicid, start_rip);
> +

This should be probably factored properly (didn't Jeremy have smp_ops 
for this some time ago) so that even the default case is a call.

>  	Dprintk("Asserting INIT.\n");
>  
>  	/*
> @@ -679,7 +683,8 @@ do_rest:
>  				/* trampoline code not run */
>  				printk("Not responding.\n");
>  #ifdef APIC_DEBUG
> -			inquire_remote_apic(apicid);
> +			if (get_uv_system_type() != UV_NON_UNIQUE_APIC)
> +				inquire_remote_apic(apicid);

Dito.


-Andi