Fix pollsession turn queue starving session addition and removal - #637
Open
Roytak wants to merge 1 commit into
Open
Fix pollsession turn queue starving session addition and removal#637Roytak wants to merge 1 commit into
Roytak wants to merge 1 commit into
Conversation
michalvasko
requested changes
Sep 4, 2026
| set(READ_INACTIVE_TIMEOUT 20 CACHE STRING "Maximum number of seconds waiting for new data once some data have arrived") | ||
| set(READ_ACTIVE_TIMEOUT 300 CACHE STRING "Maximum number of seconds for receiving a full message") | ||
| set(MAX_PSPOLL_THREAD_COUNT 6 CACHE STRING "Maximum number of threads that could simultaneously access a ps_poll structure") | ||
| set(PSPOLL_THREAD_COUNT 6 CACHE STRING "Expected number of threads that simultaneously access a ps_poll structure, its queue grows past this value if needed") |
Contributor
Author
There was a problem hiding this comment.
Removed from both CMakeLists.txt, README.md and libnetconf2.pc.in. Will need to update netopeer2's cmake accordingly.
| if (!ps->queue_size) { | ||
| /* start with the configured size */ | ||
| new_size = (NC_PS_QUEUE_SIZE > 1) ? NC_PS_QUEUE_SIZE : 2; | ||
| } else if (ps->queue_size > UINT8_MAX / 2) { |
Contributor
Author
There was a problem hiding this comment.
Added an extra branch to cap at UINT8_MAX.
| nc_ps_queue_add(ps, 0); | ||
|
|
||
| /* broadcast to all other threads that the queue moved */ | ||
| pthread_cond_broadcast(&ps->cond); |
Member
There was a problem hiding this comment.
Definitely more efficient to pthread_cond_signal() in this case.
Contributor
Author
There was a problem hiding this comment.
Changed to pthread_cond_signal at the cost of each thread needing it's own cond.
Roytak
force-pushed
the
pspoll-prio-queue
branch
from
September 4, 2026 16:09
9b9cf25 to
7defa06
Compare
The pollsession turn queue was strictly FIFO, so nc_ps_add_session() and the other bounded operations queued up behind every poll thread and each of them kept its turn for the whole timeout it was given. A newly established session was therefore not polled for up to thread_count * poll_timeout msec. Threads now queue up with a priority. Every operation except nc_ps_poll() only walks the session array, so it is queued up in front of all the poll threads, and the polling thread notices it is no longer at the head of the queue and hands the turn over. The turn holder is tracked separately from the queue head, as the head is now the thread that gets the turn next. The queue is also allocated and grows on demand instead of failing once MAX_PSPOLL_THREAD_COUNT threads use the same pollsession, which silently dropped the session being added. Waiting longer than NC_PS_QUEUE_TIMEOUT is only an error for the high priority threads now, a poll thread may legitimately wait behind several other poll threads for longer than that. Also fixes the session idle timeout being compared against the poll deadline instead of the current time, which terminated active sessions when the poll timeout was longer than the remaining idle time, and read an uninitialized timespec with an infinite timeout. Fixes #561
Roytak
force-pushed
the
pspoll-prio-queue
branch
from
September 4, 2026 16:45
7defa06 to
ba8528b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The turn queue was strictly FIFO, so
nc_ps_add_session()and the otherbounded operations queued up behind every poll thread, each holding its turn
for the whole timeout it was given so a new session went unpolled for up to
thread_count * poll_timeoutmsec.Threads now queue up with a priority: everything except
nc_ps_poll()onlywalks the session array, so it is queued in front of the poll threads, and the
polling thread hands the turn over once it is no longer first. Queue entries
are
pthread_t, so there are no generated IDs. The queue also grows on demandinstead of failing once
PSPOLL_THREAD_COUNTthreads share a pollsession,which silently dropped the session being added.
Also fixes the session idle timeout being compared against the poll deadline
instead of the current time. New
test_ps_pollcovers the above.Fixes #561