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.

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

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

ton31337 · 2024-10-03T11:03:16Z

lib/agentx.c

@@ -332,7 +332,6 @@ static void smux_trap_multi_index_thd(struct event *thread)
 			return;
 		}
 		ringbuf_get(ibuf_ax, &notification_vars, sizeof(notification_vars));
-		zlog_err("%s get %p %p", __func__, notification_vars, *notification_vars);


Could you amend this fix into f6efd02?

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.

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

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

lib: agentx change agentx_event_update prototype

81f7790

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

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

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 from 1318f79 to 6519edd Compare October 8, 2024 16:58

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

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 Oct 3, 2024

fdumontet6WIND Oct 3, 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

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

vjardin Oct 2, 2024

Choose a reason for hiding this comment

fdumontet6WIND Oct 2, 2024

Choose a reason for hiding this comment

ton31337 Oct 3, 2024

Choose a reason for hiding this comment

fdumontet6WIND Oct 3, 2024

Choose a reason for hiding this comment

ton31337 commented Oct 3, 2024

mjstapp left a comment

Choose a reason for hiding this comment

donaldsharp Oct 3, 2024

Choose a reason for hiding this comment

fdumontet6WIND Oct 3, 2024

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

vjardin Oct 5, 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