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

On Tue, Mar 25, 2008 at 11:25:34AM +0100, Andi Kleen wrote:
Jack Steiner <steiner@xxxxxxx> 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

Will do (assuming it doesn't ripple thru too much code eliminating
warnings - doesn't look bad at first glance).



(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?

Yes.



+
+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.

Deleted comment.



+
+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)

Added to list of loose-ends that need addressing.



+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?

No, not sure why I did that. Deleted the irq disable...




+ 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.

Deleted the memset. Should not be depending on poison values. Was useful
in debugging but it has outlived it's usefulness.



===================================================================
--- 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

Not sure that I understand why. The overhead of UV is minimal & we want UV
enabled in all distro kernels. OTOH, small embedded systems probably want to
eliminate every last bit of unneeded code.

Might make sense to have a config option. Thoughts????



@@ -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.

By factored, do you means something like:
is_uv_legacy_system()
is_us_non_unique_apicid_system()
...

Or maybe:
is_uv_system_type(x) # where x is UV_NON_UNIQUE_APIC, etc


-Andi

Thanks for the careful review.

--- jack
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/