Re: [PATCH] powerpc/numa: Fix topology_physical_package_id() on pSeries

[Michael Ellerman <mpe@ellerman.id.au>]

Cédric Le Goater <clg@kaod.org> writes:
> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
>
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.

I'm not convinced this is correct on any platforms :)

The node (nid) is just a number made up by Linux, so it's not a
"physical package id".

It might correspond to a "physical package" (whatever that means), on
existing skiboot, but it's not guaranteed.

The PAPR text around associativity and so on is incredibly dense, but I
think one thing that is clear is that firmware is allowed to present to
us whatever boundaries it thinks are most meaningful.

ie. if two things on one "physical package" have a meaningful distance
between them, then they might be presented to us as two nodes.

Having said that, if you look at the documentation in the kernel we
have:

	physical_package_id: physical package id of cpu#. Typically
	corresponds to a physical socket number, but the actual value
	is architecture and platform dependent.

Which is nicely vague.

But then:

	core_siblings: internal kernel map of cpu#'s hardware threads
	within the same physical_package_id.

Which is not true for us already on bare metal. And is just wrong on
modern CPUs where there's multiple non-siblings in a single
core/chip/package.

So a patch to update the documentation would be good :)

As far as what we should do in our topology code, I think we should use
the chip-id when we have it - ie. on bare metal.

It may not be exactly correct, but it's at least not filtered through
any indirection about NUMA-ness, ie. the associativity properties.

Also we've been using it for several years and I don't think we should
risk breaking anything by changing the value now.

As far as pseries, I'm still a bit dubious, but maybe using nid is
better than providing nothing, even if it is a lie.

cheers