On 12/02/2020 11:59, Sudeep Holla wrote:
On Wed, Feb 12, 2020 at 11:20:12AM +0000, John Garry wrote:
Hi Jeremy,
Hi Sudeep,
I have a question about $subject for you, since you wrote the code.
This function returns a unique identifier for the package, but would not be
the logically indexed package id we would expect, like 0, 1, 2, ...
Firstly, it must be physical socket number and not logical id.
That's really what I meant.
It returns of the offset in the PPTT of the topology physical CPU node.
Yes, intentionally. We don't want to generate a logical index for this.
Simply not going to happen as we can't guarantee unique number always.
We need to get that uniqueness from the firmware and hence the choice of
offset. Remember that the offset is used only if firmware conveniently
ignored all the optional properties including UID in the processor
container representing the physical socket.
So I may get something like this:
john@ubuntu:~$ more
/sys/devices/system/cpu/cpu80/topology/physical_package_id
5418
Good, now the platform have a reason to fix it in the firmware if it is
very hard to see and understand the above value.
For sure, this does not violate the ABI in
Documentation/ABI/testing/sysfs-devices-system-cpu:
Very good to see you are not disagreeing with that :)
"physical_package_id: physical package id of cpu#. Typically corresponds to
a physical socket number, but the actual value is architecture and platform
dependent."
Question: Is there any reason for which we cannot assign an indexed package
id to each package node?
Yes, as mentioned above. We are not going to do extra work for lazy firmware.
I don't think it's reasonable to just label this as lazy. The table may
just not have the flag set unintentionally. FW and software guys make
mistakes, like the mistakes in PPTT, itself.
Linux also will be lazy on such platform and provide weird unique numbers
like in the above case you have mentioned.
Personally I think that the kernel can be do better than provide
meaningless values like this, since it knows the processor IDs and which
physical package they belong to.
If not, at least make the user know of potential deficiencies in the table.
Some userspace tools rely on a sane meaningful package id, like perf:
Good that you mention now. Time to update the firmware then.
[...]
This can only deal with a socket id which fits in a byte. I'd rather not
change this code if possible.
Agreed, add UID to the processor container, job done.
Thanks,
John
