Re: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology.

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Wed, Nov 01, 2017 at 02:29:26PM -0600, Al Stone wrote:
> On 10/20/2017 03:22 AM, Lorenzo Pieralisi wrote:
> > On Thu, Oct 19, 2017 at 11:54:22AM -0500, Jeremy Linton wrote:
> > 
> > [...]
> > 
> >>>> +			cpu_topology[cpu].core_id   = topology_id;
> >>>> +			topology_id = setup_acpi_cpu_topology(cpu, 2);
> >>>> +			cpu_topology[cpu].cluster_id = topology_id;
> >>>> +			topology_id = setup_acpi_cpu_topology(cpu, max_topo);
> >>>
> >>> If you want a package id (that's just a package tag to group cores), you
> >>> should not use a large level because you know how setup_acpi_cpu_topology()works, you should add an API that allows you to retrieve the package id
> >>> (so that you can use th ACPI_PPTT_PHYSICAL_PACKAGE flag consistenly,
> >>> whatever it represents).
> >>
> >> I don't think the spec requires the use of PHYSICAL_PACKAGE... Am I
> >> misreading it? Which means we need to "pick" a node level to
> >> represent the physical package if one doesn't exist...
> > 
> > The specs define a means to detect if a given PPTT node corresponds to a
> > package (I am refraining from stating again that to me that's not clean
> > cut what a package is _architecturally_, I think you know my POV by now)
> > and that's what you need to use to retrieve a packageid for a given cpu,
> > if I understand the aim of the physical package flag.
> > 
> > Either that or that flag is completely useless.
> > 
> > Lorenzo
> > 
> > ACPI 6.2 - Table 5-151 (page 248)
> > Physical package
> > -----------------
> > Set to 1 if this node of the processor topology represents the boundary
> > of a physical package, whether socketed or surface mounted.  Set to 0 if
> > this instance of the processor topology does not represent the boundary
> > of a physical package.
> > 
> 
> I've been following the discussion and I'm not sure I understand what the
> confusion is around having a physical package ID.  Since I'm the one that
> insisted it be in the spec, I'd be glad to clarify anything.  My apologies
> for not saying anything sooner but things IRL have been very complicated
> of late.
> 
> What was intended was a simple flag that was meant to tell me if a CPU ID
> (this could be a CPU, a cluster, a processor container -- I don't really
> care which) is *also* an actual physical device on a motherboard.  That is
> the only intent; there was no architectural meaning intended at all -- that
> is what the PPTT structures are for, in conjunction with any DSDT information
> uncovered later in the boot process.
> 
> However, in the broader server ecosystem, this can be incredibly useful.  There
> are a significant number of software products sold or supported that base their
> fees on the number of physical sockets in use.  There have been in the past (and
> may be in the near future) machines where the cost of the lease on the machine
> is determined by how many physical sockets (or even CPUs) are in use, even if
> the machine has many more available.
> 
> Some vendors also include FRU (Field Replaceable Unit) location info in their
> ACPI tables.  So, for example, one or more CPUs or caches might fail in one
> physical package, which is then reported to a maintenance system of some sort
> that tells some human which of the physical sockets on what motherboard needs a
> replacement device, or it's simply noted and shut off until it's time to replace
> the entire server, or perhaps it's logged and used in an algorithm to predict
> when the server might fail completely.
> 
> So, that's why the flag exists in the spec.  It seems to make sense to me to
> have a package ID as part of struct cpu_topology -- it might even be really
> handy for CPU hotplug.  If you don't, it seems to me a whole separate struct
> would be needed with more cpumasks to show who belongs to what physical package;
> that might be okay but seems unnecessarily complicated to me.
> 
> You can also tell me that I have completely missed the point of the discussion
> so far :-).  But if you do, you have to tell me what I missed.
> 
> Hope this helps clarify...

Hi Al,

yes it does.

I think we agree that package ID has a HW/architectural meaning on x86,
it has none on PPTT (ie it totally depends on how PPTT is enumerated).

That's the first remark. So, if the package flag is used to group CPUs
and provide the topology package hierarchy to the kernel/userspace fine,
if it is to be used to provide scheduler/userspace with an ID that can
identify a HW "component" of sorts it is not fine because the topology
package ID is a SW construction on ARM systems relying on PPTT
(and DT - by the way).

So, to group CPUs and call them a package, fine by me (with a hope
FW developers won't play too much with that package flag to make
things work but use it consistenly instead).

Having said that, all I asked is that, given that we _know_ (thanks
to the PPTT flag) the package boundary, let's use it to initialize
the topology package level and that's where this patch series should
stop IMHO.

For the time being, I see no point in adding another arbitrary topology
level (ie COD) with no architectural meaning, as I said, this is vague
enough already and there is legacy (and DT systems) to take into account
too.

Thanks,
Lorenzo