Hello!

On Thu, Jan 12, 2012 at 11:37:00PM +0800, Joshua Zhu wrote:

> Hi,
>
> 'cpu_set_t' should be used instead of 'long' when calling
> sched_setaffinity(2). Otherwise it may fail on some Linux systems.
> Please see the attachment for more detail.

According to sched_setaffinity(2) at least in kernels >= 2.6.9 it
should be ok to use any length as long as bits above kernel mask
used aren't referenced.

I believe the real problem is that code uses 32 as cpusetsize,
while this is the size in bytes. Hence it references arbitrary
memory and this causes EINVAL to be returned by glibc if there are
bits referenced above kernel cpumask size, see
sysdeps/unix/sysv/linux/sched_setaffinity.c in glibc's sources.

The following patch should be enough:

diff -r e1296af53cc0 src/os/unix/ngx_process_cycle.c
--- a/src/os/unix/ngx_process_cycle.c Mon Dec 26 00:00:00 2011 +0400
+++ b/src/os/unix/ngx_process_cycle.c Fri Jan 13 11:58:01 2012 -0500
@@ -914,7 +914,10 @@
ngx_log_error(NGX_LOG_NOTICE, cycle->log, 0,
"sched_setaffinity(0x%08Xl)", cpu_affinity);

- if (sched_setaffinity(0, 32, (cpu_set_t *) &cpu_affinity) == -1) {
+ if (sched_setaffinity(0, sizeof(cpu_affinity),
+ (cpu_set_t *) &cpu_affinity)
+ == -1)
+ {
ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
"sched_setaffinity(0x%08Xl) failed", cpu_affinity);
}

Could you please test if it works for you?

It passes my limited testing, but I wasn't really able to
reproduce the problem without modifying the sched_setaffinity()
call in question to use bigger cpusetsize.

[...]

> diff -uprN nginx-1.1.12/src/core/nginx.c nginx-1.1.12-bugfix/src/core/nginx.c
> --- nginx-1.1.12/src/core/nginx.c 2011-11-16 04:35:42.000000000 +0800
> +++ nginx-1.1.12-bugfix/src/core/nginx.c 2012-01-12 11:06:00.000000000 +0800
> @@ -1245,15 +1245,15 @@ ngx_set_cpu_affinity(ngx_conf_t *cf, ngx
> ngx_core_conf_t *ccf = conf;
>
> u_char ch;
> - u_long *mask;
> + cpu_set_t *mask;
> ngx_str_t *value;
> - ngx_uint_t i, n;
> + ngx_uint_t i, j, n;
>
> if (ccf->cpu_affinity) {
> return "is duplicate";
> }
>
> - mask = ngx_palloc(cf->pool, (cf->args->nelts - 1) * sizeof(long));
> + mask = ngx_palloc(cf->pool, (cf->args->nelts - 1) * sizeof(cpu_set_t));
> if (mask == NULL) {
> return NGX_CONF_ERROR;
> }

[...]

While I generally agree that using cpu_set_t and CPU_* macros
explicitly would be better, I don't think we want to introduce
linux-centric code here in platform-independent code.

Instead we probably want to leave this code generic enough to be
useable with other scheduler interfaces, and convert the data
somewhere near platform-specific whatever_setaffinity() calls if
needed. (Likely unneeded for the Linux with the above patch.)

Maxim Dounin

_______________________________________________
nginx mailing list
nginx@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx