Re: Need some help in HAPROXY setup.

[Aleksandar Lazic]

Hi all.

I resend this just to highlight the question from below.

> Finally the question is, afaiu, how to balance based on test_ha AND
> counter_out AND lua.parseElement

This are different tables.

Best regards
Aleks

Am 09.03.2018 um 18:28 schrieb Aleksandar Lazic:
> Hi Amit.
> 
> *Please keep the mailing list in loop, thanks.*
> 
> In case you haven't received my last mail here is the web link to the
> maillinglist archive.
> 
> https://www.mail-archive.com/haproxy@formilux.org/msg29252.html
> 
> Am 09.03.2018 um 07:43 schrieb amit raj:
> 
>> Hello Alex,
>>
>> Two things we have to achieve with the HAPROXY to be very clear.
>>
>> 1.The most important thing we want to achieve is uniform Load Balancing
>> (with sessions and bytes in and bytes out parameter should be uniform
>> across all the backend).
> 
> Let me describe what I have understood.
> 
> You want to use more the one balancing decision base.
> 
> * session (cookie, url, ...)
> * bytes in. I assume from the client
> * bytes out. I assume from the client
> 
> I this right? # <= Please answer this question, thanks.
> 
> Please can you describe in a ASCII flow how the decision flow should work.
> 
> What I have seen in the doc are this acl samples
> 
> https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#7.3.1-table_bytes_in_rate
> 
> https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#7.3.1-table_bytes_out_rate
> 
> 
> But I'm not sure if this is what you need.
> 
>> 2.When we have tried using keep-alive mode for achieving uniform load
>> balancing , the HAPROXY is sending RST for closing the connection with
>> the backend , this is what we really do not want .As per our use case
>> HAPROXY should use FIN to close the backend connection.
>>
>> Currently we are using "option-http-tunnel" in the frontend of the
>> application and "no-http-tunnel" and "keep-alive" in the backend this
>> solves our RST issue and HAPROXY is closing the connection properly with
>> FIN.But the uniform Load Balancing  as I mentioned in point one got
>> screwed up.
>>
>> Following are  my basic HAPROXY config we are using for testing and
>> fixing  the issue.
> 
> Thank you for the full haproxy config.
> 
>> global
>>
>>   log /dev/log    local0
>>   log /dev/log    local1 notice
>> #    lua-load /etc/haproxy/parseJsonValue.lua
>>   stats socket /var/run/haproxy/haproxy.sock mode 777 level admin
>>   expose-fd listeners
>>   chroot /var/lib/haproxy
>>   #lua-load /etc/haproxy/lua
>>   stats timeout 30s
>>   user haproxy
>>   group haproxy
>>   #    daemon
>>   maxconn 2000
>>
>> defaults
>>
>>   log global
>>   mode    http
>>   retries 3
>>   option  httplog
>>   #option http-server-close
>>   option  dontlognull
>>   option forwardfor
>>   timeout connect 5000
>>   timeout client  5
>>   timeout server  5
>>   errorfile 400 /etc/haproxy/errors/400.http
>>   errorfile 403 /etc/haproxy/errors/403.http
>>   errorfile 408 /etc/haproxy/errors/408.http
>>   errorfile 500 /etc/haproxy/errors/500.http
>>   errorfile 502 /etc/haproxy/errors/502.http
>>   errorfile 503 /etc/haproxy/errors/503.http
>>   errorfile 504 /etc/haproxy/errors/504.http
>>
>> #--
>> # internal statistics
>> #--
>>
>> listen stats
>>   bind ipv4@:8999
>>   mode http
>>   stats enable
>>   stats uri  /stats
>> #--
>> # frontend instances
>> #--
>>
>> frontend test_ha
>>
>>   bind ipv4@:8080
>>   option http-tunnel
>>   #    http-request add-header X-Internal-Value %[lua.parseJsonValue]
>>   default_backend test_back
>>
>> #--
>> # backend instances
>> #--
>>
>> backend test_back
>>
>>   #  http-request set-header X-LB %[req.hdr(Host),lower]%[req.uri,lower]
>>   balance roundrobin
>>
>>   no option http-tunnel
>>   option http-keep-alive>    # option http-server-close
>>   #option http-buffer-request
>>
>> #    stick-table type string size 30k expire 30m
>> #    stick on "lua.parseElement" table test_back
>>
>>   server test2 test02.xyz.abc.com:80 
>>   server test3 test03.xy.abc.com:80 
>>
>> Please let me know if you need any other info.
> 
> Ah I think I understand now what you mean.
> 
> https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#4-stick-table
> 
> I haven't used stick-table very extensive so I hope a more experienced
> Person will correct my suggestion.
> 
> --
> frontend test_ha
> 
>     bind ipv4@:8080
>     option http-tunnel
> #    http-request add-header X-Internal-Value %[lua.parseJsonValue]
> 
>     stick-table type integer size 30k expire 30m store bytes_in_cnt
> 
>     default_backend counter_out
> 
> backend counter_out
>     bind ipv4@:8081
>     option http-tunnel
> 
>     stick-table type integer size 30k expire 30m store bytes_out_cnt
>     default_backend test_ha
> 
> backend be_test_ha
> 
>  bind ipv4@:8082
>  option http-tunnel
> 
>  #    

Re: Order of acls not important?

[Tim Düsterhus]

Stefan,

Am 15.03.2018 um 21:24 schrieb Stefan Husch|qutic development:
> frontend 10.10.10.10_80
>bind 10.10.10.10:80 name 10.10.10.10:80 
>mode http
>option http-keep-alive
>option log-separate-errors
>option httplog
>acl acl_1 path_beg -i /.well-known/acme-challenge/
>use_backend acme_challenge_backend if acl_1
>acl acl_2 hdr_reg(host) -i \b(?:\d{1,3}\.){3}\d{1,3}\b
>http-request redirect code 301 location https://example.com if acl_2
>acl acl_3 req.proto_http
>http-request redirect scheme https code 301 if acl_3
> 
> I thought the acls are processed from 1 to 3, but the curl result is not 
> going to the acme_challenge_backend, but doing a https redirect.

The ACLs order is not relevant, I recommend to group them all together
at the top (and give them meaningful names) and then act on them at the
bottom. Here's an example excerpt of my config:

> acl  acme_challenge  path_beg  /.well-known/acme-challenge/
> acl  example.com hdr(host)  -i  example.com
> # repeat for other domains
>
> redirect  code  301  scheme  https  if  !acme_challenge 
> example.com
> # repeat for other domains
>
> use_backend bk_letsencrypt if acme_challenge

Basically add the !acme_challenge to your redirects. Another possibility
is to add the acme-challenge logic to your HTTPS backend, Let's Encrypt
follows redirects. This does require an existing certificate for your
domains though.

The HTTP protocol (without 's') is implied for your frontend, as you
only listen on port 80, that might simplify your configuration as well.

Best regards
Tim Düsterhus

[PATCH] BUG/MINOR: cli: Ensure all command outputs end with a LF

[Aurélien Nephtali]

Hello,

This patch adds some missing LF to the outputs of some commands.
It may break some scripts that rely on these broken outputs but it also
breaks the parsing of pipelined commands.

-- 
Aurélien.
>From 058559ce78d05c87e7c19fd7866a52ccce608a39 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= 
Date: Thu, 15 Mar 2018 21:48:50 +0100
Subject: [PATCH] BUG/MINOR: cli: Ensure all command outputs end with a LF
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since 200b0fac ("MEDIUM: Add support for updating TLS ticket keys via
socket"), 4147b2ef ("MEDIUM: ssl: basic OCSP stapling support."),
4df59e9 ("MINOR: cli: add socket commands and config to prepend
informational messages with severity") and 654694e1 ("MEDIUM: stats/cli:
add support for "set table key" to enter values"), commands
'set ssl tls-key', 'set ssl ocsp-response', 'set severity-output' and
'set table' do not always send an extra LF at the end of their outputs.

This is required as mentioned in doc/management.txt:

"Since multiple commands may be issued at once, haproxy uses the empty
line as a delimiter to mark an end of output for each command"

Signed-off-by: Aurélien Nephtali 
---
 src/cli.c | 2 +-
 src/ssl_sock.c| 4 ++--
 src/stick_table.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/cli.c b/src/cli.c
index 84b6229b7..7bffbdd56 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -1145,7 +1145,7 @@ static int cli_parse_set_severity_output(char **args, struct appctx *appctx, voi
 		return 0;
 
 	appctx->ctx.cli.severity = LOG_ERR;
-	appctx->ctx.cli.msg = "one of 'none', 'number', 'string' is a required argument";
+	appctx->ctx.cli.msg = "one of 'none', 'number', 'string' is a required argument\n";
 	appctx->st0 = CLI_ST_PRINT;
 	return 1;
 }
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index daf428247..5acf38f60 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -8554,7 +8554,7 @@ static int cli_parse_set_tlskeys(char **args, struct appctx *appctx, void *priva
 	}
 	ssl_sock_update_tlskey_ref(ref, );
 	appctx->ctx.cli.severity = LOG_INFO;
-	appctx->ctx.cli.msg = "TLS ticket key updated!";
+	appctx->ctx.cli.msg = "TLS ticket key updated!\n";
 	appctx->st0 = CLI_ST_PRINT;
 	return 1;
 
@@ -8591,7 +8591,7 @@ static int cli_parse_set_ocspresponse(char **args, struct appctx *appctx, void *
 		return 1;
 	}
 	appctx->ctx.cli.severity = LOG_INFO;
-	appctx->ctx.cli.msg = "OCSP Response updated!";
+	appctx->ctx.cli.msg = "OCSP Response updated!\n";
 	appctx->st0 = CLI_ST_PRINT;
 	return 1;
 #else
diff --git a/src/stick_table.c b/src/stick_table.c
index fe26e3121..73c70d3e2 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -3334,7 +3334,7 @@ static int table_prepare_data_request(struct appctx *appctx, char **args)
 {
 	if (appctx->ctx.table.action != STK_CLI_ACT_SHOW && appctx->ctx.table.action != STK_CLI_ACT_CLR) {
 		appctx->ctx.cli.severity = LOG_ERR;
-		appctx->ctx.cli.msg = "content-based lookup is only supported with the \"show\" and \"clear\" actions";
+		appctx->ctx.cli.msg = "content-based lookup is only supported with the \"show\" and \"clear\" actions\n";
 		appctx->st0 = CLI_ST_PRINT;
 		return 1;
 	}
-- 
2.11.0

Re: Order of acls not important?

[PiBa-NL]

Hi,

Op 15-3-2018 om 21:24 schreef Stefan Husch|qutic development:

I thought the acls are processed from 1 to 3,

Acl's are evaluated where they are used.


What am I doing wrong? Is the acl-position in a haproxy-config not important?

Thx, Stefan


The order of the acl's themselves is not relevant.

However you should iirc get a warning that the http-request will be 
processed before the use_backend directive.


Regards,

PiBa-NL

Order of acls not important?

[Stefan Husch|qutic development]

Hi,

this might be asked before, but I didn´t found the answer yet.

The following haproxy.config has the goal to use lets encrypt and https 
redirect together on a frontend.

frontend 10.10.10.10_80
bind 10.10.10.10:80 name 10.10.10.10:80 
mode http
option http-keep-alive
option log-separate-errors
option httplog
acl acl_1 path_beg -i /.well-known/acme-challenge/
use_backend acme_challenge_backend if acl_1
acl acl_2 hdr_reg(host) -i \b(?:\d{1,3}\.){3}\d{1,3}\b
http-request redirect code 301 location https://example.com if acl_2
acl acl_3 req.proto_http
http-request redirect scheme https code 301 if acl_3

I thought the acls are processed from 1 to 3, but the curl result is not going 
to the acme_challenge_backend, but doing a https redirect.

$ curl -i http://example.com/.well-known/acme-challenge/
HTTP/1.1 301 Moved Permanently
Content-length: 0
Location: https://example.com/.well-known/acme-challenge/

What am I doing wrong? Is the acl-position in a haproxy-config not important?

Thx, Stefan

[MINOR][PATCH] Fix segfault when trying to use seemless reload with at least an interface bound

[Olivier Houchard]

Hi,

Trying to do a seemless reload while at least one socket has been bound to
a specifig interface will lead to a segfault, because the guy who wrote that
code did it by copying/pasting, and forgot to change an instance of
"namespace" to "iface".
The attached patch should fix it.

Regards,

Olivier
>From b249119e571a1b5c597819701e5ec6f7d4525cf8 Mon Sep 17 00:00:00 2001
From: Olivier Houchard 
Date: Thu, 15 Mar 2018 17:48:49 +0100
Subject: [PATCH] MINOR: seemless reload: Fix crash when an interface is
 specified.

When doing a seemless reload, while receiving the sockets from the old process
the new process will die if the socket has been bound to a specific
interface.
This happens because the code that tries to parse the informations bogusly
try to set xfer_sock->namespace, while it should be setting wfer_sock->iface.

This should be backported to 1.8.
---
 src/haproxy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index 4b740e135..e3e89c031 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1180,7 +1180,7 @@ static int get_old_sockets(const char *unixsocket)
goto out;
}
memcpy(xfer_sock->iface, [curoff], len);
-   xfer_sock->namespace[len] = 0;
+   xfer_sock->iface[len] = 0;
curoff += len;
}
if (curoff + sizeof(int) > maxoff) {
-- 
2.14.3

Re: segfault in haproxy=1.8.4

[Christopher Faulet]

Le 15/03/2018 à 15:50, Willy Tarreau a écrit :

On Thu, Mar 15, 2018 at 02:49:59PM +0100, Christopher Faulet wrote:

When we scan a queue, it is locked. So on your mark, the server queue is
already locked. To remove a pendconn from a queue, we also need to have a
lock on this queue, if it is still linked. Else we can safely remove it,
nobody can fall on the pendconn by scanning the queue.


OK that's exactly the information I was missing, so I think all is good
then!


In fact without locking the queue (server one and/or proxy one), it is
unsafe to loop on it. In same way, we must lock the queue to remove a
pendconn from it. I will add comments to clarify everything. But, I really
think it is thread-safe.


Absolutely. Thanks for the detailed explanation!



Nice, I'm relieved. You made me doubt myself :)
Here is the updated patch.

Thanks
--
Christopher Faulet
>From 91b1349b6a1a64d43cc41e8546ff1d1ce17a8e14 Mon Sep 17 00:00:00 2001
From: Christopher Faulet 
Date: Wed, 14 Mar 2018 16:18:06 +0100
Subject: [PATCH] BUG/MAJOR: threads/queue: Fix thread-safety issues on the
 queues management

The management of the servers and the proxies queues was not thread-safe at
all. First, the accesses to ->pend_pos were not protected. So it was
possible to release it on a thread (for instance because the stream is released)
and to use it in same time on another one (because we redispatch pending
connections for a server). Then, the accesses to stream's information (flags and
target) from anywhere is forbidden. To be safe, The stream's state must always
be updated in the context of process_stream.

So to fix these issues, the queue module has been refactored. A lock has been
added in the pendconn structure. And now, when we try to dequeue a pending
connection, we start by unlinking it from the server/proxy queue and we wake up
the stream. Then, it is the stream reponsibility to really dequeue it (or
release it). This way, we are sure that only the stream can create and release
its  field.

However, be careful. This new implementation should be thread-safe
(hopefully...). But it is not optimal and in some situations, it could be really
slower in multi-threaded mode than in single-threaded one. The problem is that,
when we try to dequeue pending connections, we process it from the older one to
the newer one independently to the thread's affinity. So we need to wait the
other threads' wakeup to really process them. If threads are blocked in the
poller, this will add a significant latency. This problem happens when maxconn
values are very low.

This patch must be backported in 1.8.
---
 include/common/hathreads.h |   2 +
 include/proto/queue.h  |   1 +
 include/types/queue.h  |  10 +-
 include/types/stream.h |   2 +-
 src/proto_http.c   |   2 -
 src/queue.c| 324 +++--
 src/stream.c   |   3 +-
 7 files changed, 209 insertions(+), 135 deletions(-)

diff --git a/include/common/hathreads.h b/include/common/hathreads.h
index 3da6dba4..19299db7 100644
--- a/include/common/hathreads.h
+++ b/include/common/hathreads.h
@@ -240,6 +240,7 @@ enum lock_label {
 	PIPES_LOCK,
 	START_LOCK,
 	TLSKEYS_REF_LOCK,
+	PENDCONN_LOCK,
 	LOCK_LABELS
 };
 struct lock_stat {
@@ -360,6 +361,7 @@ static inline const char *lock_label(enum lock_label label)
 	case PIPES_LOCK:   return "PIPES";
 	case START_LOCK:   return "START";
 	case TLSKEYS_REF_LOCK: return "TLSKEYS_REF";
+	case PENDCONN_LOCK:return "PENDCONN";
 	case LOCK_LABELS:  break; /* keep compiler happy */
 	};
 	/* only way to come here is consecutive to an internal bug */
diff --git a/include/proto/queue.h b/include/proto/queue.h
index f66d809f..2d4773a0 100644
--- a/include/proto/queue.h
+++ b/include/proto/queue.h
@@ -38,6 +38,7 @@ extern struct pool_head *pool_head_pendconn;
 
 int init_pendconn();
 struct pendconn *pendconn_add(struct stream *strm);
+int pendconn_dequeue(struct stream *strm);
 void pendconn_free(struct pendconn *p);
 void process_srv_queue(struct server *s);
 unsigned int srv_dynamic_maxconn(const struct server *s);
diff --git a/include/types/queue.h b/include/types/queue.h
index 4b354514..42dbbd04 100644
--- a/include/types/queue.h
+++ b/include/types/queue.h
@@ -24,15 +24,19 @@
 
 #include 
 #include 
+#include 
 
 #include 
 
 struct stream;
 
 struct pendconn {
-	struct list list;		/* chaining ... */
-	struct stream *strm;		/* the stream waiting for a connection */
-	struct server *srv;		/* the server we are waiting for */
+	intstrm_flags; /* stream flags */
+	struct stream *strm;
+	struct proxy  *px;
+	struct server *srv;/* the server we are waiting for, may be NULL */
+	struct listlist;   /* next pendconn */
+	__decl_hathreads(HA_SPINLOCK_T lock);
 };
 
 #endif /* _TYPES_QUEUE_H */
diff --git a/include/types/stream.h b/include/types/stream.h
index 227b0ffb..0dbc79f4 100644
--- 

Re: segfault in haproxy=1.8.4

[Willy Tarreau]

On Thu, Mar 15, 2018 at 02:49:59PM +0100, Christopher Faulet wrote:
> When we scan a queue, it is locked. So on your mark, the server queue is
> already locked. To remove a pendconn from a queue, we also need to have a
> lock on this queue, if it is still linked. Else we can safely remove it,
> nobody can fall on the pendconn by scanning the queue.

OK that's exactly the information I was missing, so I think all is good
then!

> In fact without locking the queue (server one and/or proxy one), it is
> unsafe to loop on it. In same way, we must lock the queue to remove a
> pendconn from it. I will add comments to clarify everything. But, I really
> think it is thread-safe.

Absolutely. Thanks for the detailed explanation!

Willy

Re: segfault in haproxy=1.8.4

[Christopher Faulet]

Le 15/03/2018 à 12:16, Willy Tarreau a écrit :

+static struct stream *pendconn_process_next_strm(struct server *srv, struct 
proxy *px)
   {
+   struct pendconn *p = NULL;
+   struct server   *rsrv;
rsrv = srv->track;
if (!rsrv)
rsrv = srv;
+   if (srv->nbpend) {
+   list_for_each_entry(p, >pendconns, list) {


-- mark here --


+   if (!HA_SPIN_TRYLOCK(PENDCONN_LOCK, >lock))
+   goto ps_found;
+   }
+   p = NULL;
+   }
+
+  ps_found:
+   if (srv_currently_usable(rsrv) && px->nbpend) {
+   struct pendconn *pp;
+
+   list_for_each_entry(pp, >pendconns, list) {
+   /* If the server pendconn is older than the proxy one,
+* we process the server one. */
+   if (p && !tv_islt(>strm->logs.tv_request, 
>strm->logs.tv_request))
+   goto pendconn_found;


Here there is still a race : pp->strm is dereferenced out of the lock,
so the owner could very well decide to disappear in the mean time. I
think this check needs to be moved in the trylock below. Or another
option (probably simpler and cleaner) would be to add a tv_request
field to the pendconn, and directly use it.


Well, not really. When we access to a pendconn in a queue (proxy or server)
we always get a lock on it. In other side, a stream must release its
pendconn, if any, before being released. To do so the right queue must be
locked by the stream. So when we manipulate a pendconn in a queue we are
sure that the stream cannot release it in same time. And if the stream
released it, it cannot be in the queue.

Everything should be thread-safe because I added an lock on the pendconn
_AND_ the pendconn is now only released by the stream itself. These 2
conditions are necessary and sufficient to guarantee the thread-safety.


OK that's fine. I think that you should indicate for certain functions
that they must only be called under this or that lock (I remember seeing
one this morning in this situation, I don't remember which one).

But then what prevents a stream from removing itself from the queue at
the mark above ? It takes the lock, removes itself, unlocks, but  is
still seen by the function, the trylock succeeds (on just freed memory),
the stream is dereferenced again after being freed, and we may even
unlink it twice, immediately corrupting memory. I may be wrong but I
don't see what prevents this case from being possible. If it is, you may
need to use another lock when manipulating this list (likely the server
lock as you initially did), though that could become expensive.



When we scan a queue, it is locked. So on your mark, the server queue is 
already locked. To remove a pendconn from a queue, we also need to have 
a lock on this queue, if it is still linked. Else we can safely remove 
it, nobody can fall on the pendconn by scanning the queue.


In fact without locking the queue (server one and/or proxy one), it is 
unsafe to loop on it. In same way, we must lock the queue to remove a 
pendconn from it. I will add comments to clarify everything. But, I 
really think it is thread-safe.


--
Christopher Faulet

small cleanup of src/dns.c

[Илья Шипицин]

Hello,

small issue (no real impact) identified by cppcheck
From 733d99f42d93898232bb8c3c953b662ee889c034 Mon Sep 17 00:00:00 2001
From: Ilya Shipitsin 
Date: Thu, 15 Mar 2018 16:38:38 +0500
Subject: [PATCH] CLEANUP: remove duplicate code in src/dns.c

issue was identified by cppcheck

[src/dns.c:2037] -> [src/dns.c:2041]: (warning) Variable 'appctx->st2' is reassigned a value before the old one has been used. 'break;' missing?
---
 src/dns.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/dns.c b/src/dns.c
index 280bc155..c49a4c6d 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -2034,9 +2034,6 @@ static int cli_io_handler_dump_resolvers_to_buffer(struct appctx *appctx)
 			return 0;
 		}
 
-		appctx->st2 = STAT_ST_FIN;
-		/* fall through */
-
 	default:
 		appctx->st2 = STAT_ST_FIN;
 		return 1;
-- 
2.14.3

cppcheck finding

[Илья Шипицин]

Hi,

[src/51d.c:373]: (error) Invalid number of character '{' when no macros are
defined.

?

Re: segfault in haproxy=1.8.4

[Willy Tarreau]

On Thu, Mar 15, 2018 at 11:32:41AM +0100, Christopher Faulet wrote:
> I tested with an ugly and really unsafe/buggy hack to migrate the stream and
> its client FD before waking it up, and this problem disappears. So this is
> definitely the right way to handle it. But for now, this could be a problem
> for everyone using maxconn with threads.

Well, it cannot be worse than the current situation! Also, don't worry, as
in general when working with very low maxconn, you face high wake-up rates
because operations are much less batched, so the workloads exhibiting this
case where some tasks are idle waiting for a connection shouldn't be that
common, and that will buy us time to try to address this better.

> > > +static struct stream *pendconn_process_next_strm(struct server *srv, 
> > > struct proxy *px)
> > >   {
> > > + struct pendconn *p = NULL;
> > > + struct server   *rsrv;
> > >   rsrv = srv->track;
> > >   if (!rsrv)
> > >   rsrv = srv;
> > > + if (srv->nbpend) {
> > > + list_for_each_entry(p, >pendconns, list) {

-- mark here --

> > > + if (!HA_SPIN_TRYLOCK(PENDCONN_LOCK, >lock))
> > > + goto ps_found;
> > > + }
> > > + p = NULL;
> > > + }
> > > +
> > > +  ps_found:
> > > + if (srv_currently_usable(rsrv) && px->nbpend) {
> > > + struct pendconn *pp;
> > > +
> > > + list_for_each_entry(pp, >pendconns, list) {
> > > + /* If the server pendconn is older than the proxy one,
> > > +  * we process the server one. */
> > > + if (p && !tv_islt(>strm->logs.tv_request, 
> > > >strm->logs.tv_request))
> > > + goto pendconn_found;
> > 
> > Here there is still a race : pp->strm is dereferenced out of the lock,
> > so the owner could very well decide to disappear in the mean time. I
> > think this check needs to be moved in the trylock below. Or another
> > option (probably simpler and cleaner) would be to add a tv_request
> > field to the pendconn, and directly use it.
> 
> Well, not really. When we access to a pendconn in a queue (proxy or server)
> we always get a lock on it. In other side, a stream must release its
> pendconn, if any, before being released. To do so the right queue must be
> locked by the stream. So when we manipulate a pendconn in a queue we are
> sure that the stream cannot release it in same time. And if the stream
> released it, it cannot be in the queue.
> 
> Everything should be thread-safe because I added an lock on the pendconn
> _AND_ the pendconn is now only released by the stream itself. These 2
> conditions are necessary and sufficient to guarantee the thread-safety.

OK that's fine. I think that you should indicate for certain functions
that they must only be called under this or that lock (I remember seeing
one this morning in this situation, I don't remember which one).

But then what prevents a stream from removing itself from the queue at
the mark above ? It takes the lock, removes itself, unlocks, but  is
still seen by the function, the trylock succeeds (on just freed memory),
the stream is dereferenced again after being freed, and we may even
unlink it twice, immediately corrupting memory. I may be wrong but I
don't see what prevents this case from being possible. If it is, you may
need to use another lock when manipulating this list (likely the server
lock as you initially did), though that could become expensive.

OK for the rest :-)

Willy

Re: segfault in haproxy=1.8.4

[Christopher Faulet]

Le 15/03/2018 à 07:19, Willy Tarreau a écrit :

Hi Christopher,

first, thank you for this one, I know how painful it can have been!


Hi Willy,

Thanks for your support :)


On Wed, Mar 14, 2018 at 09:56:19PM +0100, Christopher Faulet wrote:
(...)

But it is not optimal and in some situations, it could be really
slower in multi-threaded mode than in single-threaded one. The problem is that,
when we try to dequeue pending connections, we process it from the older one to
the newer one independently to the thread's affinity. So we need to wait the
other threads' wakeup to really process them. If threads are blocked in the
poller, this will add a significant latency. This problem happens when maxconn
values are very low.


This problem is not related to your patch but to the task scheduler, which
we need to improve^Wfix so that it can properly wake up a task on a sleeping
thread. We need to check this with Emeric and Olivier to see how to best
address it in 1.8 and how to ensure that 1.9 is clean regarding this.



I tested with an ugly and really unsafe/buggy hack to migrate the stream 
and its client FD before waking it up, and this problem disappears. So 
this is definitely the right way to handle it. But for now, this could 
be a problem for everyone using maxconn with threads.



I'm having a few comments on the code below :


@@ -97,44 +76,66 @@ static inline struct pendconn *pendconn_from_px(const 
struct proxy *px) {
   * The stream is immediately marked as "assigned", and both its  and
   *  are set to ,
   */
-static struct stream *pendconn_get_next_strm(struct server *srv, struct proxy 
*px)
+static struct stream *pendconn_process_next_strm(struct server *srv, struct 
proxy *px)
  {
-   struct pendconn *ps, *pp;
-   struct stream *strm;
-   struct server *rsrv;
+   struct pendconn *p = NULL;
+   struct server   *rsrv;
  
  	rsrv = srv->track;

if (!rsrv)
rsrv = srv;
  
-	ps = pendconn_from_srv(srv);

-   pp = pendconn_from_px(px);
-   /* we want to get the definitive pendconn in  */
-   if (!pp || !srv_currently_usable(rsrv)) {
-   if (!ps)
-   return NULL;
-   } else {
-   /* pendconn exists in the proxy queue */
-   if (!ps || tv_islt(>strm->logs.tv_request, 
>strm->logs.tv_request))
-   ps = pp;
+   if (srv->nbpend) {
+   list_for_each_entry(p, >pendconns, list) {
+   if (!HA_SPIN_TRYLOCK(PENDCONN_LOCK, >lock))
+   goto ps_found;
+   }
+   p = NULL;
+   }
+
+  ps_found:
+   if (srv_currently_usable(rsrv) && px->nbpend) {
+   struct pendconn *pp;
+
+   list_for_each_entry(pp, >pendconns, list) {
+   /* If the server pendconn is older than the proxy one,
+* we process the server one. */
+   if (p && !tv_islt(>strm->logs.tv_request, 
>strm->logs.tv_request))
+   goto pendconn_found;


Here there is still a race : pp->strm is dereferenced out of the lock,
so the owner could very well decide to disappear in the mean time. I
think this check needs to be moved in the trylock below. Or another
option (probably simpler and cleaner) would be to add a tv_request
field to the pendconn, and directly use it.


Well, not really. When we access to a pendconn in a queue (proxy or 
server) we always get a lock on it. In other side, a stream must release 
its pendconn, if any, before being released. To do so the right queue 
must be locked by the stream. So when we manipulate a pendconn in a 
queue we are sure that the stream cannot release it in same time. And if 
the stream released it, it cannot be in the queue.


Everything should be thread-safe because I added an lock on the pendconn 
_AND_ the pendconn is now only released by the stream itself. These 2 
conditions are necessary and sufficient to guarantee the thread-safety.



+  pendconn_found:
+   pendconn_unlinked(p);
+   p->strm_flags  = (p->strm_flags & (SF_DIRECT | SF_ADDR_SET));
+   p->strm_flags |= SF_ASSIGNED;


I think that it would be simpler to consider that strm_flags start with
the stream's initial flags, then only manipulate the ones we're interested
in, and only apply the final mask at the end. This way the enumeration of
all manipulated flags doesn't need to be known at each location. This means
you should only keep SF_ASSIGNED above and drop the line filterinng on the
two other flags. See below for the other instances.


Right, I will change that.

[...]


-/*
- * Detaches pending connection , decreases the pending count, and frees
- * the pending connection. The connection might have been queued to a specific
- * server as well as to the proxy. The stream also gets marked unqueued.
+/* Try to dequeue pending connection attached to the stream . It must
+ * always exists here. If the 

Re: segfault in haproxy=1.8.4

[Willy Tarreau]

Hi Christopher,

first, thank you for this one, I know how painful it can have been!

On Wed, Mar 14, 2018 at 09:56:19PM +0100, Christopher Faulet wrote:
(...)
> But it is not optimal and in some situations, it could be really
> slower in multi-threaded mode than in single-threaded one. The problem is 
> that,
> when we try to dequeue pending connections, we process it from the older one 
> to
> the newer one independently to the thread's affinity. So we need to wait the
> other threads' wakeup to really process them. If threads are blocked in the
> poller, this will add a significant latency. This problem happens when maxconn
> values are very low.

This problem is not related to your patch but to the task scheduler, which
we need to improve^Wfix so that it can properly wake up a task on a sleeping
thread. We need to check this with Emeric and Olivier to see how to best
address it in 1.8 and how to ensure that 1.9 is clean regarding this.

I'm having a few comments on the code below :

> @@ -97,44 +76,66 @@ static inline struct pendconn *pendconn_from_px(const 
> struct proxy *px) {
>   * The stream is immediately marked as "assigned", and both its  and
>   *  are set to ,
>   */
> -static struct stream *pendconn_get_next_strm(struct server *srv, struct 
> proxy *px)
> +static struct stream *pendconn_process_next_strm(struct server *srv, struct 
> proxy *px)
>  {
> - struct pendconn *ps, *pp;
> - struct stream *strm;
> - struct server *rsrv;
> + struct pendconn *p = NULL;
> + struct server   *rsrv;
>  
>   rsrv = srv->track;
>   if (!rsrv)
>   rsrv = srv;
>  
> - ps = pendconn_from_srv(srv);
> - pp = pendconn_from_px(px);
> - /* we want to get the definitive pendconn in  */
> - if (!pp || !srv_currently_usable(rsrv)) {
> - if (!ps)
> - return NULL;
> - } else {
> - /* pendconn exists in the proxy queue */
> - if (!ps || tv_islt(>strm->logs.tv_request, 
> >strm->logs.tv_request))
> - ps = pp;
> + if (srv->nbpend) {
> + list_for_each_entry(p, >pendconns, list) {
> + if (!HA_SPIN_TRYLOCK(PENDCONN_LOCK, >lock))
> + goto ps_found;
> + }
> + p = NULL;
> + }
> +
> +  ps_found:
> + if (srv_currently_usable(rsrv) && px->nbpend) {
> + struct pendconn *pp;
> +
> + list_for_each_entry(pp, >pendconns, list) {
> + /* If the server pendconn is older than the proxy one,
> +  * we process the server one. */
> + if (p && !tv_islt(>strm->logs.tv_request, 
> >strm->logs.tv_request))
> + goto pendconn_found;

Here there is still a race : pp->strm is dereferenced out of the lock,
so the owner could very well decide to disappear in the mean time. I
think this check needs to be moved in the trylock below. Or another
option (probably simpler and cleaner) would be to add a tv_request
field to the pendconn, and directly use it.

> +
> + if (!HA_SPIN_TRYLOCK(PENDCONN_LOCK, >lock)) {
> + /* Let's switch from the server pendconn to the
> +  * proxy pendconn. Don't forget to unlock the
> +  * server pendconn, if any. */
> + if (p)
> + HA_SPIN_UNLOCK(PENDCONN_LOCK, >lock);
> + p = pp;
> + goto pendconn_found;
> + }
> + }
>   }
> - strm = ps->strm;
> - __pendconn_free(ps);
>  
> - /* we want to note that the stream has now been assigned a server */
> - strm->flags |= SF_ASSIGNED;
> - strm->target = >obj_type;
> - __stream_add_srv_conn(strm, srv);
> + if (!p)
> + return NULL;
> +
> +  pendconn_found:
> + pendconn_unlinked(p);
> + p->strm_flags  = (p->strm_flags & (SF_DIRECT | SF_ADDR_SET));
> + p->strm_flags |= SF_ASSIGNED;

I think that it would be simpler to consider that strm_flags start with
the stream's initial flags, then only manipulate the ones we're interested
in, and only apply the final mask at the end. This way the enumeration of
all manipulated flags doesn't need to be known at each location. This means
you should only keep SF_ASSIGNED above and drop the line filterinng on the
two other flags. See below for the other instances.

> @@ -206,26 +204,28 @@ struct pendconn *pendconn_add(struct stream *strm)
>   */
>  int pendconn_redistribute(struct server *s)
>  {
> - struct pendconn *pc, *pc_bck;
> + struct pendconn *p, *pback;
>   int xferred = 0;
>  
> + /* The REDISP option was specified. We will ignore cookie and force to
> +  * balance or use the dispatcher. */
> + if ((s->proxy->options & (PR_O_REDISP|PR_O_PERSIST)) != PR_O_REDISP)
> + return