Misc fixes 20250103 #12335
Closed
Misc fixes 20250103 #12335
Conversation
This file contains 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
A locking was needed to be able to access the thread from its id when the function is in fact executed in the context of the thread itself. By moving timestamp to the ThreadVars, we can avoid the lock. Effect on locking contention is immediate: First run is before the change and second one after the move: Lock Cnt Avg ticks Max ticks Total ticks Cont Func ------------------ ---------- --------- ------------ ------------ ------- --------- (mtx) tm-threads.c:2190 1049627 1193 17819166 1253023306 21777 TmThreadsSetThreadTimestamp (mtx) flow-hash.c:875 48945 527 582920 25825014 0 FlowGetFlowFromHash (mtx) flow-spare-pool.c:175 8 540 1056 4320 0 FlowSpareGetFromPool (mtx) flow-hash.c:752 729 977 4816 712754 0 FlowGetNew (mtx) util-pool-thread.c:181 760 1401 21486 1065246 0 PoolThreadGetById (mtx) flow-hash.c:922 48216 401 435596 19352498 0 FlowGetFlowFromHash (mtx) util-logopenfile.c:187 1252 8774 1969800 10986230 36 OutputWriteLock Overall: locks 1890455, average cost 992, contentions 29103, total ticks 1875539286 Lock Cnt Avg ticks Max ticks Total ticks Cont Func ------------------ ---------- --------- ------------ ------------ ------- --------- (mtx) flow-hash.c:875 48945 379 38140 18553560 0 FlowGetFlowFromHash (mtx) flow-spare-pool.c:175 8 531 726 4252 0 FlowSpareGetFromPool (mtx) flow-hash.c:752 729 890 8696 648918 0 FlowGetNew (mtx) flow-hash.c:922 48216 325 210022 15696986 0 FlowGetFlowFromHash (mtx) util-logopenfile.c:187 1252 2462 634282 3083086 28 OutputWriteLock (mtx) util-pool-thread.c:181 731 1040 6692 760402 0 PoolThreadGetById (mtx) util-debug.c:692 1 574 574 574 0 SCLogMessage Overall: locks 718515, average cost 433, contentions 1956, total ticks 311154230 TimeSetByThread is called only once in all code and it is from FlowWorker. TimeSetByThread is in fact calling TmThreadsSetThreadTimestamp which is only called from this function. So it seems we have no cross thread usage of the function and switching to a non locking approach will improve the situation. Ticket: 7474
When removal is going to empty the bucket list, this does not mean we don't need to compare the data. Note: Nothing like fixing a 16 years old bug. Ticket: 7475
regit
force-pushed
the
misc-fixes-20250103
branch
from
b301bd9 to
c546dc2
Compare
|
Information: QA ran without warnings. Pipeline 24063 |
|
Information: QA ran without warnings. Pipeline 24064 |
|
This PR breaks thread safety of A different refactor will soon be merged, here #12313 |
OK, I'm going to close and reopen for reamining issue (and add another small one on top). |
|
Can you do a PR for the hash table stuff? In general, I prefer logically separated PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Some small fixes.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to tickets:
Describe changes: