Agentx thread #16986

fdumontet6WIND · 2024-10-02T19:32:35Z

Introduce a dedicated AgentX thread using frr_pthread.
Add mutex locks (ax_mtx, ax_io_mtx) to manage thread
synchronization for trap transfer and I/O operations.
Implemented ring buffers (ibuf_ax) for handling
"master -> AgentX" communication, improving data handling
between threads.
Update the SNMP read operations to use mutex locks to
ensure thread-safe execution.
Integrated a new dedicated thread to send SNMP traps,
ensuring separation of responsibilities between the main
and AgentX threads.
Enhanced trap handling to support multi-index traps, with
excess traps being discarded if the buffer is full,
preventing overflow.
Enhanced trap handling to support multi-index traps.
When more than "RINGBUF_NB_TRAP" traps are pending for
transmission, subsequent traps are discarded to prevent
overflow.

This update significantly improves concurrency,
synchronization, and trap management within the AgentX
module, with added protection against socket's buffer overflow
from excessive traps. The socket's buffer overflow is leading
to process deadlock.

vjardin

Please, avoid pulling some dependencies specific for bgpd.

lib/agentx.c

vjardin · 2024-10-02T19:45:28Z

lib/agentx.c

@@ -23,25 +23,62 @@
 #include "libfrr.h"
 #include "xref.h"
 #include "lib/libagentx.h"
+#include "ringbuf.h"	 /* for ringbuf_remain, ringbuf_peek, ringbuf_.. */
+#include "frr_pthread.h" /* for struct frr_pthread */
+#include "bgpd/bgpd.h"	 /* for bgp_pth_ax */


Why a a specific dependency with bgpd ?

taken into account

lib/agentx.c

ton31337 · 2024-10-03T11:06:48Z

Also, could you fix Verify Source, and frrbot issues?

mjstapp

I'm having a hard time following what problem is being solved here.

It sounds like there can be a problem if there are many traps to be sent - that the existing code is not detecting that, is not discarding events when it begins to overrun a receiver, and/or is not handling EWOULDBLOCK. Is that correct? wouldn't it be simpler to make the code detect and handle those conditions directly -rather than injecting a new pthread and then teaching it to handle the error conditions?

Is there any other problem present? I'm a bit terrified by this line from the description:

"Update the SNMP read operations to use mutex locks to ensure thread-safe execution."

so ... it is not possible to make reads of daemon data "thread-safe" - right? only the main pthread can access daemon internals. what change are you proposing to make there?

donaldsharp · 2024-10-03T13:38:20Z

lib/agentx.c

 DEFINE_HOOK(agentx_enabled, (), ());

 //bool agentx_enabled = false;

 static struct event_loop *agentx_tm;
+static struct event_loop *main_tm;


can we name this something better? and at least put some documentation into why agentx code has too event_loop data structure pointers. Let's help our future selves out here

taken ento account

donaldsharp · 2024-10-03T14:29:15Z

lib/agentx.c:438: error: log message contains tab or newline: "%s not enougth space in ibuf_ax needed : %lu free :\t %lu" (in smux_trap_multi_index())

needs to be fixed

fdumontet6WIND · 2024-10-03T14:34:38Z

tomjstapp
The problem is that, in certain high-load situations, AgentX blocks the entire bgpd process.

We can indeed address some cases by implementing flow control on the traps. Unfortunately, other cases related to the internal mechanisms of AgentX SNMP emerge. It's to provide a global solution, rather than a series of incomplete fixes, that we propose handling the sending of traps in a specific pthread.

mjstapp · 2024-10-03T15:43:57Z

So I'd like to understand that situation better. If the new pthread is wedged somewhere, and a daemon's main pthread is shutting down and waiting for it to stop, how will that work?

tomjstapp The problem is that, in certain high-load situations, AgentX blocks the entire bgpd process.

We can indeed address some cases by implementing flow control on the traps. Unfortunately, other cases related to the internal mechanisms of AgentX SNMP emerge. It's to provide a global solution, rather than a series of incomplete fixes, that we propose handling the sending of traps in a specific pthread.

fdumontet6WIND · 2024-10-03T20:41:41Z

ci:rerun

vjardin · 2024-10-05T17:27:49Z

lib/agentx.c

+/* mutex dedicated to trap transfert between threads */
+pthread_mutex_t ax_mtx;
+/* mutex dedicated to send/read exclusion */
+pthread_mutex_t ax_io_mtx;


Shouldn't they be static ? both ax_mtx and ax_io_mtx.

vjardin

LGTM, just a minor update (static) that would be welcomed.

vjardin · 2024-10-07T12:50:46Z

Thanks for the static that are being added. ;)

fdumontet6WIND · 2024-10-07T16:45:29Z

sorry for not having notified the change.

vjardin · 2024-10-08T17:49:36Z

Please @fdumontet6WIND , do you have some data on the improvements you are getting ?

LGTM

fdumontet6WIND · 2024-10-08T19:29:01Z

added some comments following Donald sharp advice

vjardin · 2024-10-10T15:12:29Z

why closed ?

fdumontet6WIND · 2024-10-10T15:42:49Z

need some improvments

fdumontet6WIND · 2024-10-12T18:11:46Z

ci:rerun

fdumontet6WIND · 2024-10-14T17:04:46Z

ci:rerun

ton31337 · 2024-10-16T07:48:31Z

lib/agentx.c

@@ -83,7 +88,10 @@ static void agentx_timeout(struct event *t)
 		snmp_timeout();
 		run_alarms();
 	} else {
-		zlog_err("%s mutex already locked", __func__);
+		if (nb_locktry_timeout_fail % 100 == 0)


Could you bundle this fixup with e464755? Also, what should an operator do if I see such an error?

taken into account.

if an operator see a great number of such an error, restarting processes using agentx is good option

fdumontet6WIND · 2024-10-16T11:24:23Z

ci:rerun

eqvinox · 2024-10-16T11:37:27Z

tomjstapp The problem is that, in certain high-load situations, AgentX blocks the entire bgpd process.

We can indeed address some cases by implementing flow control on the traps. Unfortunately, other cases related to the internal mechanisms of AgentX SNMP emerge. It's to provide a global solution, rather than a series of incomplete fixes, that we propose handling the sending of traps in a specific pthread.

AgentX is blocking the process in write/send for traps, if I understand correctly?

If that's the problem, I'd rather have a buffer/queue without threads. Use event_add_write and make a call when the fd is ready to be written… in the same thread.

fdumontet6WIND · 2024-10-16T13:04:17Z

Hi David,
fd may be ready to be written, but has not enough buffer free memory for sending all what we want.
the send will be blocked.
On another hand there is many reason for blocking , for instance when a timeout occurs.

add "struct event *" parameter to agentx_event_update function. the function may be called via event_add*** functions. Signed-off-by: Francois Dumontet <[email protected]>

- Introduce a dedicated AgentX thread using `frr_pthread`. - Add mutex locks (`ax_mtx`, `ax_io_mtx`) to manage thread synchronization for trap transfer and I/O operations. - Implemented ring buffers (`ibuf_ax`) for handling "master -> AgentX" communication, improving data handling between threads. - Update the SNMP read operations to use mutex locks to ensure thread-safe execution. - Integrated a new dedicated thread to send SNMP traps, ensuring separation of responsibilities between the main and AgentX threads. - Enhanced trap handling to support multi-index traps, with excess traps being discarded if the buffer is full, preventing overflow. - Enhanced trap handling to support multi-index traps. When more than "RINGBUF_NB_TRAP" traps are pending for transmission, subsequent traps are discarded to prevent overflow. This update significantly improves concurrency, synchronization, and trap management within the AgentX module, with added protection against socket's buffer overflow from excessive traps. The socket's buffer overflow is leading to process deadlock. Signed-off-by: Francois Dumontet <[email protected]>

fdumontet6WIND · 2024-10-25T20:22:23Z

ci:rerun

fdumontet6WIND · 2024-10-26T16:48:05Z

ci:rerun

frrbot bot added the libfrr label Oct 2, 2024

github-actions bot added master size/L labels Oct 2, 2024

vjardin requested changes Oct 2, 2024

View reviewed changes

fdumontet6WIND force-pushed the agentx_thread branch 2 times, most recently from fddcf30 to f6efd02 Compare October 2, 2024 19:53

ton31337 reviewed Oct 3, 2024

View reviewed changes

lib/agentx.c Outdated Show resolved Hide resolved

fdumontet6WIND force-pushed the agentx_thread branch 2 times, most recently from 7557c63 to 8535a1b Compare October 3, 2024 12:45

mjstapp reviewed Oct 3, 2024

View reviewed changes

donaldsharp reviewed Oct 3, 2024

View reviewed changes

fdumontet6WIND force-pushed the agentx_thread branch from 8535a1b to 43660e4 Compare October 3, 2024 13:42

fdumontet6WIND force-pushed the agentx_thread branch 2 times, most recently from 97b75e2 to f03720f Compare October 3, 2024 15:06

fdumontet6WIND force-pushed the agentx_thread branch from f03720f to 57976b4 Compare October 4, 2024 12:17

fdumontet6WIND requested a review from vjardin October 5, 2024 08:20

vjardin reviewed Oct 5, 2024

View reviewed changes

vjardin approved these changes Oct 5, 2024

View reviewed changes

fdumontet6WIND force-pushed the agentx_thread branch from 57976b4 to b1f47b3 Compare October 7, 2024 10:15

github-actions bot added the rebase PR needs rebase label Oct 7, 2024

fdumontet6WIND force-pushed the agentx_thread branch from b1f47b3 to 1318f79 Compare October 7, 2024 12:23

fdumontet6WIND force-pushed the agentx_thread branch 2 times, most recently from 6519edd to 48685e3 Compare October 8, 2024 17:13

fdumontet6WIND closed this Oct 10, 2024

fdumontet6WIND reopened this Oct 11, 2024

fdumontet6WIND force-pushed the agentx_thread branch 2 times, most recently from 7f94aa3 to 256fc73 Compare October 11, 2024 15:15

fdumontet6WIND force-pushed the agentx_thread branch 3 times, most recently from 845a60d to 1780159 Compare October 14, 2024 14:26

fdumontet6WIND requested a review from vjardin October 15, 2024 08:58

fdumontet6WIND force-pushed the agentx_thread branch from 1780159 to b95cb12 Compare October 15, 2024 09:01

fdumontet6WIND requested a review from ton31337 October 15, 2024 15:20

ton31337 reviewed Oct 16, 2024

View reviewed changes

fdumontet6WIND force-pushed the agentx_thread branch from b95cb12 to 6124dae Compare October 16, 2024 08:23

fdumontet6WIND added 2 commits October 25, 2024 18:30

lib: agentx change agentx_event_update prototype

1f4f413

add "struct event *" parameter to agentx_event_update function. the function may be called via event_add*** functions. Signed-off-by: Francois Dumontet <[email protected]>

fdumontet6WIND force-pushed the agentx_thread branch from da24281 to c7f12bf Compare October 25, 2024 16:33

fdumontet6WIND marked this pull request as draft November 5, 2024 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agentx thread #16986

Agentx thread #16986

fdumontet6WIND commented Oct 2, 2024

vjardin left a comment

vjardin Oct 2, 2024

fdumontet6WIND Oct 2, 2024

ton31337 commented Oct 3, 2024

mjstapp left a comment

donaldsharp Oct 3, 2024

fdumontet6WIND Oct 3, 2024

donaldsharp commented Oct 3, 2024

fdumontet6WIND commented Oct 3, 2024

mjstapp commented Oct 3, 2024

fdumontet6WIND commented Oct 3, 2024

vjardin Oct 5, 2024

vjardin left a comment

vjardin commented Oct 7, 2024

fdumontet6WIND commented Oct 7, 2024

vjardin commented Oct 8, 2024

fdumontet6WIND commented Oct 8, 2024

vjardin commented Oct 10, 2024

fdumontet6WIND commented Oct 10, 2024

fdumontet6WIND commented Oct 12, 2024

fdumontet6WIND commented Oct 14, 2024

ton31337 Oct 16, 2024

fdumontet6WIND Oct 16, 2024

fdumontet6WIND Oct 16, 2024

fdumontet6WIND commented Oct 16, 2024

eqvinox commented Oct 16, 2024

fdumontet6WIND commented Oct 16, 2024

fdumontet6WIND commented Oct 25, 2024

fdumontet6WIND commented Oct 26, 2024

Agentx thread #16986

Are you sure you want to change the base?

Agentx thread #16986

Conversation

fdumontet6WIND commented Oct 2, 2024

vjardin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ton31337 commented Oct 3, 2024

mjstapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldsharp commented Oct 3, 2024

fdumontet6WIND commented Oct 3, 2024

mjstapp commented Oct 3, 2024

fdumontet6WIND commented Oct 3, 2024

Choose a reason for hiding this comment

vjardin left a comment

Choose a reason for hiding this comment

vjardin commented Oct 7, 2024

fdumontet6WIND commented Oct 7, 2024

vjardin commented Oct 8, 2024

fdumontet6WIND commented Oct 8, 2024

vjardin commented Oct 10, 2024

fdumontet6WIND commented Oct 10, 2024

fdumontet6WIND commented Oct 12, 2024

fdumontet6WIND commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdumontet6WIND commented Oct 16, 2024

eqvinox commented Oct 16, 2024

fdumontet6WIND commented Oct 16, 2024

fdumontet6WIND commented Oct 25, 2024

fdumontet6WIND commented Oct 26, 2024