Permit init(8) use its own cpuset group.

[Konstantin Belousov]

On Mon, Jun 02, 2014 at 06:52:10PM +0400, Alexander V. Chernikov wrote:
> Hello list!
> 
> Currently init(8) uses group 1 which is root group.
> Modifications of this group affects both kernel and userland threads.
> Additionally, such modifications are impossible, for example, in presence
> of multi-queue NIC drivers (like igb or ixgbe) which binds their threads to
> particular cpus.
> 
> Proposed change ("init_cpuset" loader tunable) permits changing cpu 
> masks for
> userland more easily. Restricting user processes to migrate to/from CPU 
> cores
> used for network traffic processing is one of the cases.
> 
> Phabricator: https://phabric.freebsd.org/D141 (the same version attached 
> inline)
> 
> If there are no objections, I'll commit this next week.
Why is the tunable needed ? IMO it is more reasonable to create a new
cpuset always, at least I cannot explain why the existing behaviour
is useful.  If creating new cpuset unconditionally, you could consider
doing it in kernel in start_init().  You would also need to update the
cpuset(2) man page, which states that cpuset 1 is set for all processes.

Still, see comments below for the patch.

> Index: sbin/init/init.c
> ===================================================================
> --- sbin/init/init.c	(revision 266306)
> +++ sbin/init/init.c	(working copy)
> @@ -47,6 +47,8 @@ static const char rcsid[] =
>  #include <sys/param.h>
>  #include <sys/ioctl.h>
>  #include <sys/mount.h>
> +#include <sys/param.h>
> +#include <sys/cpuset.h>
>  #include <sys/sysctl.h>
>  #include <sys/wait.h>
>  #include <sys/stat.h>
> @@ -320,6 +322,19 @@ invalid:
>  			warning("Can't chroot to %s: %m", kenv_value);
>  	}
>  
> +	if (kenv(KENV_GET, "init_cpuset", kenv_value, sizeof(kenv_value)) > 0) {
> +		if (getpid() == 1) {
If you use the && operator for conditionals instead of two if()s, the nesting
level of the block can be reduced.

> +			cpusetid_t setid;
The variable must be declared at the function start.

> +
> +			setid = -1;
Why do you need to initialize the variable ?

> +			if (cpuset(&setid) != 0) {
> +				warning("cpu set alloc failed: %m");
> +			} else {
> +				if (cpuset_setid(CPU_WHICH_PID, 1, setid) != 0)
This could be else if () again to reduce indentation.

> +					warning("cpuset_setsid failed: %m");
> +			}
> +		}
> +	}
>  	/*
>  	 * Additional check if devfs needs to be mounted:
>  	 * If "/" and "/dev" have the same device number,

> _______________________________________________
> freebsd-hackers at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140602/2dae932c/attachment.sig>