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

Install numa package in rootfs #12144

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Install numa package in rootfs #12144

merged 1 commit into from
Jan 11, 2023

Conversation

am11
Copy link
Member

@am11 am11 commented Jan 10, 2023

Without this installed, NUMASupportInitialize() on arm{64} platforms is skipping the initialization and directly going to false branch: https://github.com/dotnet/runtime/blob/a10ee9ec8edaae541f074a77474cf6c2d1ffc56f/src/coreclr/gc/unix/gcenv.unix.cpp#L333 (HAVE_NUMA_H is 0)

@am11
Copy link
Member Author

am11 commented Jan 10, 2023

@ayakael FYI. I noticed that we don't install it in aports either: https://github.com/alpinelinux/aports/blob/4a065d2a45541825bbd6cf22653df2fc806f3ba3/community/dotnet7-build/APKBUILD.

libnuma is a soft dependency: when available, use it otherwise use a fallback implementation. But in order to achieve that behavior, it has to be installed on the build machine. Otherwise, only the fallback code is compiled and then it would make no difference on .NET processes, if user choose to install numactl-dev package on their machine.

It is better to add it to the build dependencies list. Then it is up to the consumer of dotnet package if they want to install numa package.

@ayakael
Copy link
Contributor

ayakael commented Jan 10, 2023

@ayakael FYI. I noticed that we don't install it in aports either: https://github.com/alpinelinux/aports/blob/4a065d2a45541825bbd6cf22653df2fc806f3ba3/community/dotnet7-build/APKBUILD.

libnuma is a soft dependency: when available, use it otherwise use a fallback implementation. But in order to achieve that behavior, it has to be installed on the build machine. Otherwise, only the fallback code is compiled and then it would make no difference on .NET processes, if user choose to install numactl-dev package on their machine.

It is better to add it to the build dependencies list. Then it is up to the consumer of dotnet package if they want to install numa package.

Many thanks for the heads up. I'll include this with the 7.0.102 and 6.0.113 update.

@am11
Copy link
Member Author

am11 commented Jan 10, 2023

@jkotas, PTAL.

@jkotas jkotas requested a review from janvorli January 10, 2023 23:55
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@janvorli janvorli merged commit 2561b84 into dotnet:main Jan 11, 2023
jkotas added a commit that referenced this pull request Jan 12, 2023
jkotas added a commit that referenced this pull request Jan 12, 2023
am11 added a commit to am11/arcade that referenced this pull request Jan 13, 2023
janvorli pushed a commit that referenced this pull request Jan 13, 2023
* Revert "Revert "Install numa package in rootfs (#12144)" (#12191)"

This reverts commit 12b42d0.

* Erase numactl-dev from Alpine 3.13 arm

* Fix shellcheck warning about string replacement
https://www.shellcheck.net/wiki/SC2001

* Add version check

* Simplify
@am11
Copy link
Member Author

am11 commented Sep 29, 2023

I'll include this with the 7.0.102 and 6.0.113 update.

@ayakael, note that we have removed this dependency from .NET 8+ dotnet/runtime@de5a04b, so you can drop it when the newer version is prepared. It will tune to system's NUMA node configurations out-of-the-box. 🙂

@am11 am11 deleted the feature/numa branch September 29, 2023 14:15
@am11
Copy link
Member Author

am11 commented Feb 2, 2024

@ayakael, just a reminder that we removed this dependency in .NET 8 so feel free to delete this line https://github.com/alpinelinux/aports/blob/12aaee3b2df187077ee3049db4f1399cbac1277e/community/dotnet8-runtime/APKBUILD#L86 🙂

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

Successfully merging this pull request may close these issues.

4 participants