fpm binding master and children processes to specific core(s). #10075

devnexen · 2022-12-09T19:34:07Z

rational is there is git snippets and similar floating around about how to achieve this via command line.
So idea is to do it "natively" especially those above mostly focus on linux whereas more platforms are cpu affinity capable. For now at least, focusing on simple cases, one core id or a range.

sapi/fpm/www.conf.in

iluuu1994 · 2022-12-11T16:38:01Z

@devnexen Thank you! I'll have a look soon. 🙂

sapi/fpm/tests/cpuaffinity.phpt

iluuu1994

I wonder generally whether the main use-case here would be to pin specific workers to specific CPUs, rather than pinning all workers to a range of CPUs. I've never actually used setaffinity so I'm not sure if that makes a big difference or if the scheduler is smart enough to avoid these context switches.

sapi/fpm/fpm/fpm_unix.c

sapi/fpm/php-fpm.conf.in

sapi/fpm/tests/cpuaffinity.phpt

iluuu1994 · 2022-12-22T16:10:00Z

sapi/fpm/tests/cpuaffinity-fail.phpt

+<?php include "skipif.inc";
+
+if (!str_contains(PHP_OS, 'Linux')) {
+    die('skipped supported only on Linux');


FreeBSD supports cpuset_setaffinity, is there any reason not to test this?

Yeah why is this limited to Linux only if cpuset_setaffinity is already implemented?

sapi/fpm/fpm/fpm_unix.c

iluuu1994 · 2022-12-22T16:19:19Z

sapi/fpm/fpm/fpm_unix.c

+#if defined(HAVE_SCHED_SETAFFINITY) || defined(HAVE_CPUSET_SETAFFINITY)
+	cpu_set_t cset;
+#endif
+	long min;


I find these slightly misleading, as they are only used as temporary values for fpm_cpuaffinity_add. I'd prefer if min/max were parameters to fpm_cpuaffinity_add but it's not a big deal.

yeah agree, probably not much point to define them in struct as it can be just param. Also agree that it's not a big deal though :)

Ok I will rephrase it. Please can you change this too.

sapi/fpm/fpm/fpm_unix.c

bukka

I think the idea makes sense in general. Added some comments. The ones that stand out for me are following 2 issue.

As I mentioned in one of the the comments I don't like the configuration as it seems very cryptic. I think more declarative approach (potentially introducing more options) would be less error prone and easier to understand IMO.

Support for containers (Docker) is necessary as this would likely be reported as a bug later if it doesn't work correctly. IIRC _SC_NPROCESSORS_ONLN reports number of processors in the host rather than what the container is limited to so this might need to be addressed if that's the case.

configure.ac

bukka · 2022-12-22T18:45:14Z

sapi/fpm/fpm/fpm_unix.c

@@ -349,6 +358,102 @@ static int fpm_unix_conf_wp(struct fpm_worker_pool_s *wp) /* {{{ */
 }
 /* }}} */

+#if HAVE_FPM_CPUAFFINITY
+struct fpm_cpuaffinity_conf {
+#if defined(HAVE_SCHED_SETAFFINITY) || defined(HAVE_CPUSET_SETAFFINITY)


This check seems useless as it is covered by HAVE_FPM_CPUAFFINITY

I think the idea is to extend this with different checks that are platform specific.

well it should be then updated or if it's not ready for review yet, then it should be probably a draft PR. I'd rather not to merge anything that has got some part with "maybe in the future"... :)

This comment still stands.

sapi/fpm/fpm/fpm_unix.c

sapi/fpm/php-fpm.conf.in

sapi/fpm/fpm/fpm_unix.c

sapi/fpm/fpm/fpm_conf.c

bukka · 2022-12-22T19:19:23Z

@iluuu1994

I wonder generally whether the main use-case here would be to pin specific workers to specific CPUs, rather than pinning all workers to a range of CPUs

It sets affinity to all workers (children) in the pool. Pools are the way how the workers are differentiated in FPM. So if you need to select only specific ones, then you need multiple pools.

devnexen · 2022-12-22T20:45:21Z

It sets affinity to all workers (children) in the pool.

master process and/or workers (if you do not set worker's, it will inherit master's setting).

bukka · 2022-12-23T17:47:18Z

It sets affinity to all workers (children) in the pool.

master process and/or workers (if you do not set worker's, it will inherit master's setting).

Ok finally got it - I initially thought that there's just pool config but now I see there's global config for master as well.

bukka · 2022-12-23T17:54:27Z

sapi/fpm/www.conf.in

+; Default Value: inherits master's cpu affinity
+; process.cpumask = "8"
+; process.cpumask = "0-3"
+; process.cpumask = "4-7;10-12"


If this format is chosen, it needs more description about the format.

devnexen · 2023-01-11T20:01:30Z

@bukka ping :)

bukka · 2023-01-14T21:26:33Z

@devnexen I just had a look and CPU_COUNT certainly looks like a better solution. Might be better to just error if sched_getaffinity fails (returns -1) rather than trying to use sysconf though. Otherwise there are still some unresolved comments so please go through them and either fix them or replay so they all get resolved. Also the FreeBSD failure is related. Once it's all green and comments resolved, I will give a try to see if it all works as expected for me too. 😉

devnexen · 2023-01-15T18:44:15Z

@devnexen I just had a look and CPU_COUNT certainly looks like a better solution. Might be better to just error if sched_getaffinity fails (returns -1) rather than trying to use sysconf though. Otherwise there are still some unresolved comments so please go through them and either fix them or replay so they all get resolved. Also the FreeBSD failure is related. Once it's all green and comments resolved, I will give a try to see if it all works as expected for me too. 😉

Think I ve addressed your concerns now :-) cheers

bukka · 2023-01-20T11:11:52Z

Cool I will take a proper look early in Feb

bukka

It still needs a bit of work

bukka · 2023-02-05T18:11:59Z

sapi/fpm/php-fpm.conf.in

+; the master process could be bound to a single core
+; process.cpu_list = "4"
+; a range from the minimum cpuid to the max required
+; process.cpu_list = "0-3"
+; or multiple ranges separated by a comma
+; process.cpu_list = "1-4,8-16,32-48"


It would be better to have the format docs before the value rather than actual example. See other entries to get some inspiration. For example

php-src/sapi/fpm/www.conf.in

Lines 276 to 283 in 64b8306

; Valid syntaxes are:

; 'ip.add.re.ss:port' - to listen on a TCP socket to a specific IPv4 address on

; a specific port;

; '[ip:6:addr:ess]:port' - to listen on a TCP socket to a specific IPv6 address on

; a specific port;

; 'port' - to listen on a TCP socket to all addresses

; (IPv6 and IPv4-mapped) on a specific port;

; '/path/to/unix/socket' - to listen on a unix socket.

.

bukka · 2023-02-05T18:14:12Z

sapi/fpm/fpm/fpm_unix.c

+
+static void fpm_cpuaffinity_init(struct fpm_cpuaffinity_conf *c)
+{
+#if defined(HAVE_FPM_CPUAFFINITY)


What's the point of this actually. Isn't this already inside this block: https://github.com/php/php-src/pull/10075/files#diff-6b26b2bd2a798d905b4419198f73f61fac2ecc265b4d361d204f565fd91d407fR405

ah yes it is last reminiscence of "further porting to other platforms" thing.

Yeah so it needs to get removed...

bukka · 2023-02-05T18:14:16Z

sapi/fpm/fpm/fpm_unix.c

+
+static void fpm_cpuaffinity_add(struct fpm_cpuaffinity_conf *c)
+{
+#if defined(HAVE_FPM_CPUAFFINITY)


What's the point of this actually. Isn't this already inside this block: https://github.com/php/php-src/pull/10075/files#diff-6b26b2bd2a798d905b4419198f73f61fac2ecc265b4d361d204f565fd91d407fR405

This still stands...

sapi/fpm/fpm/fpm_unix.c

bukka · 2023-02-05T18:16:15Z

sapi/fpm/tests/cpuaffinity-fail.phpt

+<?php include "skipif.inc";
+
+if (!str_contains(PHP_OS, 'Linux')) {
+    die('skipped supported only on Linux');


Yeah why is this limited to Linux only if cpuset_setaffinity is already implemented?

bukka · 2023-02-05T18:17:17Z

sapi/fpm/www.conf.in

+; the pool process could be bound to a single core
+; process.cpu_list = "8"
+; a range from the minimum cpuid to the max required
+; process.cpu_list = "0-3"
+; or multiple ranges separated by a comma
+; process.cpu_list = "4-7,10-12"


Same as above comment for global config - better description would be useful...

sapi/fpm/fpm/fpm_unix.c

bukka · 2023-02-05T18:24:30Z

sapi/fpm/fpm/fpm_unix.c

@@ -349,6 +358,102 @@ static int fpm_unix_conf_wp(struct fpm_worker_pool_s *wp) /* {{{ */
 }
 /* }}} */

+#if HAVE_FPM_CPUAFFINITY
+struct fpm_cpuaffinity_conf {
+#if defined(HAVE_SCHED_SETAFFINITY) || defined(HAVE_CPUSET_SETAFFINITY)


This comment still stands.

bukka · 2023-02-05T18:25:17Z

sapi/fpm/fpm/fpm_unix.c

+#if defined(HAVE_SCHED_SETAFFINITY) || defined(HAVE_CPUSET_SETAFFINITY)
+	cpu_set_t cset;
+#endif
+	long min;


yeah agree, probably not much point to define them in struct as it can be just param. Also agree that it's not a big deal though :)

bukka · 2023-02-05T18:30:50Z

@devnexen There are still bunch of comments. Please comment on or resolve them once you address them. It will be then clearer what needs to be done. I resolved all those that were done and added some other comments.

devnexen · 2023-02-06T10:10:48Z

@devnexen There are still bunch of comments. Please comment on or resolve them once you address them. It will be then clearer what needs to be done. I resolved all those that were done and added some other comments.

I think I have addressed those, unluckily, vm builders fail at the moment but at least you can try locally the multi range part :-)

bukka

Added more comments

bukka · 2023-02-09T11:40:50Z

sapi/fpm/tests/cpuaffinity-range.phpt

+pm.start_servers = 1
+pm.min_spare_servers = 1
+pm.max_spare_servers = 1
+process.cpu_list = 0-1,2-3


Could you maybe change it to 0,2-3 as it will make more sense...

bukka · 2023-02-09T11:41:31Z

sapi/fpm/tests/cpuaffinity-range.phpt

+if (!str_contains(PHP_OS, 'Linux') && !str_contains(PHP_OS, 'FreeBSD')) {
+    die('skipped supported only on Linux and FreeBSD');
+}
+
+if (str_contains(PHP_OS, 'Linux')) {
+    $cmd = 'nproc';
+} else {
+    $cmd = 'sysctl hw.ncpus';
+}
+
+$nproc = intval(exec($cmd));
+if ($nproc < 4) {
+    die('skipped supported only on 4 cores or more environments');
+}


Can you integrate those independently to the Tester (static methods) as done elsewhere...

good point.

bukka · 2023-02-09T11:45:56Z

sapi/fpm/php-fpm.conf.in

+;       process.cpu_list = "cpu id"                 - to bind to a single core
+;       process.cpu_list = "min cpu id-max cpu id"  - to bind to a core range
+                                                      from minimum cpu id to max
+;       process.cpu_list = "[min cpu id-max cpu id],[min cpu id-max cpu id],..."
+                                                    - to bind to multiple ranges
+                                                      separated by a comma


This is actually very confusing description and a bit incorrect. Could you maybe use BNF to describe and keep some examples. I didn't mean to replace the examples completely but to add some generic description.

bukka · 2023-02-09T11:46:47Z

sapi/fpm/fpm/fpm_unix.c

@@ -416,6 +521,15 @@ int fpm_unix_init_child(struct fpm_worker_pool_s *wp) /* {{{ */
 			zlog(ZLOG_SYSERROR, "[pool %s] failed to set rlimit_core for this pool. Please check your system limits or decrease rlimit_core. setrlimit(RLIMIT_CORE, %d)", wp->config->name, wp->config->rlimit_core);
 		}
 	}
+#if HAVE_FPM_CPUAFFINITY
+	if (wp->config->process_cpu_list) {
+


Why extra nesting here. Just use single condition.

bukka · 2023-02-09T11:47:11Z

sapi/fpm/fpm/fpm_unix.c

+
+static void fpm_cpuaffinity_add(struct fpm_cpuaffinity_conf *c)
+{
+#if defined(HAVE_FPM_CPUAFFINITY)


This still stands...

bukka · 2023-02-09T11:47:39Z

sapi/fpm/fpm/fpm_unix.c

+#if defined(HAVE_SCHED_SETAFFINITY) || defined(HAVE_CPUSET_SETAFFINITY)
+	cpu_set_t cset;
+#endif
+	long min;


Ok I will rephrase it. Please can you change this too.

bukka · 2023-02-09T11:48:47Z

sapi/fpm/www.conf.in

+; Valid syntaxes are:
+;       process.cpu_list = "cpu id"                 - to bind to a single core
+;       process.cpu_list = "min cpu id-max cpu id"  - to bind to a core range
+                                                      from minimum cpu id to max
+;       process.cpu_list = "[min cpu id-max cpu id],[min cpu id-max cpu id],..."
+                                                    - to bind to multiple ranges
+                                                      separated by a comma


devnexen · 2023-02-09T19:56:46Z

Added more comments

Thanks @bukka I did some changes let me know if I forgot something.

devnexen · 2023-04-05T19:07:08Z

ping @bukka when you have a moment. 🙂

bukka · 2023-04-16T17:07:54Z

I will try to find some time for this in May. Already used almost all my FPM time for this month.

bukka

I have done some testing and it contains bugs. The parser needs be fixed as the current implementation contain errors which are not that easily visible - needed some separated testing for that. It would be probably good if we had some unit testing framework (e.g. cmocka) that we could use for things like this.

bukka · 2023-05-19T13:23:05Z

sapi/fpm/fpm/fpm_unix.c

+#endif
+
+#ifdef HAVE_SYS_CPUSET_H


NIT: this can be elif I guess...

bukka · 2023-05-19T14:25:05Z

sapi/fpm/fpm/fpm_unix.c

+
+		fpm_cpuaffinity_add(&c, min, max);
+
+		token = php_strtok_r(NULL, ";", &buf);


There should be comma otherwise it considers only two elements. You should add some test with two commas to test this...

bukka · 2023-05-19T15:01:48Z

sapi/fpm/fpm/fpm_unix.c

+	while (token) {
+		char *cpu_listsep;
+
+		min = strtol(token, &cpu_listsep, 0);


this is too forgiving and allows latters in the string. For example abbbbb123,1 results in min and max 0 for the first element and 1 for min max second element. Obviously this should be error. I converted this to a small program:

#include <errno.h> #include <string.h> #include <stdio.h> #include <stdlib.h> char *php_strtok_r(char *s, const char *delim, char **last) { char *spanp; int c, sc; char *tok; if (s == NULL && (s = *last) == NULL) { return NULL; } /* * Skip (span) leading delimiters (s += strspn(s, delim), sort of). */ cont: c = *s++; for (spanp = (char *)delim; (sc = *spanp++) != 0; ) { if (c == sc) { goto cont; } } if (c == 0) /* no non-delimiter characters */ { *last = NULL; return NULL; } tok = s - 1; /* * Scan token (scan for delimiters: s += strcspn(s, delim), sort of). * Note that delim must have one NUL; we stop if we see that, too. */ for (;;) { c = *s++; spanp = (char *)delim; do { if ((sc = *spanp++) == c) { if (c == 0) { s = NULL; } else { char *w = s - 1; *w = '\0'; } *last = s; return tok; } } while (sc != 0); } /* NOTREACHED */ } int parse(const char *cpu_list_const, int cpumax) { char *token, *buf; int min, max; int rc = 0; char *cpu_list = strdup(cpu_list_const); printf("\nparsing %s\n", cpu_list); token = php_strtok_r(cpu_list, ",", &buf); while (token) { char *cpu_listsep; min = strtol(token, &cpu_listsep, 0); if (errno || min < 0 || min > cpumax) { rc = -1; } max = min; if (*cpu_listsep == '-') { if (strlen(cpu_listsep) > 1) { char *err; max = strtol(cpu_listsep + 1, &err, 0); if (errno || *err != '\0' || max < min || max > cpumax) { rc = -1; goto end; } } else { rc = -1; goto end; } } printf("min: %d, max: %d\n", min, max); token = php_strtok_r(NULL, ",", &buf); } end: printf("result: %d, cpu list: %s\n", rc, cpu_list); free(cpu_list); return rc; } int main() { parse("1,2,5", 10); parse("1-2", 4); parse("1-2,4-5,7-10", 12); // error cases parse("1-8", 4); parse("abbbbb123,1", 10); parse("123afafkalf,1ffff", 10); }

which results to

parsing 1,2,5 min: 1, max: 1 min: 2, max: 2 min: 5, max: 5 result: 0, cpu list: 1 parsing 1-2 min: 1, max: 2 result: 0, cpu list: 1-2 parsing 1-2,4-5,7-10 min: 1, max: 2 min: 4, max: 5 min: 7, max: 10 result: 0, cpu list: 1-2 parsing 1-8 result: -1, cpu list: 1-8 parsing abbbbb123,1 min: 0, max: 0 min: 1, max: 1 result: 0, cpu list: abbbbb123 parsing 123afafkalf,1ffff min: 123, max: 123 min: 1, max: 1 result: -1, cpu list: 123afafkalf

This needs to be fixed.

bukka · 2023-05-19T15:06:45Z

sapi/fpm/fpm/fpm_unix.c

+	cpumax = fpm_cpumax();
+
+	fpm_cpuaffinity_init(&c);
+	token = php_strtok_r(cpu_list, ",", &buf);


The problem here is that php_strtok_r modifies the value so you are changing the configured value in the config which should not be done. You should copy the list to a new buffer (either on heap allocated or using alloca) before calling this.

rational is there is git snippets and similar floating around about how to achieve this via command line. So the idea is to do it "natively" especially those above mostly focus on linux whereas more platforms are cpu affinity "capable".

github-actions bot added the SAPI: fpm label Dec 9, 2022

devnexen force-pushed the fpm_cpu_affinity branch 2 times, most recently from 557a88a to f55fb44 Compare December 10, 2022 15:29

devnexen changed the title ~~[wip] fpm binding master and children processes to specific core(s).~~ fpm binding master and children processes to specific core(s). Dec 10, 2022

devnexen marked this pull request as ready for review December 10, 2022 16:09

mvorisek reviewed Dec 10, 2022

View reviewed changes

sapi/fpm/www.conf.in Outdated Show resolved Hide resolved

devnexen requested a review from iluuu1994 December 11, 2022 11:48

devnexen force-pushed the fpm_cpu_affinity branch from b097796 to b049245 Compare December 16, 2022 11:24

mvorisek reviewed Dec 16, 2022

View reviewed changes

sapi/fpm/tests/cpuaffinity.phpt Outdated Show resolved Hide resolved

devnexen force-pushed the fpm_cpu_affinity branch 2 times, most recently from 3ff190e to a0be7ff Compare December 16, 2022 16:02

iluuu1994 reviewed Dec 22, 2022

View reviewed changes

iluuu1994 requested a review from bukka December 22, 2022 16:46

devnexen force-pushed the fpm_cpu_affinity branch from a0be7ff to e61c205 Compare December 22, 2022 18:07

bukka requested changes Dec 22, 2022

View reviewed changes

bukka reviewed Dec 23, 2022

View reviewed changes

devnexen force-pushed the fpm_cpu_affinity branch 2 times, most recently from 0abc26c to 64b8306 Compare January 14, 2023 22:03

bukka requested changes Feb 5, 2023

View reviewed changes

devnexen force-pushed the fpm_cpu_affinity branch 2 times, most recently from 0f4bb85 to 840c0d2 Compare February 6, 2023 09:54

bukka requested changes Feb 9, 2023

View reviewed changes

devnexen force-pushed the fpm_cpu_affinity branch from 5bd3e29 to 0b581e4 Compare February 9, 2023 19:55

devnexen force-pushed the fpm_cpu_affinity branch 2 times, most recently from b875208 to 4ae6d19 Compare February 9, 2023 22:34

devnexen force-pushed the fpm_cpu_affinity branch from 4ae6d19 to d2c20d1 Compare March 25, 2023 23:13

bukka requested changes May 19, 2023

View reviewed changes

devnexen added 4 commits May 20, 2023 18:56

build

21acaa8

changes from feedback

4c04aec

adding new tests

c910c0e

devnexen force-pushed the fpm_cpu_affinity branch from d2c20d1 to c910c0e Compare May 21, 2023 12:06

	; Valid syntaxes are:
	; 'ip.add.re.ss:port' - to listen on a TCP socket to a specific IPv4 address on
	; a specific port;
	; '[ip:6:addr:ess]:port' - to listen on a TCP socket to a specific IPv6 address on
	; a specific port;
	; 'port' - to listen on a TCP socket to all addresses
	; (IPv6 and IPv4-mapped) on a specific port;
	; '/path/to/unix/socket' - to listen on a unix socket.


		fpm_cpuaffinity_add(&c, min, max);

		token = php_strtok_r(NULL, ";", &buf);

		#endif

		#ifdef HAVE_SYS_CPUSET_H

fpm binding master and children processes to specific core(s). #10075

Are you sure you want to change the base?

fpm binding master and children processes to specific core(s). #10075

Conversation

devnexen commented Dec 9, 2022

iluuu1994 commented Dec 11, 2022

iluuu1994 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka commented Dec 22, 2022

devnexen commented Dec 22, 2022 • edited Loading

bukka commented Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

devnexen commented Jan 11, 2023

bukka commented Jan 14, 2023

devnexen commented Jan 15, 2023

bukka commented Jan 20, 2023

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka commented Feb 5, 2023

devnexen commented Feb 6, 2023

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen commented Feb 9, 2023

devnexen commented Apr 5, 2023

bukka commented Apr 16, 2023

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen commented Dec 22, 2022 •

edited

Loading

bukka commented Dec 23, 2022 •

edited

Loading