[PATCH] SO_REUSEPORT support for listen sockets

[Sepherosa Ziehau]

Hi all,

Sorry for the top post, here is the patch in the second around:
http://leaf.dragonflybsd.org/~sephe/ngx_reuseport2.diff

Addressed two problems based on feedbacks:
- Condition the code directly operates on SO_REUSEPORT
- Don't enable so_reuseport, if '-t' (i.e. ngx_test_config==1) is
specified on the command line

Best Regards,
sephe

On Sun, Jul 28, 2013 at 9:21 PM, Sepherosa Ziehau <sepherosa at gmail.com> wrote:
> 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



-- 
Tomorrow Will Never Die