[systemd-devel] [PATCH] core: support Distribute=n to distribute to n workers

[Shawn Landden]

Forgot to send my notes on the last review....

On Fri, Dec 13, 2013 at 7:12 AM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Thu, 12.12.13 23:46, Shawn Landden (shawn at churchofgit.com) wrote:
>
>> -Socket.ReusePort,                config_parse_bool,                  0,                             offsetof(Socket, reuse_port)
>> +Socket.ReusePort,                config_parse_tristate,             -1,                             offsetof(Socket, reuse_port)
>                                                                       ^^^^^
>
> Why -1 there? That should be there... The parse call doesn't make use of
> that, so it should be 0, really.
fixed, and i see your point, but i still don't know where the default
of -1 comes from...
>
>
>> +        if (s->reuse_port < 0) {
>> +                if (s->distribute > 0)
>> +                        s->reuse_port = true;
>> +                else
>> +                        s->reuse_port = false;
>> +        }
>> +
>
>
> Nitpicking: I'd just write it like this:
>
> if (s->reuse_port < 0)
>         s->reuse_port = s->distribute > 0;
thats better
>
>
>> -                if (s->n_connections >= s->max_connections) {
>> +                if (s->n_connections >= s->max_connections && !(s->distribute)) {
>
> Still too many ()....
damn
>
>>
>> -        if (se->state == SERVICE_RUNNING)
>> -                socket_set_state(s, SOCKET_RUNNING);
>> +        if (se->state == SERVICE_RUNNING) {
>> +                if (s->n_connections < s->distribute)
>> +                        ;
>> +                else
>> +                        socket_set_state(s, SOCKET_RUNNING);
>> +        }
>
> Hmm, too simple, no? We wanted that logic, that we enter
> SOCKET_RUNNING as long as at least one service is still starting up or
> when we reached the limit. I only see the check for the latter here...
So before I checked for Type=notify in socket_enter_running and then
only spawned one service,
so that I could go back to SOCKET_LISTENING here, but without that
logic, I don't see
a need for any special logic here. Since we only get these
notifications for Type=notify,
we can't just unilaterally go to

>
> Still missing the bit where the socket is duplicated for propery
> SO_REUSEPORT usgae...
OK, so this is the way lwn covered it, but using fork() to replicate
the socket works just fine,
as the attached program demonstrates (also shows lazy reverse
exponential startup), and I
see nothing in the original reuseport patches that indicate that this
is a bad idea.

I'd do this if it was simple (because it gives the EPOLLET behavior I
am looking for), but
it requires some new fields in SocketPort to handle the CLOEXEC
switcharoos, as we will
have two of the same socket open at the same time, one spawned before
the connection came in
to give to the child, and another spawned right after the connection
came in, to hold onto for the
next connection. I'd really like to avoid that since SO_REUSEPORT does
not need it.
>
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: epoll_test.c
Type: text/x-csrc
Size: 10262 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20131213/0a7e029d/attachment.c>