Re: [patch V2 11/28] x86/topology: Create logical package id

[Thomas Gleixner <tglx@linutronix.de>]

Andi,

On Mon, 22 Feb 2016, Andi Kleen wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> > +	if (c->cpuid_level >= 0x00000001) {
> > +		u32 eax, ebx, ecx, edx;
> > +
> > +		cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
> 
> Use cpuid_edx()

That does not give me EBX ...
 
> > +		/*
> > +		 * If HTT (EDX[28]) is set EBX[16:23] contain the number of
> > +		 * apicids which are reserved per package. Store the resulting
> > +		 * shift value for the package management code.
> > +		 */
> > +		if (edx & (1U << 28))
> > +			c->x86_coreid_bits = get_count_order((ebx >> 16) & 0xff);
> > +	}
> > +++ b/arch/x86/kernel/cpu/proc.c
> > @@ -12,6 +12,7 @@ static void show_cpuinfo_core(struct seq
> >  {
> >  #ifdef CONFIG_SMP
> >  	seq_printf(m, "physical id\t: %d\n", c->phys_proc_id);
> > +	seq_printf(m, "logical id\t: %d\n", c->logical_proc_id);
> 
> 
> I'm not sure it makes sense to export this. What good would it be for
> the user?
>
> If it was it would need to be documented somewhere. But I would
> just drop it and keep it kernel internal.

You are right. We print already when we change the package number, so it can
be retrieved from dmesg.
 
> FWIW every time something is added to this file it usually breaks
> some (dumb) programs.

Ok, did not think about that.
 
> > +	/*
> > +	 * Today neither Intel nor AMD support heterogenous systems. That
> > +	 * might change in the future....
> > +	 */
> > +	ncpus = boot_cpu_data.x86_max_cores * smp_num_siblings;
> > +	__max_logical_packages = DIV_ROUND_UP(nr_cpu_ids, ncpus);
> 
> FWIW Hypervisors can do nearly everything today.
> 
> I assume your code handles it.

It should. At least as long as nr_cpu_ids is sufficiently large.
 
> Let's hope that the Hypervisors always set up the correct CPUID now
> for their sibling configuration. If they don't with this change
> some CPUs would be suddenly lost.

The ones I looked at are doing is sane. Famous last words :)
 
> Would it be worth to have a kernel option where the maximum can be overriden
> in case this happens?

Let's wait for it to happen. It's done in no time, but if not needed it's just
ballast.

Thanks,

	tglx