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

restrict clang defconfigs #340

Merged
merged 1 commit into from
Mar 25, 2020
Merged

Conversation

nickdesaulniers
Copy link
Contributor

With this commit:
$ ./kci_build list_kernel_configs --kdir=
--config=next | grep clang | sort
arm64 allmodconfig clang-9
arm64 allnoconfig clang-9
arm64 defconfig clang-9
arm allmodconfig clang-9
arm allnoconfig clang-9
arm aspeed_g5_defconfig clang-9
arm multi_v5_defconfig clang-9
arm multi_v7_defconfig clang-9
x86_64 allmodconfig clang-9
x86_64 allnoconfig clang-9
x86_64 defconfig clang-9

Before this commit, we were testing 113 targets with Clang, including
broken fragments like x86_64_defconfig+kernel/configs/kvm_guest.config
and most ARM configs. The above listing gives us basic coverage of
ARMv{5|6|7|8} and x86_64.

With this commit:
$ ./kci_build list_kernel_configs --kdir=<linux kernel dir> \
  --config=next | grep clang | sort
arm64 allmodconfig clang-9
arm64 allnoconfig clang-9
arm64 defconfig clang-9
arm allmodconfig clang-9
arm allnoconfig clang-9
arm aspeed_g5_defconfig clang-9
arm multi_v5_defconfig clang-9
arm multi_v7_defconfig clang-9
x86_64 allmodconfig clang-9
x86_64 allnoconfig clang-9
x86_64 defconfig clang-9

Before this commit, we were testing 113 targets with Clang, including
broken fragments like x86_64_defconfig+kernel/configs/kvm_guest.config
and most ARM configs. The above listing gives us basic coverage of
ARMv{5|6|7|8} and x86_64.
@kees
Copy link
Contributor

kees commented Mar 17, 2020

Why the reduction? Is there a problem with the 113 target coverage? (Too noisy? Too slow?)

@nickdesaulniers
Copy link
Contributor Author

  1. The fragment merging is broken: https://patchwork.kernel.org/patch/11084747/
  2. There's a lot of duplicate coverage between all of the arm devices configs.
  3. I'd rather put those build resources towards the mainline and stable trees with a more limited set of configs (rather than one arch one tree many configs dominating).
  4. It was surprising to me when I enabled arm that many many device configs were tested, not just the (or a single) base defconfig.

If build resources aren't an issue, then we can leave it, but I'd rather spend time that reclaimed time elsewhere. We can always revisit turning on everything later, too.

@broonie
Copy link
Member

broonie commented Mar 18, 2020

The build resources for anything that isn't an allconfig aren't particularly concerning - they are all tiny in comparison with the allconfigs.

@gctucker gctucker requested a review from a team March 18, 2020 16:08
@gctucker
Copy link
Contributor

We need to build what makes sense and not build what is not useful, then extend our build capacity if we can't build what we need to build. So I think this filtering is a good approach.

Fine-tuning what we build means we get faster turn-around, less bandwidth and storage space wasted, less noise in the results and less energy wasted.

@gctucker
Copy link
Contributor

  1. The fragment merging is broken: https://patchwork.kernel.org/patch/11084747/

@nickdesaulniers Do you or someone else working on Clang want to address this issue upstream?

The plan from KernelCI's point of view is now to use an alternative method to merge config fragments and not rely on merge_config.sh. Still it would be better to have a solution around Make that would just work upstream, it just doesn't seem that trivial to me to implement.

@nickdesaulniers
Copy link
Contributor Author

It will be easier to fix merge_scripts.sh once this patch I sent yesterday lands: https://lore.kernel.org/lkml/20200317215515.226917-1-ndesaulniers@google.com/T/#u
(updates building with LLVM to not pass a ton of command line arguments)

The issue with merge_configs.sh is that it would have to pass up to 12 command line arguments to make (one for each LLVM utility, ex CC=clang LD=ld.lld AS=clang, etc).

Rather than set a bunch of env vars to control the compiler, I'd like to modify KernelCI to just invoke make LLVM=y. Then it's a 3 line change to merge_config.sh, vs a potentially larger one.

I'm happy to revisit merge_config.sh but it will be simpler to do so once llvm builds are simplified a bit.

Separate notes to self:
Oh, looks like merge_config.sh has a -m flag, that doesn't invoke make (up to the caller to invoke make). That may be simpler even for clang support.

Looks like merge_scripts.sh just runs cat in a loop, then make alldefconfig or make allnoconfig with the concatenated configs as an argument KCONFIG_ALLCONFIG=<file>, finally doing some validation.

Copy link
Contributor

@gctucker gctucker left a comment

Choose a reason for hiding this comment

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

OK thanks, that's all good to go for now.

@nickdesaulniers Please keep us posted if building config fragments with clang gets fixed upstream.

arm:
base_defconfig: 'multi_v7_defconfig'
filters:
- regex: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you don't actually need the { } brackets when defining a dictionary on multiple lines. But that's just cosmetics so not a blocker, and this file should be reviewed and cleaned up a bit anyway as things keep getting added to it.

@gctucker gctucker merged commit 8ada0a6 into kernelci:master Mar 25, 2020
@gctucker gctucker deleted the limited_targets branch March 25, 2020 10:38
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.

5 participants