Hello!

On Mon, Jan 16, 2012 at 01:52:09PM +0800, Joshua Zhu wrote:

[...]

> > 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?
> >
>
> Yes, it works on one of my Linux box which has two cores. But I have not
> tested it on more other systems yet.

Ok, I'll commit it then, as this looks like an obvious fix.

[...]

> > 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.)
> >
> >
> We use cpu_set_t and CPU_* macros in the patch not only because it's more
> generic and not depend on the implementation of the kernel (I guess the
> code in Nginx now is inspired by some _early_ kernel implementation, i.e.
> Linux 2.5.x), but also we will have no 32-core-limitation any more
> ('"worker_cpu_affinity" supports up to 32 CPU only').

Are you facing the 32-core limit in practice?

> I agree that the set_cpu_affinity related code in Nginx core should be
> platform-independent. But the code now is _not_ (please look at those
> NGX_HAVE_SCHED_SETAFFINITY macros).

As of now, the only part which isn't portable is
sched_setaffinity() call itself. Other parts (i.e. config
parsing) may be reused for other whatever_setaffinity() calls as
soon as we add support.

> And I do recommend move the
> implementation of ngx_set_cpu_affinity() to the os/ directory when
> worker_cpu_affinity is supported on other platforms.

Yes, that's basically what I suggest also. We want some generic
interface, and platform-specific wrappers which do needed
conversions/magic.

Maxim Dounin

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