On Fri, Jul 26, 2013 at 7:24 PM, Maxim Dounin <mdounin@mdounin.ru> wrote:
> Hello!
Hi,
>
> On Fri, Jul 26, 2013 at 03:59:47AM -0700, Piotr Sikora wrote:
>
>> Hey,
>>
>> > @@ -76,6 +78,13 @@
>> > 0,
>> > NULL },
>> >
>> > + { ngx_string("so_reuseport"),
>> > + NGX_MAIN_CONF|NGX_DIRECT_CONF|NGX_CONF_TAKE1,
>> > + ngx_set_so_reuseport,
>> > + 0,
>> > + 0,
>> > + NULL },
>> > +
>> > { ngx_string("debug_points"),
>> > NGX_MAIN_CONF|NGX_DIRECT_CONF|NGX_CONF_TAKE1,
>> > ngx_conf_set_enum_slot,
>> > @@ -1361,3 +1370,24 @@
>> >
>> > return NGX_CONF_OK;
>> > }
>> > +
>> > +
>> > +static char *
>> > +ngx_set_so_reuseport(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
>> > +{
>> > + ngx_str_t *value;
>> > + ngx_core_conf_t *ccf;
>> > +
>> > + ccf = (ngx_core_conf_t *) conf;
>> > +
>> > + value = (ngx_str_t *) cf->args->elts;
>> > +
>> > + if (ngx_strcmp(value[1].data, "on") == 0) {
>> > + ccf->so_reuseport = 1;
>> > + } else if (ngx_strcmp(value[1].data, "off") == 0) {
>> > + ccf->so_reuseport = 0;
>> > + } else {
>> > + return "invalid value";
>> > + }
>> > + return NGX_CONF_OK;
>> > +}
>>
>> This can be replaced with ngx_conf_set_flag_slot(), i.e.:
>>
>> + { ngx_string("so_reuseport"),
>> + NGX_MAIN_CONF|NGX_DIRECT_CONF|NGX_CONF_FLAG,
>> + ngx_conf_set_flag_slot,
>> + 0,
>> + offsetof(ngx_core_conf_t, so_reuseport),
>> + NULL },
>
> If it's kept as a global setting, it would be good idea to move
> this into events module if possible.
>
>> Also:
>> 1) like Tom said, you definitely need to guard code to make sure
>> SO_REUSEPORT is available,
>> 2) this feature should be disabled on DragonFly versions prior to the
>> 740d1d9 commit, because it clearly wouldn't do any good there,
>
> I believe SO_REUSEPORT doesn't do accept() load balancing on many
> OSes right now (e.g. FreeBSD doesn't do that), and it might not be
> a good idea to track this in nginx code. It might be better to
> just allow users to decide whether to use it or not. Not sure though.
Since so_reuseport is off by default, I don't think it will do any
harm. Any users of newer DragonFlyBSD (it will be in 3.6 release) and
Linux kernel >= 3.10, could turn it on by themselves (here I assume,
they know what they are doing).
>
>> 3) it might make sense to expose this as an option of "listen"
>> directive, instead of a global setting,
>
> Agree.
See my previous reply. Mainly because accept mutex is global (If I
didn't misunderstand the code) and accept mutex is useless if
SO_REUSEPORT is used. If making so_reuseport a "listen" option is a
must, I think we will have to make accept mutex per-listen socket
first.
>
>> 4) how does this (OS-level sharding) play with nginx's upgrade process
>> (forking of new binary and passing listening fds)? Are there any
>> side-effects of this change that could result in dropped packets /
>> requests?
>
> And obvious downside I see is that with the SO_REUSEPORT causes OS
> to allow duplicate bindings from different processes, which makes
> it possible to unintentionally run 2 copies of nginx. It might be
I think nginx is using pid file to make sure there are only one copy
of nginx is running.
> also possible that configuration test will start to do bad things
> as a result.
Yeah, if so_reuseport is specified, my patch causes nginx not creating
listen socket during configuration test (well, again, this is based on
my understanding of the original code).
Best Regards,
sephe
--
Tomorrow Will Never Die
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
