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

[Lennart Poettering]

On Mon, 09.12.13 14:08, Shawn Landden (shawn at churchofgit.com) wrote:

> Until there are some use cases for Distribute= w/o
> SO_REUSEPORT make it imply that. Otherwise we need
> a new config_parse_distribute in load-fragment.c
> and gain the same issues of config_parse_syscall,
> where NoNewPrivs can be still set to false,
> but only if set _after_ SystemCallFilter (only allowed
> when root).

We have some support to parse "tristate" values like this
already. i.e. values that can be true, false, or "dont know". They are
encoded in "int"s that are either > 0 (for true), == 0 (for false) or <
0 (for don't know). This sounds like a good use for this here, with some
code in socket_load() to set reuse_port to 1 if it is < 0 and Distribute
is > 0.

> Because it takes a while for the service to start up, and
> until then we spin in a fast epoll loop, this tends to
> start up all the instances all at once. There are a number
> of ways we can slow this instanciation down:
>  1) Call accept() and pass an additional fd to the service
>  2) Use EPOLLET: requires event to be prioritized and always
>       dispatched.
>  3) Disable and then reenable the event source every time we
>      enqueue an instance.
> 
> IMHO #1 is not acceptable. For #2, perhaps the new event loop
> infrastructure can support a special class for EPOLLET. I
> am going to investigate #3 right after sending this patch.

Hmm, another option might be to not watch the fd as long as the newly
spawned instance is still starting up. Maybe like this:

When Distribute is > 0, we are in SOCKET_LISTENING exactly when:

     1. none of the instances we spawned is currently in UNIT_ACTIVATING state

     and 

     2. we run less than the specified number of instances.

Otherwise we'd be in SOCKET_RUNNING. 

The distinction between SOCKET_LISTENING and SOCKET_RUNNING is already
in place for normal socket activation: only when in SOCKET_LISTENING
state we will watch the fd, in SOCKET_RUNNING we won't close the fd, but
we won't watch it either.

The effect of this is that startup of the instances is nicely
serialized. It won't have much effect on Type=simple services
however... But maybe this is OK...

> diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
> index 7c10c58..48a4b77 100644
> --- a/man/systemd.socket.xml
> +++ b/man/systemd.socket.xml
> @@ -519,6 +519,17 @@
>                          </varlistentry>
>  
>                          <varlistentry>
> +                                <term><varname>Distribute=</varname></term>
> +                                <listitem><para>Takes an integer
> +                                value. Systemd will spawn up to
> +                                given number of instances of service each
> +                                listening to the same socket. Default is 0.
> +                                Setting this requires corresponding service to
> +                                be an instansiated service (name ends with <literal>@.service</literal>).
> +                                Useful with <varname>Reuseport=</varname> above.</para></listitem>
> +                        </varlistentry>

s/Reuseport/ReusePort/

Also, might be worth mentioning that this conflicts with
Accept=yes. Also mention in the explanation for Accept= that it
conflicts with Distribute=.

>  static int socket_instantiate_service(Socket *s) {
> -        char *prefix, *name;
> +        _cleanup_free_ char *prefix = NULL, *name = NULL;
>          int r;
>          Unit *u;
>  
>          assert(s);
>  
>          /* This fills in s->service if it isn't filled in yet. For
> -         * Accept=yes sockets we create the next connection service
> -         * here. For Accept=no this is mostly a NOP since the service
> +         * Accept=yes and Distribute=n sockets we create the next connection
> +         * service here. Otherwise is mostly a NOP since the service
>           * is figured out at load time anyway. */
>  
> -        if (UNIT_DEREF(s->service))
> +        if (UNIT_DEREF(s->service) && !(s->distribute))

Hmm, why the extra ()?

So far we prefer explicit comparison for numeric != 0 checks. i.e. I
think it is O to write

if (foo) { ... }

only if foo is a bool of some kind or a pointer. But when it is an unsigned
I'd prefer:

if (foo > 0)

or so, to indicate that this is actually a number.

> @@ -604,9 +608,14 @@ static int instance_from_socket(int fd, unsigned nr, char **instance) {
>                  struct sockaddr_storage storage;
>          } local, remote;
>  
> -        assert(fd >= 0);
>          assert(instance);
>  
> +        if (fd < 0) {
> +                if (asprintf(&r, "%u", nr) < 0)
> +                        return -ENOMEM;
> +                goto shortcut;
> +        }
> +

Hmmm, you can make this even short by not using the goto. I mean, goto
is per-se not a pretty thing. It is only a good idea if it makes it
unnecessary to duplicate tons of clean-up calls. In this case, this
should be much nicer:

if (fd < 0) {
        if (asprintf(instance, "%u", nr) < 0)
                return -ENOMEM;
        return 0;
}

Otherwise looks prety good.

Lennart

-- 
Lennart Poettering, Red Hat