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

allow CONFIG_TRIM_UNUSED_KSYM to be enabled when build as a builtin-module #8820

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

twoertwein
Copy link
Contributor

Motivation and Context

When ZFS is built into the kernel, ZFS seems to work perfectly fine even when unused kernel symbols are removed.

Description

This patch allows CONFIG_TRIM_UNUSED_KSYM when enable_linux_builtin is used.

How Has This Been Tested?

Compiled 0.8.0 with this patch and used zfs (Archlinux, linux-lts).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ahrens ahrens added the Type: Building Indicates an issue related to building binaries label May 27, 2019
@codecov
Copy link

codecov bot commented May 27, 2019

Codecov Report

Merging #8820 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8820      +/-   ##
==========================================
+ Coverage   78.73%   78.77%   +0.04%     
==========================================
  Files         382      382              
  Lines      117812   117812              
==========================================
+ Hits        92758    92812      +54     
+ Misses      25054    25000      -54
Flag Coverage Δ
#kernel 79.37% <ø> (+0.03%) ⬆️
#user 67.58% <ø> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 841a7a9...dc9b7df. Read the comment docs.

If ZFS is built with enable_linux_builtin, it seems to be possible
to compile the kernel with TRIM_UNUSED_KSYM.

Signed-off-by: Torsten Wörtwein <twoertwein@gmail.com>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 28, 2019
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks, adding a buildin exclusion makes good sense. By definition when building with the rest of the kernel nothing should have been trimmed yet. Could you just verify from the config.log that the HAVE_PDE_DATA check in config/kernel-pde-data.m4 is working correctly. According to the commit which introduce this check 4b9dddf it's the reason this was required.

@twoertwein
Copy link
Contributor Author

I assume this is the relevant part of config.log:

configure:36799: checking whether PDE_DATA() is available
configure:36833: cp conftest.c conftest.h build && make modules -C /home/twoertwein/AUR/linux-lts/src/linux-4.19.45 EXTRA_CFLAGS=-Werror -Wframe-larger-than=4096   M=/home/twoertwein/AUR/linux-lts/src/zfs-zfs-0.8.0/build modpost=true
configure:36836: $? = 0
configure:36838: test -s build/conftest.o
configure:36841: $? = 0
configure:36893: result: yes

it seems to be happy :)

@twoertwein
Copy link
Contributor Author

it would probably be good to extend the automated built-in test-case (simply duplicate it but have CONFIG_TRIM_UNUSED_KSYM=y?) to check whether it introduces some unexpected behavior? "buildbot/Kernel.org Built-in x86_64 (BUILD)" seems to fail due to a git issue?

@behlendorf
Copy link
Contributor

Looks good. Yes, I think it would be good to follow up with a tweak to the CI to define CONFIG_TRIM_UNUSED_KSYM=y in our custom builtin kernel builds. But let's delay that for a short while after merging this to prevent an unnecessary CI failures due to it.

@behlendorf behlendorf requested a review from kusumi May 29, 2019 23:22
@behlendorf behlendorf merged commit 1ec7eda into openzfs:master Jun 5, 2019
behlendorf pushed a commit that referenced this pull request Jun 7, 2019
If ZFS is built with enable_linux_builtin, it seems to be possible
to compile the kernel with TRIM_UNUSED_KSYM.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Torsten Wörtwein <twoertwein@gmail.com>
Closes #8820
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested) Type: Building Indicates an issue related to building binaries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants