[PATCH] SO_REUSEPORT support for listen sockets

[Sepherosa Ziehau]

On Fri, Jul 26, 2013 at 7:24 PM, Maxim Dounin <mdounin at 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