Date: Mon, 2 Jun 2014 19:48:50 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: "Alexander V. Chernikov" <melifaro@FreeBSD.org> Cc: hackers@freebsd.org Subject: Re: Permit init(8) use its own cpuset group. Message-ID: <20140602164850.GS3991@kib.kiev.ua> In-Reply-To: <538C8F9A.4020301@FreeBSD.org> References: <538C8F9A.4020301@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (FreeBSD)
iQIcBAEBAgAGBQJTjKryAAoJEJDCuSvBvK1BbUkP/0yDiTkAjyiMWLdm19y8Rkbz
2K5TcZCnbHbHvrd+Z3zGuH+Op7vSWvJ1sHuE4Qfe9VSPDQyk8T3vhSEp9s/KWlhe
dwOhBDxL6Nk/lhn9eGjLWjAe/UmVS0Q8M+v0QX4X7ZbCgVaHQApLu+z9ejeGmCt6
bGqZmXmYmshtPEm8k9NlV9sHwaAB8ySVGf9WYWUDPBg+MS9IFACavRF1tQeXYaH1
8duuzSbhUvARaM5qEz9fwep28AiyX7EFgsukfytS9O7JStDc8Qgjdc2iCtzCUqXI
duYwhMNfCEhoYbCCjdycd4FtyKhLgBbXEWMj5n7PipN9+qUemSwH7Ascjo0ybw/e
sXt35km9tqKm+xpsHzkKrW73/ibewCf5bN4xndaOwlgNRrmKNYvwCYa2xi4nRUqB
u6BCBBtIaDN+Vwa/993AKegbwPMF27HOdPGEpSigcq5AUYAh631rRNnNh0hipMn/
TiWR8BvBqNqkigKr/FYcUv2OrsengywUYSE81zVoWM3r/MdyKCqcmo6p9yHDuEXp
nSEFZjg/JYjIgGfjTvTvqoFjijw7k59zPFXhOGpyr0WSztstIq6p0mEheuMN6n0v
zOSXLChAeT/ijCIgTpp52jz0/BOmtzehWx329U2ueLN4GfNWKLvUck9VWFwZ0gBl
l4Fda7DWVipIP7Pg2pCe
=EELr
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140602164850.GS3991>