Page MenuHomeFreeBSD
Differential D3197

Do not compile in the really expensive entropy harvesting unless it is requested.
ClosedPublic
Actions

Authored by markm on Jul 25 2015, 1:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Nov 22 2024, 6:30 AM
Unknown Object (File)
Nov 22 2024, 4:57 AM
Unknown Object (File)
Nov 18 2024, 9:46 PM
Unknown Object (File)
Nov 18 2024, 3:11 PM
Unknown Object (File)
Nov 18 2024, 9:54 AM
Unknown Object (File)
Nov 6 2024, 5:51 PM
Unknown Object (File)
Oct 28 2024, 11:55 PM
Unknown Object (File)
Oct 28 2024, 8:34 PM
View All Files
Subscribers
imp
security

Details

Reviewers
scottl
vsevolod
jmg
delphij
imp
rwatson
Group Reviewers
manpages
secteam
Commits
rS287023: Make the UMA harvesting go away completely if not wanted. Default to "not…
Summary

High-performance folks have objected to my harvesting of entropy in the zone allocator (UMA). This work will set that harvesting to optional, with the default to "off". The code concerned is compiled out, so there are NO instructions added when it is not present. The call lines remain, but these will be null macros by default.

Folks who need this entropy may turn it on with a kernel build option, and SDT probes are set in place to assist the measurements needed in making the decision.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

markm updated this revision to Diff 7292.Jul 25 2015, 1:14 PM
markm retitled this revision from to Do do compile in the really expensive entropy harvesting unless this is requested..
markm updated this object.
markm edited the test plan for this revision. (Show Details)
markm added reviewers: scottl, rwatson, jmg, delphij, imp, vsevolod.
markm added a subscriber: security.
Herald added a subscriber: imp. · View Herald TranscriptJul 25 2015, 1:14 PM
markm added a comment.Jul 25 2015, 1:17 PM

ScottL, JeffR - this is not complete yet, but does the general idea meet your requirements?

markm updated this object.Jul 25 2015, 1:19 PM
imp added inline comments.Jul 25 2015, 3:19 PM
sys/vm/uma_core.c
2137–2140

Not entirely sure that this long comment is needed...

But if you must, you should make it a one liner:

/* Enable entropy collection for RANDOM_ENABLE_UMA kernel option */

or something. Here and below.

markm retitled this revision from Do do compile in the really expensive entropy harvesting unless this is requested. to Do not compile in the really expensive entropy harvesting unless it is requested..Jul 25 2015, 3:53 PM
markm marked an inline comment as done.
markm updated this revision to Diff 7301.Jul 25 2015, 4:02 PM

Shorten some comments.

scottl accepted this revision.Jul 25 2015, 4:02 PM
scottl edited edge metadata.
This revision is now accepted and ready to land.Jul 25 2015, 4:02 PM
imp accepted this revision.Jul 25 2015, 5:19 PM
imp edited edge metadata.

Glad you liked my suggestion.

delphij edited edge metadata.Jul 25 2015, 5:29 PM

I like the idea, but it looks like the new version doesn't have the random.h changes. If combined with the sys/conf/NOTES, sys/conf/files and sys/sys/random.h changes in the first version of changeset, then consider this as an "Accept".

jmg requested changes to this revision.Jul 25 2015, 5:55 PM
jmg edited edge metadata.

Please post a complete patch that includes both sets of changes, and adds proper documentation.

sys/conf/NOTES
2987 ↗(On Diff #7292)

Comment here is necessary for people to understand what this does.

sys/sys/random.h
93 ↗(On Diff #7292)

I don't like creating a new function for this, just for two points... putting ifdefs in the uma code to me seems better.

Also, these new functions are undocumented, and need to be documented if added.

This revision now requires changes to proceed.Jul 25 2015, 5:55 PM
markm added a comment.Jul 26 2015, 1:43 PM
In D3197#64019, @delphij wrote:

I like the idea, but it looks like the new version doesn't have the random.h changes. If combined with the sys/conf/NOTES, sys/conf/files and sys/sys/random.h changes in the first version of changeset, then consider this as an "Accept".

Oops - yes - it is both together. I messed up using arcanist. :-]

markm updated this revision to Diff 7343.Jul 26 2015, 1:49 PM
markm edited edge metadata.
  • Make the UMA harvesting go away completely if not wanted. Default to "not wanted".
  • Fix comment as per review suggestion.
markm added a comment.Jul 26 2015, 1:55 PM
This comment was removed by markm.
sys/sys/random.h
93 ↗(On Diff #7343)

I would be prepared to put in comments where these function wrappers are used, except I was just asked to remove this.

Wrapping them in ifdefs will be a nasty mess, and documenting them as if they were API functions when they are only used in one place is a waste of time.

markm marked 3 inline comments as done.Jul 26 2015, 1:58 PM
markm added inline comments.
sys/conf/NOTES
2987 ↗(On Diff #7343)

On its way. This is part of the WIP.

markm updated this revision to Diff 8037.Aug 19 2015, 9:30 AM
markm marked an inline comment as done.
markm edited edge metadata.

Massage this into a more commit-worthy version; document what is going on and provide SDT probes to assist adopters.

Herald added reviewers: manpages, secteam. · View Herald TranscriptAug 19 2015, 9:30 AM
markm updated this object.Aug 19 2015, 9:31 AM
markm edited edge metadata.
markm updated this object.Aug 19 2015, 10:13 AM
Closed by commit rS287023: Make the UMA harvesting go away completely if not wanted. Default to "not… (authored by markm). · Explain WhyAug 22 2015, 12:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
vm/
10 lines

Diff 7301

View Options

sys/vm/uma_core.c

Loading...