Hi Maciej,
On Wed, Apr 06, 2022 at 11:17:44AM +0200, Maciej Zdeb wrote:
> Hi Willy,
>
> I've managed to implement the easy part (attached patches).
Thanks for this! I'm having a minor comment on the 3rd patch, see at
the end.
> However this is
> not enough to get it to work properly. I'm stuck in a debug of following
> stack trace (BUG_ON task_queue() function):
>
> FATAL: bug condition "task->thread_mask != tid_bit" matched at
> include/haproxy/task.h:320
> call trace(12):
> | 0x55f921169be3 [c7 04 25 01 00 00 00 00]: main+0x112f53
> | 0x55f92116c4ee [48 8b 43 08 48 8b 78 10]: si_applet_wake_cb+0x7e/0xf3
> | 0x55f9211cc276 [48 8b 43 08 48 8b 40 10]: task_run_applet+0x196/0x5bd
> | 0x55f92118e6ca [48 89 c5 eb 0f 90 4c 89]:
> run_tasks_from_lists+0x3aa/0x871
> | 0x55f92118ef73 [29 44 24 14 8b 7c 24 14]:
> process_runnable_tasks+0x3d3/0x892
> | 0x55f92115eb44 [83 3d f9 ef 1d 00 01 0f]: run_poll_loop+0x124/0x3d4
> | 0x55f92115efc9 [48 8b 1d d0 94 12 00 4c]: main+0x108339
> | 0x55f9210589ff [31 c0 e8 ba a6 16 00 31]: main+0x1d6f/0x296a
>
> I'm not sure why it happens. I suspect that it happens because of
> session_new() in peer_session_create() that creates another task on the
> current thread. If a session task tries to wake up a peer applet task then
> it would explain the stack trace, however I've failed to fully understand
> the "flow" between applet and session (and how/when task_queue is called in
> this flow). I would appreciate any help or tips on how to proceed.
I hadn't thought about that, as usual the devil is in the details :-(
The problem here is that it's not allowed to requeue a task that belongs
to another thread, and that's exactly what is being done there, because
while the applet runs on its target thread, it's bound to a stream that
was programmed for another thread, and when requeuing its task it fails.
We could imagine having a stream_new_on() but that's not realistic
because stream_new() already touches some thread-specific stuff and
even calls the frontend's accept() function or initializes streams.
Thus if we sum up, it's not possible to create a stream for another thread,
but functionally speaking, that's not something we absolutely want to do,
we would also be fine with having someone else create such a stream. And
in an ideal world, the applet's init code could be the one responsible
for creating its stream. Historically it wasn't even possible because both
contexts were strongly tied. But with the changes Christopher recently did
to keep the applets away, maybe that's something we can think of. If so,
that would also make the Lua client and http_client setup code so much
cleaner, I'll discuss this with Christopher to see what's missing to
reach that point, and will get back to you ASAP (or please yell at me
if you don't hear about me for some time).
Regarding the comment on the 3rd patch:
> From c7f1be0dab823b81a1806d8eaca906d9c2483ac8 Mon Sep 17 00:00:00 2001
> From: Maciej Zdeb <mac...@zdeb.pl>
> Date: Mon, 4 Apr 2022 13:08:11 +0200
> Subject: [PATCH 3/3] MINOR: peers: Balance applets across threads
>
(...)
> @@ -3236,7 +3240,7 @@ static struct appctx *peer_session_create(struct peers
> *peers, struct peer *peer
>
> s->res.flags |= CF_READ_DONTWAIT;
>
> - peers->applet_count[tid]++;
> + peers->applet_count[thr]++;
You'll need to switch to:
HA_ATOMIC_INC(&peers->applet_count[thr]);
instead. Indeed, since the counter belongs to another thread, any other
thread could be manipulating it at the same time.
thanks!
Willy
