Permit init(8) use its own cpuset group.

[Alexander V. Chernikov]

On 02.06.2014 20:48, Konstantin Belousov wrote:
> 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
Initially I've planned to make this behavior to be default. However I 
though someone
will complain on changing defaults. That's why I've introduced the 
tunable for that.

> 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.
Please take a look on the new version doing this (also updated in 
phabricator,
https://phabric.freebsd.org/D141 ).
>
> 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: D141.diff
Type: text/x-patch
Size: 4416 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140603/28e774fd/attachment.bin>