Skip to content
New issue

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

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

Already on GitHub? # to your account

[RFC] Using Clang (ThinLTO) for the default kernel in the cachyos repository #286

Closed
ptr1337 opened this issue Aug 29, 2024 · 21 comments · Fixed by #304
Closed

[RFC] Using Clang (ThinLTO) for the default kernel in the cachyos repository #286

ptr1337 opened this issue Aug 29, 2024 · 21 comments · Fixed by #304
Assignees
Labels
enhancement New feature or request

Comments

@ptr1337
Copy link
Member

ptr1337 commented Aug 29, 2024

Hi,

Im considering to use the ThinLTO for the "linux-cachyos" as default in the cachyos repository.
Clang built kernel has been pretty stabilized over the years and is also used by Google in Android as default, together with ThinLTO.

The Clang built Kernel provides as a bunch of benefits, like:

There are some outstanding issues, which needs to be discussed/fixed:

This issue is for general discussions and fiddling out the pending issue, which needs to be resolved.
Also, we should consider providing the default kernel built with GCC too, with a "linux-cachyos-gcc" variant or equal, but this will be only used for the cachyos repository.

known problematic DKMS Modules:

Benchmarking

We should benchmark the ThinLTO kernel compared to the GCC quite much.
There will be a bunch of cachyos-benchmarker tests done, as well as a bigger phoronix test suite.

Aditionally we should test using different marches, e.g v3/znver4 if there are any regressions found

@ptr1337 ptr1337 added the enhancement New feature or request label Aug 29, 2024
@ventureoo
Copy link
Member

DKMS probably will not be such a priority issue since we already mostly rely on NVIDIA/v4l2loopback modules that are already pre-built for our kernel.

@ptr1337
Copy link
Member Author

ptr1337 commented Aug 29, 2024

vmware-workstation has problems with Clang built kernels.

According log it fails because of Wstrict-prototypes . Passing to the dkms.conf of it -Wno-strict-prototypes should fix it there.

@ventureoo
Copy link
Member

Done with fixes for issues with NVIDIA 390xx modules: CachyOS/CachyOS-PKGBUILDS#318

@ventureoo ventureoo changed the title [Idea] Using Clang (ThinLTO) for the default kernel in the cachyos repository [RFC] Using Clang (ThinLTO) for the default kernel in the cachyos repository Aug 29, 2024
@ptr1337
Copy link
Member Author

ptr1337 commented Sep 1, 2024

We could also test and think about enabling kCFI and FineIBT for the kernel as default, when switching to the LLVM built kernels.
This needs to be tested as well as benchmarked.

kCFI should have a smaller performance hit compared to CFI, but it should be still present.
This would increase the security of the kernel.

@ptr1337
Copy link
Member Author

ptr1337 commented Sep 7, 2024

We could also test and think about enabling kCFI and FineIBT for the kernel as default, when switching to the LLVM built kernels. This needs to be tested as well as benchmarked.

kCFI should have a smaller performance hit compared to CFI, but it should be still present. This would increase the security of the kernel.

kCFI appears to break the NVIDIA module. So, this can not be used yet. Same for FineIBT.

@shelterx
Copy link

shelterx commented Sep 13, 2024

vmware-workstation has problems with Clang built kernels.

According log it fails because of Wstrict-prototypes . Passing to the dkms.conf of it -Wno-strict-prototypes should fix it there.

I usually patch the -Werror line away when I compile my own kernels, the warning comes from /lib/modules/version/build/scripts/Makefile.extrawarn
KBUILD_CFLAGS += -Werror=strict-prototypes

Removing it doesn't seem to cause any issues.

But for cachyos repro kernels it needs some workaround.

EDIT:
Entire post. :)

@1Naim
Copy link
Member

1Naim commented Sep 21, 2024

diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
index 6c23c6af797f..59b2e19416af 100644
--- a/scripts/Makefile.clang
+++ b/scripts/Makefile.clang
@@ -31,9 +31,7 @@ endif
 # certain optimization flags it knows it has not implemented.
 # Make it behave more like gcc by erroring when these flags are encountered
 # so they can be implemented or wrapped in cc-option.
-CLANG_FLAGS	+= -Werror=unknown-warning-option
 CLANG_FLAGS	+= -Werror=ignored-optimization-argument
 CLANG_FLAGS	+= -Werror=option-ignored
-CLANG_FLAGS	+= -Werror=unused-command-line-argument
 KBUILD_CPPFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 1d13cecc7cc7..ec97275c8852 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -12,7 +12,6 @@ KBUILD_CFLAGS += -Wundef
 KBUILD_CFLAGS += -Werror=implicit-function-declaration
 KBUILD_CFLAGS += -Werror=implicit-int
 KBUILD_CFLAGS += -Werror=return-type
-KBUILD_CFLAGS += -Werror=strict-prototypes
 KBUILD_CFLAGS += -Wno-format-security
 KBUILD_CFLAGS += -Wno-trigraphs
 KBUILD_CFLAGS += $(call cc-disable-warning,frame-address,)
@@ -68,9 +67,6 @@ KBUILD_CFLAGS += $(KBUILD_CFLAGS-y) $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
 # Prohibit date/time macros, which would make the build non-deterministic
 KBUILD_CFLAGS += -Werror=date-time
 
-# enforce correct pointer usage
-KBUILD_CFLAGS += $(call cc-option,-Werror=incompatible-pointer-types)
-
 # Require designated initializers for all marked structures
 KBUILD_CFLAGS += $(call cc-option,-Werror=designated-init)

Applying this diff to the kernel makes all found incompatible dkms modules except rtl88xxau-aircrack-dkms-git to build and install successfully.

@1Naim
Copy link
Member

1Naim commented Sep 21, 2024

Looking at https://github.com/aircrack-ng/rtl8812au, it seems that it is mainly targeting ARM/ARM64 devices. Therefore it is out of scope for us.

@ptr1337
Copy link
Member Author

ptr1337 commented Sep 22, 2024

@1Naim
We can think about this applying the patchset, when if thinlto then source + ...
This would avoid patching it also for GCC Kernels.

@1Naim
Copy link
Member

1Naim commented Sep 23, 2024

We can do a rollout first to the LTO kernels with a 2-4 week grace period before we make it the default.

@ms178
Copy link

ms178 commented Sep 24, 2024

Please don't forget the problems with the winesync-dkms module on newer LTO-Kernels (see: dell/dkms#439). This is a winesync problem though but shows when a modern LTO-Kernel is used. The following changes to winesync.c and winesync.h are needed to get it compiled properly with newer LTO-Kernels, see the following with more details: ms178/archpkgbuilds@ee01c7f

@ptr1337
Copy link
Member Author

ptr1337 commented Sep 24, 2024

Please don't forget the problems with the winesync-dkms module on newer LTO-Kernels (see: dell/dkms#439). This is a winesync problem though but shows when a modern LTO-Kernel is used. The following changes to winesync.c and winesync.h are needed to get it compiled properly with newer LTO-Kernels, see the following with more details: ms178/archpkgbuilds@ee01c7f

Thats not reproduceable on llvm18. Also, we have dropped winesync support, therefore not a thing

@ms178
Copy link

ms178 commented Sep 24, 2024

Please don't forget the problems with the winesync-dkms module on newer LTO-Kernels (see: dell/dkms#439). This is a winesync problem though but shows when a modern LTO-Kernel is used. The following changes to winesync.c and winesync.h are needed to get it compiled properly with newer LTO-Kernels, see the following with more details: ms178/archpkgbuilds@ee01c7f

Thats not reproduceable on llvm18. Also, we have dropped winesync support, therefore not a thing

As mentioned in the dkms issue, it is also relevant for NTSync although with a different modpost error. Please keep it in mind as sooner or later it might become a problem with LLVM-19+.

@ptr1337
Copy link
Member Author

ptr1337 commented Sep 25, 2024

Please don't forget the problems with the winesync-dkms module on newer LTO-Kernels (see: dell/dkms#439). This is a winesync problem though but shows when a modern LTO-Kernel is used. The following changes to winesync.c and winesync.h are needed to get it compiled properly with newer LTO-Kernels, see the following with more details: ms178/archpkgbuilds@ee01c7f

Thats not reproduceable on llvm18. Also, we have dropped winesync support, therefore not a thing

As mentioned in the dkms issue, it is also relevant for NTSync although with a different modpost error. Please keep it in mind as sooner or later it might become a problem with LLVM-19+.

That is neither an issue, since NTSync is shipped inside the kernel.

@1Naim
Copy link
Member

1Naim commented Sep 25, 2024

Please don't forget the problems with the winesync-dkms module on newer LTO-Kernels

Well, the winesync module hasn't been updated for 2 years now. It is very much deprecated and abandoned at this point. We also do not ship it anymore as mentioned by @ptr1337.

it is also relevant for NTSync although with a different modpost error

We will tackle it once Arch has updated their LLVM toolchain. I'm sure it will cause build problems when that happens if there are no updates from upstream we need to patch it.

@ptr1337
Copy link
Member Author

ptr1337 commented Sep 27, 2024

Benchmarked Full vs ThinLTO:
image

All in all, we can likely keep ThinLTO

@TheWyn
Copy link

TheWyn commented Sep 27, 2024

Why not go all the way with full lto?

@1Naim
Copy link
Member

1Naim commented Sep 28, 2024

kernel_version_comparison_All
ThinLTO is marginally better.

edit: You can also say thay its margin of error. In that case, there would also be no reason to use Full LTO.

@TheWyn
Copy link

TheWyn commented Sep 28, 2024

Oh I see

kernel_version_comparison_All

@ptr1337
Copy link
Member Author

ptr1337 commented Sep 28, 2024

From my side, I think we are mostly ready for this. We could start rolling this out on the Zen4 Repository first, and then proceed over with the v4/v3 repository after around one week (maybe with 6.11.2)

@1Naim
Copy link
Member

1Naim commented Sep 28, 2024

IMO, we should target for 6.11.2 for all repos. Either that or wait a few days after ISO release then push to znver4.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants