Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scalable FIFO support to LogThreadedSource based sources #3969

Merged
merged 22 commits into from Jan 6, 2023

Conversation

bazsi
Copy link
Collaborator

@bazsi bazsi commented Mar 30, 2022

This is a spin-off from #3966 which merges the thread ID space for worker threads and associates a thread_id even with
threads spawned by LogThreadedSourceDriver. This will enable the scalable input processing in LogQueueFifo and makes a few things simpler.

It also increases the number of supported threads to 256 which is becoming a simple compile-time constant, instead of a constraint imposed by the underlying data structure (the previous limit of 64 being the number of bits in a guint64).

While #3966 still depends on an ivykis change (buytenh/ivykis#24) but I don't want to stall the commits that make sense even without the ivykis dependency.

@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from 240c477 to 832d2ba Compare March 30, 2022 09:26
@kira-syslogng
Copy link
Contributor

Build FAILURE

1 similar comment
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi changed the title Add scalable FIFO support to LogThreadedSource based sources WIP: Add scalable FIFO support to LogThreadedSource based sources Mar 30, 2022
@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from 24b2dce to 2b9d7c8 Compare March 30, 2022 10:54
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch 2 times, most recently from d165e03 to c898e56 Compare March 31, 2022 06:20
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@bazsi bazsi changed the title WIP: Add scalable FIFO support to LogThreadedSource based sources Add scalable FIFO support to LogThreadedSource based sources Mar 31, 2022
@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from c898e56 to 491709b Compare April 4, 2022 09:00
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch 2 times, most recently from 81bba33 to a7bebaf Compare May 25, 2022 06:41
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from a7bebaf to d2619a7 Compare June 2, 2022 13:46
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from d2619a7 to 6b2f0fc Compare July 3, 2022 14:49
@bazsi
Copy link
Collaborator Author

bazsi commented Jul 3, 2022

Rebased against current master.

@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from 6b2f0fc to 3a2466d Compare September 9, 2022 08:38
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi
Copy link
Collaborator Author

bazsi commented Sep 13, 2022

@kira-syslogng retest this please;

@MrAnno MrAnno self-requested a review September 15, 2022 12:45
@mitzkia
Copy link
Collaborator

mitzkia commented Sep 16, 2022

@kira-syslogng retest this please;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 16, 2022

@kira-syslogng retest this please;

Copy link
Collaborator

@MrAnno MrAnno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've marked my important note with [change-request].

lib/mainloop-worker.c Outdated Show resolved Hide resolved
@@ -673,7 +678,8 @@ log_queue_fifo_new(gint log_fifo_size, const gchar *persist_name)

self->super.free_fn = log_queue_fifo_free;

for (i = 0; i < log_queue_max_threads; i++)
self->num_input_queues = log_queue_max_threads;
for (i = 0; i < num_input_queues; i++)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is undeclared.

lib/logqueue-fifo.c Outdated Show resolved Hide resolved
lib/logqueue-fifo.c Show resolved Hide resolved
@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from 3a2466d to ad4596a Compare January 2, 2023 17:41
@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from ad4596a to fc585b7 Compare January 5, 2023 08:22
@bazsi
Copy link
Collaborator Author

bazsi commented Jan 5, 2023

rebased and squashed.

@kira-syslogng
Copy link
Contributor

Build FAILURE

@MrAnno
Copy link
Collaborator

MrAnno commented Jan 5, 2023

There is still a fixup commit in the history.

It seems the commit check is broken, it shows success incorrectly. I'm fixing it: #4270

@bazsi
Copy link
Collaborator Author

bazsi commented Jan 5, 2023 via email

This patch removes the 64 thread limitation from _allocate_thread_id and
increases the current maximum to 256.

Allocations that depend on per-thread state currently only use the
actual number of threads allowed by the --worker-threads command
line option (which defaults to the number of cores in the system),
so increasing this limit does not cause an immediate increase in
memory usage.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
This naming matches the existing naming convention of LogPipe method names,
also I intend to introduce a pre_config_init() hook as well.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…reads

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
… each message

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Instead of relying on a single global log_queue_max_threads variable,
save it at constructor time.

Also, handle the case where we get a thread_id that is larger than this
saved value, instead of failing an assert.

This patch is a preparatory step to allow threaded sources to use the
scalable input path for a LogQueue, because if we want to support that
we need to make the thread_id space configuration dependent.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
… max_threads value

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Instead of a layering violation (cfg calling into mainloop-worker)
and to allow registering thread IDs from non-LogPipe instances
(e.g. the mainloop-io-worker layer).

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
The "thread_id" value had dual uses:

  1) at one point it was 1 based, so that 0 meant uninitialized state
  2) in other cases it was 0 based, so we could use it as an array index

This patch makes the two uses distinct. The "id" term is only referred
to the 1 based value, 0 meaning uninitialized. In all other cases we
are referring to "thread_index" to better reflect that this is a 0
based value. In the latter case, -1 means uninitialized.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
@bazsi bazsi force-pushed the logthrsource-scalable-fifo-support branch from fc585b7 to ddf3758 Compare January 5, 2023 19:45
@kira-syslogng
Copy link
Contributor

Build FAILURE

@MrAnno
Copy link
Collaborator

MrAnno commented Jan 6, 2023

The macOS runner seems to have been updated and now it contains a broken net-snmp pc file. I'm working on a fix.

@MrAnno MrAnno merged commit 1e97181 into syslog-ng:master Jan 6, 2023
@bazsi bazsi deleted the logthrsource-scalable-fifo-support branch May 3, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants