[PATCH 1 of 2] fix sched_setaffinity(2) call failure

[Maxim Dounin]

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