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

interfaces/cpu-control: allow to control cpufreq tunables #8127

Merged
merged 2 commits into from
Feb 17, 2020

Conversation

alexmurray
Copy link
Collaborator

This is needed to support snaps like auto-cpufreq to use strict confinement as per https://forum.snapcraft.io/t/classic-confinement-request-for-auto-cpufreq/15386/16

Signed-off-by: Alex Murray alex.murray@canonical.com

/sys/devices/system/cpu/cpufreq/boost w,

# https://www.kernel.org/doc/html/latest/admin-guide/pm/intel_pstate.html#user-space-interface-in-sysfs
/sys/devices/system/cpu/cpufreq/intel_pstate/* r,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be /sys/devices/system/cpu/intel_pstate/

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes every file there readable and select files are made writable by the rules below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No @bboozzoo is right - I added an extra 'cpufreq' into the path which should be removed - will repush a fixed version soon.

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Looks sensible, +1

Signed-off-by: Alex Murray <alex.murray@canonical.com>
Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@zyga
Copy link
Collaborator

zyga commented Feb 13, 2020

The failure is caused by tests/main/ubuntu-core-snapd - I am working on a solution.

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! LGTM as is though there are some questions inline that may help future-proof this.

@@ -35,6 +35,24 @@ const cpuControlConnectedPlugAppArmor = `
/sys/devices/system/cpu/cpu*/online rw,
/sys/devices/system/cpu/smt/* r,
/sys/devices/system/cpu/smt/control w,

# https://www.kernel.org/doc/html/latest/admin-guide/pm/cpufreq.html#policy-interface-in-sysfs
/sys/devices/system/cpu/cpufreq/* r,

Choose a reason for hiding this comment

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

I wonder if this should be: /sys/devices/system/cpu/cpufreq/{,**} r,


# https://www.kernel.org/doc/html/latest/admin-guide/pm/cpufreq.html#policy-interface-in-sysfs
/sys/devices/system/cpu/cpufreq/* r,
/sys/devices/system/cpu/cpufreq/policy*/* r,

Choose a reason for hiding this comment

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

With the above suggestion, this rule can go away.

/sys/devices/system/cpu/cpufreq/boost w,

# https://www.kernel.org/doc/html/latest/admin-guide/pm/intel_pstate.html#user-space-interface-in-sysfs
/sys/devices/system/cpu/intel_pstate/* r,

Choose a reason for hiding this comment

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

Perhaps /sys/devices/system/cpu/intel_pstate/{,*} r, instead?

@zyga
Copy link
Collaborator

zyga commented Feb 13, 2020

This is now fixed by merging #8129

Signed-off-by: Alex Murray <alex.murray@canonical.com>
@alexmurray
Copy link
Collaborator Author

Thanks for the review @jdstrand & @zyga - please see the additional commit which includes @jdstrand's suggested changes - I agree this makes more sense to try and make all attributes readable and then only specific ones writable (and this is more future proof if other subdirectories etc are added with additional attributes).

@mvo5 mvo5 merged commit d4a45e9 into canonical:master Feb 17, 2020
@AdnanHodzic
Copy link
Contributor

AdnanHodzic commented Feb 17, 2020

Thank you everyone.

IIRC @alexmurray said these changes will be released as part of 2.44 version of snapd. Does anyone have any indication when this release will go live?

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