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 <srv> and
* <srv_conn> are set to <srv>,
*/
-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 <ps> */
- if (!pp || !srv_currently_usable(rsrv)) {
- if (!ps)
- return NULL;
- } else {
- /* pendconn exists in the proxy queue */
- if (!ps || tv_islt(&pp->strm->logs.tv_request,
&ps->strm->logs.tv_request))
- ps = pp;
+ if (srv->nbpend) {
+ list_for_each_entry(p, &srv->pendconns, list) {
+ if (!HA_SPIN_TRYLOCK(PENDCONN_LOCK, &p->lock))
+ goto ps_found;
+ }
+ p = NULL;
+ }
+
+ ps_found:
+ if (srv_currently_usable(rsrv) && px->nbpend) {
+ struct pendconn *pp;
+
+ list_for_each_entry(pp, &px->pendconns, list) {
+ /* If the server pendconn is older than the proxy one,
+ * we process the server one. */
+ if (p && !tv_islt(&pp->strm->logs.tv_request,
&p->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 <p>, 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 <strm>. It must
+ * always exists here. If the pendconn is still linked to the server or the
+ * proxy queue, nothing is done and the function returns 1. Otherwise,
+ * <strm>->flags and <strm>->target are updated, the pendconn is released and 0
+ * is returned.
+ */
+int pendconn_dequeue(struct stream *strm)
+{
+ struct pendconn *p;
+
+ if (unlikely(!strm->pend_pos)) {
+ /* unexpected case because it is called by the stream itself and
+ * only the stream can release a pendconn. So it is only
+ * possible if a pendconn is released by someone else or if the
+ * stream is supposed to be queued but without its associated
+ * pendconn. In both cases it is a bug! */
+ abort();
+ }
+ p = strm->pend_pos;
+ HA_SPIN_LOCK(PENDCONN_LOCK, &p->lock);
+
+ /* the pendconn is still linked to the server/proxy queue, so unlock it
+ * and go away. */
+ if (!LIST_ISEMPTY(&p->list)) {
+ HA_SPIN_UNLOCK(PENDCONN_LOCK, &p->lock);
+ return 1;
+ }
Just a question, in case it's still linked, when will it be forced to be
dequeued ? I'm unsure about the situation where we can end up here with
an entry still linked in fact, and would like to ensure we never leave
an outdated entry in the queue.
Good question. I'll check. But the stream may be woken up for another
reason. So it is probably possible to try to dequeue it while this was
not explicitly requested. To be honest, I don't know if it is possible.
But I don't know either if it is impossible :)
At least, we are sure that when we want to dequeue a stream, it was
first removed from the queue then woken up. And if it is still in the
queue, it is still pending.
BTW, I'll check.
- p->strm->pend_pos = NULL;
- HA_ATOMIC_SUB(&p->strm->be->totpend, 1);
+ HA_ATOMIC_SUB(&strm->be->totpend, 1);
I'm just seeing that on a few occurrences above, we only need <strm> to
access the backend's lock. Most likely the code can be simplified by
adding the pointer to the backend into the pendconn struct as well,
just like we already have the pointer to the server.
Good idea. I'll do that.
+static void pendconn_unlinked(struct pendconn *p)
Just a minor matter of taste, for me the use of a past participle in the
name "pendconn_unlinked" makes me think it returns a boolean indicating
whether or not it's unlinked. Since it's an action in fact, it's probably
better to call it "pendconn_unlink".
You're right, that's a mistake. I'll fix that.
--
Christopher Faulet
