Bug 142434 - [patch] Add cpuset(1) support to rc.subr(8)
Summary: [patch] Add cpuset(1) support to rc.subr(8)
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: conf (show other bugs)
Version: 7.2-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Kyle Evans
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-01-07 18:40 UTC by Miroslav Lachman
Modified: 2024-01-10 03:21 UTC (History)
3 users (show)

See Also:
kevans: mfc-stable13?


Attachments
file.diff (2.14 KB, patch)
2010-01-07 18:40 UTC, Miroslav Lachman
no flags Details | Diff
cpuset_rc.subr_72.patch (2.12 KB, patch)
2010-01-07 18:58 UTC, Miroslav Lachman
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miroslav Lachman 2010-01-07 18:40:00 UTC
Add support for cpuset(1) to rc.subr
If ${name}_cpuset is specified, command will be runned on specified CPUs.

cpuset is available in STABLE branch for more than one year (7.1+), but is still not widely known / used and there is no general support to use cpuset in rc scripts.
FreeBSD can benefit from feature like this compared to other OSes.

Fix: Apply attached patch and try something like this in /etc/rc.conf

sshd_enable="YES"
sshd_cpuset="0"
mysql_enable="YES" 
mysql_cpuset="1-3" 
lighttpd_enable="YES" 
lighttpd_cpuset="2" 
proftpd_enable="YES" 
proftpd_cpuset="1,2" 


Then after boot or restart of services, you will have services assigned to defined CPUs like this:

# /usr/local/etc/rc.d/mysql-server status
mysql is running as pid 11952.
on CPU(s) 1,2,3

# /usr/local/etc/rc.d/lighttpd status
lighttpd is running as pid 12011.
on CPU(s) 2

# /usr/local/etc/rc.d/proftpd status
proftpd is running as pid 11882.
on CPU(s) 1,2


The patch is not fully tested, but allows most currently available services to be assigned to CPUs without modification of existing rc scripts.

Note: some complex rc scripts can not use cpuset, for example rc.d/jail

patch is for 9-CURRENT but should work on older releases too

Patch attached with submission follows:
Comment 1 Miroslav Lachman 2010-01-07 18:58:46 UTC
Attached patch is for 7.2-RELEASE
Comment 2 Mark Linimon freebsd_committer freebsd_triage 2010-01-07 19:15:18 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-rc

Over to maintainer(s).
Comment 3 Miroslav Lachman 2011-04-26 12:24:19 UTC
Can some committer reply to this PR if there are any interest to have 
this feature in base rc.subr for 9.0 RELEASE?
Should I change something in the patch?

Otherwise this PR can be closed if there is some reason not to have it.

Miroslav Lachman
Comment 4 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 07:59:09 UTC
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped
Comment 5 Kyle Evans freebsd_committer freebsd_triage 2021-12-09 04:06:51 UTC
Take. I like this idea.
Comment 6 Kyle Evans freebsd_committer freebsd_triage 2021-12-09 04:19:30 UTC
I know you wrote this a long time ago now, but do you happen to have some context for this?

> +	# fix for background-fsck problem / check if value start with number

I don't immediately see what the background-fsck problem might be, unless it's specifically referring to the -x check with the problem being that background fsck is run prior to /usr/bin being mounted.
Comment 7 Miroslav Lachman 2021-12-09 18:32:47 UTC
(In reply to Kyle Evans from comment #6)
I am not sure of the detail but I know there was some problem in rc scripts or in rc.subr routines with a dash in the value of the variable name="background-fsck" in rc.d/bgfsck. Then it breaks execution of rc script, cpuset call or something around. I don't remember it well.
https://cgit.freebsd.org/src/tree/etc/rc.d/bgfsck?h=stable/10
name="background-fsck"
rcvar="background_fsck"
start_cmd="bgfsck_start"

As I see in newer versions, it was renamed to name="background_fsck" (underscore instead of dash)
https://cgit.freebsd.org/src/tree/libexec/rc/rc.d/bgfsck
name="background_fsck"
desc="Run fsck in background"
rcvar="background_fsck"
start_cmd="bgfsck_start"

I don't know if there was / is any other rc.d script with the "bad" dash in value of variable "name".
Comment 8 Miroslav Lachman 2021-12-09 18:46:22 UTC
(In reply to Kyle Evans from comment #6)
I search the archives and found that I specifically asked the question about "background-fskc" on freebsd-rc@ mailinglist prior to my first post with patch to add cpuset support. But my question in unanswered 11 years.

https://lists.freebsd.org/pipermail/freebsd-rc/2010-January/001814.html

https://lists.freebsd.org/pipermail/freebsd-rc/2010-January/001816.html

----------- start of old post
bgfsck is the only one script using hyphen in the name and it is causing 
some problems with eval:

debug output for rc.d/bgfsck

+ eval '_chdir=$background-fsck_chdir' '_chroot=$background-fsck_chroot' 
'_nice=$background-fsck_nice' '_user=$background-fsck_user' 
'_group=$background-fsck_group' '_groups=$background-fsck_groups'
+ _chdir=-fsck_chdir _chroot=-fsck_chroot _nice=-fsck_nice 
_user=-fsck_user _group=-fsck_group _groups=-fsck_groups


debug output for rc.d/sshd

+ eval '_chdir=$sshd_chdir' '_chroot=$sshd_chroot' '_nice=$sshd_nice' 
'_user=$sshd_user' '_group=$sshd_group' '_groups=$sshd_groups'
+ _chdir='' _chroot='' _nice='' _user='' _group='' _groups=''

I think that variables should be empty in both cases. Am I wrong?
----------- end of old post

As you can see there were some "-fsck" values in paces where it should be empty.

I think it is now fixed by renaming "background-fsck" to "background_fsck"
Comment 9 Miroslav Lachman 2022-10-16 20:20:57 UTC
Any news on this? Will it ever be implemented in base?
Comment 10 Graham Perrin freebsd_committer freebsd_triage 2022-10-17 12:36:15 UTC
Keyword: 

    patch
or  patch-ready

– in lieu of summary line prefix: 

    [patch]

* bulk change for the keyword
* summary lines may be edited manually (not in bulk). 

Keyword descriptions and search interface: 

    <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>
Comment 11 Enji Cooper freebsd_committer freebsd_triage 2023-05-11 04:19:33 UTC
Other variables exist in rc.subr:
```
 775 #       ${name}_env     n       Environment variables to run ${command} with.
 776 #
 777 #       ${name}_env_file n      File to source variables to run ${command} with.
 778 #
 779 #       ${name}_fib     n       Routing table number to run ${command} with.
 780 #
 781 #       ${name}_nice    n       Nice level to run ${command} at.
 782 #
 783 #       ${name}_oomprotect n    Don't kill ${command} when swap space is exhausted.
```
This seems like a straightforward ask. I'll check with kylev and see what I can do.
Comment 12 Kyle Evans freebsd_committer freebsd_triage 2023-05-11 04:30:49 UTC
(In reply to Enji Cooper from comment #11)

Sorry, forgot about this. Looking at the patch again and previous responses, I think I'm happy to land this as-is, I'm just going to remove the comment about "fix for background-fsck problem" -- probably replace it with "Loose validation of the configured cpu set"
Comment 13 commit-hook freebsd_committer freebsd_triage 2023-05-11 04:41:04 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=0661f93892a2564a64c5650ffffa3d73417172a9

commit 0661f93892a2564a64c5650ffffa3d73417172a9
Author:     Miroslav Lachman <000.fbsd_quip.cz>
AuthorDate: 2023-05-11 04:38:23 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2023-05-11 04:40:18 +0000

    rc: add support for cpuset(1)

    If ${name}_cpuset is specified (and /usr is mounted), cpuset(1) will be
    run to limit the service to the configured cpuset.

    PR:             142434
    Reviewed by:    kevans

 libexec/rc/rc.subr | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)
Comment 14 Miroslav Lachman 2023-05-11 11:57:59 UTC
13 years after my initial patch submission :) Thank you for taking care of it!

I noticed you committed with my debugging code + comment.

   # for cpuset debug only, not committable (cut)
   if [ -n "$_cpuset" -a -x $CPUSET ]; then
       echo -n "on CPU(s)"
       $CPUSET -g -p "$rc_pid" | cut -s -d: -f 2
   fi

I think this information is really useful for "status" and should be there, but I was not sure if it is OK to use pipe and cut (utilities from /usr/bin). Maybe it is OK because now I see "kldstat -v | egrep -q -e" elsewhere in rc.subr. 
(I think egrep is not recommended and should be grep -E for compatibility)
Back to topic - if it is good to have this code in rc.subr, please remove the comment line: 
"# for cpuset debug only, not committable (cut)"


And I would like to ask if anyone has tested it with the current rc.d/jail? I remember it did not work with jail 13 years ago, but a lot has changed in that time. I don't know if it will work or if it will just silently not set cpuset without breaking anything or jail will not start at all. Just to be on the safe side.