Allow specifying number of cores allowed via kernel config
ClosedPublic
Actions

Authored by stevek on Jun 11 2016, 7:23 PM.

Details

Reviewers

sjg
jtl

Commits

rS303355: Add the NUM_CORE_FILES kernel config option which specifies the limit for the

Summary

Add NUM_CORES option for setting num_cores value and sanity check it at compile time to ensure it is within the valid range of 0-10.

Test Plan

Test build modified GENERIC kernel configuration file with NUM_CORES option set to 11.
Also one with NUM_CORES option set to 2 booted on amd64 install in VirtualBox.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

stevek updated this revision to Diff 17520.Jun 11 2016, 7:23 PM

stevek retitled this revision from to Allow specifying number of cores allowed via kernel config.

stevek updated this object.

stevek edited the test plan for this revision. (Show Details)

stevek set the repository for this revision to rS FreeBSD src repository - subversion.

stevek added subscribers: jtl, sjg.

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptJun 11 2016, 7:23 PM

stevek updated this object.Jun 11 2016, 7:24 PM

sjg added inline comments.Jul 26 2016, 7:40 PM

sys/conf/options
68 ↗	(On Diff #17520)	I know NUM_CORES makes sense given the code in kern_sig,c but NUM_CORE_FILES might be more meaningful here?

Change option name to NUM_CORE_FILES, per review comments.

works for me

This revision is now accepted and ready to land.Jul 26 2016, 8:59 PM

When you add a kernel option, I think you also need to add something to head/sys/conf/NOTES.

This revision now requires changes to proceed.Jul 26 2016, 9:11 PM

Change MAX_NUM_CORES to MAX_NUM_CORE_FILES.
Add entry for NUM_CORE_FILES option in the NOTES file.

See nits. Approved after you address my comments however you see fit (fix, ignore, whatever :-) ).

sys/kern/kern_sig.c
3145 ↗	(On Diff #18780)	Minor style nit. I think I usually see tabs between identifiers and values. For example, see line 94, lines 192-198, line 3168, line 3185, etc. in this file. (This may indeed be a tab, and Phabricator is just not showing it that way.)
3147 ↗	(On Diff #18780)	same.
3149 ↗	(On Diff #18780)	I think the check should probably be > 0? I believe 0 and 1 will behave equivalently, and I'm not sure 0 makes logical sense. (Even if the value is 0, I believe 1 core file will be created.)

This revision is now accepted and ready to land.Jul 26 2016, 11:51 PM

stevek added inline comments.Jul 27 2016, 2:51 AM

sys/kern/kern_sig.c
3145 ↗	(On Diff #18780)	Yes, this is Phabricator messing with things, it's actually a tab.
3147 ↗	(On Diff #18780)	Same here, it's Phabricator messing with things.
3149 ↗	(On Diff #18780)	Actually, look at the code in sysctl_debug_num_cores_check (lines 3162-3165 in this review.) The value can be 0 through MAX_NUM_CORE_FILES, inclusive. If num_cores is 0, you don't get anything added for the %I expansion (as if the %I was not there.) Whereas num_cores 1 would give a 0 for the %I expansion.