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

2014-01-06 Thread David Timothy Strauss 
On Sun, Jan 5, 2014 at 1:44 PM, Zbigniew Jędrzejewski-Szmek
zbys...@in.waw.pl wrote:
 3. The strategy of dup()ing the socket doesn't work. I wrote
a simple server in python which logs the connections [2], and hooked
it up into systemd [3-4] (*). If REUSEPORT was working correctly,
each connection would be handled by just one instance, either created
previously, or newly created by systemd for this connection. But
I see the same connection being accept()ed by one of the instances
and systemd itself spawning a new instance. I'm pretty sure that what
Lennart wrote before, that you need to create a new socket bound to
the same port for REUSEPORT to take effect, is true.

I've also seen pretty even (but not perfect, especially with
event-based designs) distribution with multiple processes simply
running accept() on the same inherited socket.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2014-01-06 Thread Zbigniew Jędrzejewski-Szmek 
On Mon, Jan 06, 2014 at 04:14:40AM -0800, David Timothy Strauss wrote:
 On Sun, Jan 5, 2014 at 1:44 PM, Zbigniew Jędrzejewski-Szmek
 zbys...@in.waw.pl wrote:
  3. The strategy of dup()ing the socket doesn't work. I wrote
 a simple server in python which logs the connections [2], and hooked
 it up into systemd [3-4] (*). If REUSEPORT was working correctly,
 each connection would be handled by just one instance, either created
 previously, or newly created by systemd for this connection. But
 I see the same connection being accept()ed by one of the instances
 and systemd itself spawning a new instance. I'm pretty sure that what
 Lennart wrote before, that you need to create a new socket bound to
 the same port for REUSEPORT to take effect, is true.
 
 I've also seen pretty even (but not perfect, especially with
 event-based designs) distribution with multiple processes simply
 running accept() on the same inherited socket.
It'll work fine with multiple daemons all waiting in accept (e.g. in a threaded
server), because only one will be woken. But if they are using poll, they will
all get notified and race. But this second option is what we're working towards
here.

Zbyszek

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2014-01-05 Thread Zbigniew Jędrzejewski-Szmek 
On Thu, Dec 19, 2013 at 12:21:26PM -0800, Shawn Landden wrote:
 On Fri, Dec 13, 2013 at 8:23 PM, Shawn Landden sh...@churchofgit.com wrote:
  If Distribute=n, turns SO_REUSEPORT on, and spawns
  n workers to handling incoming requests.
 
  SO_REUSEPORT sockets on the same port must all be created
  by the same uid, therefore using the option allows
  other root programs (or programs of the same user
  if running in --user mode) to hijack this port, even
  after systemd reserves it.
 
  We spawn workers at a rate approximentally reverse
  exponentially proportianal to the number of incoming connections.
  Faster based on the time for new workers to start accept()ing
  and their load, or slower if systemd is under load.
Hi Shawn,
sorry for the delay.

Your patch is nice, but I found three issues:

1. The documentation is still lacking. I made a small patch which extends
   and clarifies the description of Distribute=n a bit, but I think that
   even more explanation should be given [1]. Maybe you fold it into your
   patch?

2. It is possible that the instance name might be taken. One legitimate
   case would be when the socket is started, some instances are created,
   and the socket is stopped and started again. Then the connection count
   will be reset to 0. The user might also start an instance by hand. Such
   situations should not prevent the connection from being accepted.
   Something similar happens when snapshots are created, and systemd
   loops looking for a free name. The same fallback should be implemented
   here, either with linearly increasing instances, or maybe with random
   numbers in case the instance names is occupied.

3. The strategy of dup()ing the socket doesn't work. I wrote
   a simple server in python which logs the connections [2], and hooked
   it up into systemd [3-4] (*). If REUSEPORT was working correctly,
   each connection would be handled by just one instance, either created
   previously, or newly created by systemd for this connection. But
   I see the same connection being accept()ed by one of the instances
   and systemd itself spawning a new instance. I'm pretty sure that what
   Lennart wrote before, that you need to create a new socket bound to
   the same port for REUSEPORT to take effect, is true.

[1] 
http://in.waw.pl/~zbyszek/distribute-n/0001-Fix-Distribute-n-documentation.patch
[2] http://in.waw.pl/~zbyszek/distribute-n/socket_logger.py
[3] http://in.waw.pl/~zbyszek/distribute-n/distributed.socket
[4] http://in.waw.pl/~zbyszek/distribute-n/distributed@.service

(*) In the python script, it seems that print() statements don't reach
the journal, but systemd.journal.send()s do. I guess I'm missing something.
But that's why logging is duplicted. If somebody could explain this,
that would be great.
   
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2013-12-13 Thread Lennart Poettering 
On Thu, 12.12.13 23:46, Shawn Landden (sh...@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.


 +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;


 -if (s-n_connections = s-max_connections) {
 +if (s-n_connections = s-max_connections  
 !(s-distribute)) {

Still too many ()

  
 -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...

Still missing the bit where the socket is duplicated for propery
SO_REUSEPORT usgae...


Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2013-12-13 Thread Shawn Landden 
Forgot to send my notes on the last review

On Fri, Dec 13, 2013 at 7:12 AM, Lennart Poettering
lenn...@poettering.net wrote:
 On Thu, 12.12.13 23:46, Shawn Landden (sh...@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@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
ELF€	@@ @8@@@@@@ÀÀ@@@@ÔÔ ØØ`Ø`ÀÈ ðð`ð`àà@@DDPåtdÌÌ@Ì@44Qåtd/lib64/ld-linux-x86-64.so.2GNU GNUžXÂØd’ú÷0íšB’˜ža!^†

	P˜6=De[†ž /Ky€k*libc.so.6setuidsockethtonsepoll_waitforklistengetpidprintfmemsetbindsetsockoptepoll_ctlcloseepoll_create1acceptsleep__libc_start_mainwrite__gmon_start__GLIBC_2.9GLIBC_2.3.2GLIBC_2.2.5ii
­ri	·ui	ÃÐ`
ð`ø````` `(`0`	8`
@`

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

2013-12-13 Thread Shawn Landden 
If Distribute=n, turns SO_REUSEPORT on, and spawns
n workers to handling incoming requests.

SO_REUSEPORT sockets on the same port must all be created
by the same uid, therefore using the option allows
other root programs (or programs of the same user
if running in --user mode) to hijack this port, even
after systemd reserves it.

This patch is currently not ready for merging because if
the service never accept()s
the socket passed to it, we stay in EPOLLIN, looping
infinitely. To prevent this we need to do one of:

 *) Call accept() and pass an additional fd to the service.
 *) Use EPOLLET: requires event to always dispatched when it comes in.
 *) Disable and then reenable the event source (requiring
 that we re-create the socket) every time we
 enqueue an instance, essentially emulating the behavior
 of EPOLLET.
---
 TODO  |  3 +-
 man/systemd.socket.xml| 15 +++-
 src/core/dbus-socket.c|  4 +-
 src/core/load-fragment-gperf.gperf.m4 |  3 +-
 src/core/service.c|  4 +-
 src/core/socket.c | 70 +--
 src/core/socket.h |  5 ++-
 src/shared/conf-parser.c  | 32 
 src/shared/conf-parser.h  |  1 +
 9 files changed, 108 insertions(+), 29 deletions(-)

diff --git a/TODO b/TODO
index dad55c4..fbc2609 100644
--- a/TODO
+++ b/TODO
@@ -73,7 +73,7 @@ Features:
 
 * rfkill,backlight: we probably should run the load tools inside of the udev 
rules so that the state is properly initialized by the time other software sees 
it
 
-* Add a new Distribute=$NUMBER key to socket units that makes use of 
SO_REUSEPORT to distribute network traffic on $NUMBER instances
+* tmpfiles: when applying ownership to /run/log/journal, also do this for the 
journal fails contained in it
 
 * move config_parse_path_strv() out of conf-parser.c
 
@@ -187,7 +187,6 @@ Features:
 * teach ConditionKernelCommandLine= globs or regexes (in order to match 
foobar={no,0,off})
 
 * Support SO_REUSEPORT with socket activation:
-  - Let systemd maintain a pool of servers.
   - Use for seamless upgrades, by running the new server before stopping the
 old.
 
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 7c10c58..6799020 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -404,7 +404,8 @@
 designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation. Incompatible with
+
varnameDistribute=/varname/para/listitem
 /varlistentry
 
 varlistentry
@@ -519,6 +520,18 @@
 /varlistentry
 
 varlistentry
+termvarnameDistribute=/varname/term
+listitemparaTakes 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 varnameReusePort=/varname 
above.
+Incompatible with 
varnameAccept=true/varname./para/listitem
+/varlistentry
+
+varlistentry
 termvarnameSmackLabel=/varname/term
 termvarnameSmackLabelIPIn=/varname/term
 
termvarnameSmackLabelIPOut=/varname/term
diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c
index 74217df..036c9af 100644
--- a/src/core/dbus-socket.c
+++ b/src/core/dbus-socket.c
@@ -40,7 +40,6 @@ static int property_get_listen(
 void *userdata,
 sd_bus_error *error) {
 
-
 Socket *s = SOCKET(userdata);
 SocketPort *p;
 int r;
@@ -115,7 +114,8 @@ const sd_bus_vtable bus_socket_vtable[] = {
 SD_BUS_PROPERTY(MessageQueueMaxMessages, x, bus_property_get_long, 
offsetof(Socket, mq_maxmsg), 0),
 SD_BUS_PROPERTY(MessageQueueMessageSize, x, bus_property_get_long, 
offsetof(Socket, mq_msgsize), 0),
 SD_BUS_PROPERTY(Result, s, property_get_result, offsetof(Socket, 
result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
-SD_BUS_PROPERTY(ReusePort, b,  bus_property_get_bool, 
offsetof(Socket, reuse_port), 0),
+SD_BUS_PROPERTY(ReusePort, b,  bus_property_get_tristate, 
offsetof(Socket,

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

2013-12-13 Thread Shawn Landden 
If Distribute=n, turns SO_REUSEPORT on, and spawns
n workers to handling incoming requests.

SO_REUSEPORT sockets on the same port must all be created
by the same uid, therefore using the option allows
other root programs (or programs of the same user
if running in --user mode) to hijack this port, even
after systemd reserves it.

We spawn workers at a rate approximentally reverse
exponentially proportianal to the number of incoming connections.
Faster based on the time for new workers to start accept()ing
and their load, or slower if systemd is under load.
---
 TODO  |  3 +-
 man/systemd.socket.xml| 15 +++-
 src/core/dbus-socket.c|  4 +--
 src/core/load-fragment-gperf.gperf.m4 |  3 +-
 src/core/service.c|  4 +--
 src/core/socket.c | 68 ++-
 src/core/socket.h |  5 ++-
 src/shared/conf-parser.c  | 32 +
 src/shared/conf-parser.h  |  1 +
 9 files changed, 101 insertions(+), 34 deletions(-)

diff --git a/TODO b/TODO
index 0b43888..2abe1b4 100644
--- a/TODO
+++ b/TODO
@@ -73,7 +73,7 @@ Features:
 
 * rfkill,backlight: we probably should run the load tools inside of the udev 
rules so that the state is properly initialized by the time other software sees 
it
 
-* Add a new Distribute=$NUMBER key to socket units that makes use of 
SO_REUSEPORT to distribute network traffic on $NUMBER instances
+* tmpfiles: when applying ownership to /run/log/journal, also do this for the 
journal fails contained in it
 
 * move config_parse_path_strv() out of conf-parser.c
 
@@ -181,7 +181,6 @@ Features:
 * teach ConditionKernelCommandLine= globs or regexes (in order to match 
foobar={no,0,off})
 
 * Support SO_REUSEPORT with socket activation:
-  - Let systemd maintain a pool of servers.
   - Use for seamless upgrades, by running the new server before stopping the
 old.
 
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 7c10c58..6799020 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -404,7 +404,8 @@
 designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation. Incompatible with
+
varnameDistribute=/varname/para/listitem
 /varlistentry
 
 varlistentry
@@ -519,6 +520,18 @@
 /varlistentry
 
 varlistentry
+termvarnameDistribute=/varname/term
+listitemparaTakes 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 varnameReusePort=/varname 
above.
+Incompatible with 
varnameAccept=true/varname./para/listitem
+/varlistentry
+
+varlistentry
 termvarnameSmackLabel=/varname/term
 termvarnameSmackLabelIPIn=/varname/term
 
termvarnameSmackLabelIPOut=/varname/term
diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c
index 74217df..036c9af 100644
--- a/src/core/dbus-socket.c
+++ b/src/core/dbus-socket.c
@@ -40,7 +40,6 @@ static int property_get_listen(
 void *userdata,
 sd_bus_error *error) {
 
-
 Socket *s = SOCKET(userdata);
 SocketPort *p;
 int r;
@@ -115,7 +114,8 @@ const sd_bus_vtable bus_socket_vtable[] = {
 SD_BUS_PROPERTY(MessageQueueMaxMessages, x, bus_property_get_long, 
offsetof(Socket, mq_maxmsg), 0),
 SD_BUS_PROPERTY(MessageQueueMessageSize, x, bus_property_get_long, 
offsetof(Socket, mq_msgsize), 0),
 SD_BUS_PROPERTY(Result, s, property_get_result, offsetof(Socket, 
result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
-SD_BUS_PROPERTY(ReusePort, b,  bus_property_get_bool, 
offsetof(Socket, reuse_port), 0),
+SD_BUS_PROPERTY(ReusePort, b,  bus_property_get_tristate, 
offsetof(Socket, reuse_port), 0),
+SD_BUS_PROPERTY(Distribute, u,  bus_property_get_unsigned, 
offsetof(Socket, distribute), 0),
 SD_BUS_PROPERTY(SmackLabel, s, NULL, offsetof(Socket, smack), 0),
 SD_BUS_PROPERTY(SmackLabelIPIn, s, NULL, offsetof(Socket, 
smack_ip_in), 0),

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

2013-12-12 Thread Shawn Landden 
If Distribute=n, turns SO_REUSEPORT on, and spawns
n workers to handling incoming requests.

SO_REUSEPORT sockets on the same port must all be created
by the same uid, therefore using the option allows
other root programs (or programs of the same user
if running in --user mode) to hijack this port, even
after systemd reserves it.

This patch is currently not ready for merging because it is
possible to do quite a number of operations, and yet have no assurance
that we have accomplished anything, ending up in an
infinite loop respawning a service that never accept()s
the socket passed to it, and therefore leaving our copy
of the socket always in EPOLLIN status. To prevent this we need
to do one of:

 *) Call accept() and pass an additional fd to the service.
 *) Use EPOLLET: requires event to always dispatched when it comes in.
 *) Disable and then reenable the event source (requiring
 that we re-create the socket) every time we
 enqueue an instance, essentially emulating the behavior
 of EPOLLET.
---
 TODO  |  3 +-
 man/systemd.socket.xml| 15 ++-
 src/core/dbus-socket.c|  4 +-
 src/core/load-fragment-gperf.gperf.m4 |  3 +-
 src/core/socket.c | 76 ---
 src/core/socket.h |  5 ++-
 src/shared/conf-parser.c  | 32 +++
 src/shared/conf-parser.h  |  1 +
 8 files changed, 109 insertions(+), 30 deletions(-)

diff --git a/TODO b/TODO
index dad55c4..fbc2609 100644
--- a/TODO
+++ b/TODO
@@ -73,7 +73,7 @@ Features:
 
 * rfkill,backlight: we probably should run the load tools inside of the udev 
rules so that the state is properly initialized by the time other software sees 
it
 
-* Add a new Distribute=$NUMBER key to socket units that makes use of 
SO_REUSEPORT to distribute network traffic on $NUMBER instances
+* tmpfiles: when applying ownership to /run/log/journal, also do this for the 
journal fails contained in it
 
 * move config_parse_path_strv() out of conf-parser.c
 
@@ -187,7 +187,6 @@ Features:
 * teach ConditionKernelCommandLine= globs or regexes (in order to match 
foobar={no,0,off})
 
 * Support SO_REUSEPORT with socket activation:
-  - Let systemd maintain a pool of servers.
   - Use for seamless upgrades, by running the new server before stopping the
 old.
 
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 7c10c58..6799020 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -404,7 +404,8 @@
 designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation. Incompatible with
+
varnameDistribute=/varname/para/listitem
 /varlistentry
 
 varlistentry
@@ -519,6 +520,18 @@
 /varlistentry
 
 varlistentry
+termvarnameDistribute=/varname/term
+listitemparaTakes 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 varnameReusePort=/varname 
above.
+Incompatible with 
varnameAccept=true/varname./para/listitem
+/varlistentry
+
+varlistentry
 termvarnameSmackLabel=/varname/term
 termvarnameSmackLabelIPIn=/varname/term
 
termvarnameSmackLabelIPOut=/varname/term
diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c
index 74217df..036c9af 100644
--- a/src/core/dbus-socket.c
+++ b/src/core/dbus-socket.c
@@ -40,7 +40,6 @@ static int property_get_listen(
 void *userdata,
 sd_bus_error *error) {
 
-
 Socket *s = SOCKET(userdata);
 SocketPort *p;
 int r;
@@ -115,7 +114,8 @@ const sd_bus_vtable bus_socket_vtable[] = {
 SD_BUS_PROPERTY(MessageQueueMaxMessages, x, bus_property_get_long, 
offsetof(Socket, mq_maxmsg), 0),
 SD_BUS_PROPERTY(MessageQueueMessageSize, x, bus_property_get_long, 
offsetof(Socket, mq_msgsize), 0),
 SD_BUS_PROPERTY(Result, s, property_get_result, offsetof(Socket, 
result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
-SD_BUS_PROPERTY(ReusePort, b,

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

2013-12-11 Thread Lennart Poettering 
On Tue, 10.12.13 18:53, Shawn Landden (sh...@churchofgit.com) wrote:

 @@ -116,6 +115,7 @@ const sd_bus_vtable bus_socket_vtable[] = {
  SD_BUS_PROPERTY(MessageQueueMessageSize, x, 
 bus_property_get_long, offsetof(Socket, mq_msgsize), 0),
  SD_BUS_PROPERTY(Result, s, property_get_result, offsetof(Socket, 
 result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
  SD_BUS_PROPERTY(ReusePort, b,  bus_property_get_bool,
  offsetof(Socket, reuse_port), 0),

Did you turn reuseport into a tristate as suggested? If so, then
bus_property_get_bool needs to become bus_property_get_tristate here so
that the right type is serialized to dbus.

 +Socket.ReusePort,config_parse_bool, -1,  
offsetof(Socket, reuse_port)

Similar here... (and what's the -1?)

As it turns out I recently removed config_parse_tristate() as part of
9588bc32096fc8342bfd8b989689717186d7d86e. You should probably restore
that function from that commit to make use of it here...


 +Socket *socket = SOCKET(UNIT_DEREF(s-accept_socket));
  log_debug_unit(u-id,
 %s: got READY=1, u-id);
  
 +if (socket  socket-distribute  socket-n_connections)
 +socket_enter_listening(socket);
 +

Hmm, I would prefer if we wouldn't reach over too much into the socket
here, i.e. handle this within socket_trigger_notify(). That function is
always called when a unit that is triggered by the socket changes state.

 +if (s-reuse_port == -1) {

I'd prefer not checking against specific values,but really just if 
0,  0, or == 0. Checking against specific values would kinda suggest
that there are more than three values.

if (s-reuse_port  0) ...

Also, please do this part as part of socket_load() somewhere, so that
the  0 state is turned into  0 or == 0 already when loading the unit.

 -if (cfd  0) {
 +if (cfd  0  !(s-distribute)) {

The extra ()... Also, please use == 0 here since it is a number...

(Same fix at a few other places...)

 +if (s-distribute  0  (s-n_connections = s-distribute 
 || SERVICE(UNIT_DEREF(s-service))-type == SERVICE_NOTIFY))
 +socket_set_state(s, SOCKET_RUNNING);
 +

Why do you check for SERVICE_NOTIFY here?

Also, maybe I missed something, but if SO_REUSEPORT is used you need to
create a new socket for each connection and bind it to the same
address. You have to duplicate the existing socket, but not with dup(),
but instead by creating a new one with the same parameters and with
SO_REUSEPORT... The first instance this spawns should get the original
socket though, but all others should get one of these copies...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2013-12-11 Thread Zbigniew Jędrzejewski-Szmek 
On Tue, Dec 10, 2013 at 06:53:33PM -0800, Shawn Landden wrote:
Please add a commit message that says what the commit does.

This part can remain, but please prefix it with Some open
questions remain: or something.
 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.
 
 With Type=notify, we wait until a service tells us it is ready
 before we listen again and thereby start up more instances.
 
 What if someone want to use the templating namespace ('@')
 with Distribute=?

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2013-12-10 Thread Shawn Landden 
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.

With Type=notify, we wait until a service tells us it is ready
before we listen again and thereby start up more instances.

What if someone want to use the templating namespace ('@')
with Distribute=?
---
 TODO  |  3 +-
 man/systemd.socket.xml| 15 +++-
 src/core/dbus-socket.c|  2 +-
 src/core/load-fragment-gperf.gperf.m4 |  3 +-
 src/core/service.c|  4 ++
 src/core/socket.c | 72 ---
 src/core/socket.h |  8 +++-
 7 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/TODO b/TODO
index 2fb9cd3..697d568 100644
--- a/TODO
+++ b/TODO
@@ -69,7 +69,7 @@ Features:
 
 * rfkill,backlight: we probably should run the load tools inside of the udev 
rules so that the state is properly initialized by the time other software sees 
it
 
-* Add a new Distribute=$NUMBER key to socket units that makes use of 
SO_REUSEPORT to distribute network traffic on $NUMBER instances
+* tmpfiles: when applying ownership to /run/log/journal, also do this for the 
journal fails contained in it
 
 * we probably should replace the left-over uses of strv_append() and replace 
them by strv_push() or strv_extend()
 
@@ -187,7 +187,6 @@ Features:
 * teach ConditionKernelCommandLine= globs or regexes (in order to match 
foobar={no,0,off})
 
 * Support SO_REUSEPORT with socket activation:
-  - Let systemd maintain a pool of servers.
   - Use for seamless upgrades, by running the new server before stopping the
 old.
 
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 7c10c58..6799020 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -404,7 +404,8 @@
 designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation. Incompatible with
+
varnameDistribute=/varname/para/listitem
 /varlistentry
 
 varlistentry
@@ -519,6 +520,18 @@
 /varlistentry
 
 varlistentry
+termvarnameDistribute=/varname/term
+listitemparaTakes 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 varnameReusePort=/varname 
above.
+Incompatible with 
varnameAccept=true/varname./para/listitem
+/varlistentry
+
+varlistentry
 termvarnameSmackLabel=/varname/term
 termvarnameSmackLabelIPIn=/varname/term
 
termvarnameSmackLabelIPOut=/varname/term
diff --git a/src/core/dbus-socket.c b/src/core/dbus-socket.c
index 74217df..68c95a0 100644
--- a/src/core/dbus-socket.c
+++ b/src/core/dbus-socket.c
@@ -40,7 +40,6 @@ static int property_get_listen(
 void *userdata,
 sd_bus_error *error) {
 
-
 Socket *s = SOCKET(userdata);
 SocketPort *p;
 int r;
@@ -116,6 +115,7 @@ const sd_bus_vtable bus_socket_vtable[] = {
 SD_BUS_PROPERTY(MessageQueueMessageSize, x, bus_property_get_long, 
offsetof(Socket, mq_msgsize), 0),
 SD_BUS_PROPERTY(Result, s, property_get_result, offsetof(Socket, 
result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
 SD_BUS_PROPERTY(ReusePort, b,  bus_property_get_bool, 
offsetof(Socket, reuse_port), 0),
+SD_BUS_PROPERTY(Distribute, u,  bus_property_get_unsigned, 
offsetof(Socket, distribute), 0),
 SD_BUS_PROPERTY(SmackLabel, s, NULL, offsetof(Socket, smack), 0),
 SD_BUS_PROPERTY(SmackLabelIPIn, s, NULL, offsetof(Socket, 
smack_ip_in), 0),
 SD_BUS_PROPERTY(SmackLabelIPOut, s, NULL, offsetof(Socket, 
smack_ip_out), 0),
diff --git a/src/core/load-fragment-gperf.gperf.m4 
b/src/core/load-fragment-gperf.gperf.m4