Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cluster_cpus_list to detect clusters for ARM #240

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

prashanthswami
Copy link
Contributor

On newer kernels (6.x+) a separate topology node exists to express the list of cpus associated with each cluster. The cluster topology node explicitly refers to cores that share some resources (e.g. cache) as opposed to 'core_siblings', which only indicate they are on the same physical package.

This distinction is relevant on 6.x, where some devices may have all their cores on a single physical package but cores do not all share the same resources on that package. In these cases, the existing logic incorrectly reports that there is a single cluster (because all cores are siblings), even though not all cores share the same resources.

Test: Verified on ARM64 Android device that has multiple clusters, where core_siblings reports all cores are siblings.
Test: Added mock hardware tests, verified pass on ARM64 hardware.

On newer kernels (6.x+) a separate topology node exists to express the
list of cpus associated with each cluster. The cluster topology node
explicitly refers to cores that share some resources (e.g. cache) as
opposed to 'core_siblings', which only indicate they are on the same
physical package.

This distinction is relevant on 6.x, where some devices may have all
their cores on a single physical package but cores do not all share the
same resources on that package. In these cases, the existing logic
incorrectly reports that there is a single cluster (because all cores
are siblings), even though not all cores share the same resources.

Test: Verified on ARM64 Android device that has multiple clusters, where
core_siblings reports all cores are siblings.
Test: Added mock hardware tests, verified pass on ARM64 hardware.
@digantdesai
Copy link
Contributor

cc @kimishpatel FYI

@digantdesai
Copy link
Contributor

digantdesai commented Jul 10, 2024

LGTM, thanks.

@digantdesai digantdesai merged commit 2ceea8a into pytorch:main Jul 10, 2024
11 checks passed
@prashanthswami
Copy link
Contributor Author

For posterity, here's the commit that introduced cluster_cpus. I should stress that this didn't really "replace" core_sibilings_list, so much as it "created a more specific node that identifies a true 'cluster'". We were using core_siblings_list before as a proxy, but if you take a look at the implementation you'll see that it's simply checking to see if they are on the same package and then marks them as a core sibling. There's no explicit guarantee that they share an internal system bus (though one might imagine that there are probably many examples of SoCs that have N-cores in a single cluster, on a single package).

That is why that we also retain the fallback path - if we cannot use the ACPI-defined clusters, at worst we'll still mirror the behavior we've always had and just assume we're on a chipset where the cores are in a single cluster as before.

@digantdesai
Copy link
Contributor

Thank you @prashanthswami, very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants