Skip to content

Conversation

@masaori335
Copy link
Contributor

Fix below TSan report.

WARNING: ThreadSanitizer: data race (pid=74346)
  Write of size 8 at 0x0000041330f8 by main thread (mutexes: write M0, write M1, write M2, write M3, write M4, write M5, write M6, write M7, write M8, write M9, write M10, write M11, write M12, write M13):
    #0 Thread::get_hrtime_updated() I_Thread.h:194 (traffic_server:x86_64+0x80513a) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #1 PriorityEventQueue::PriorityEventQueue() PQ-List.cc:28 (traffic_server:x86_64+0x849536) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #2 PriorityEventQueue::PriorityEventQueue() PQ-List.cc:27 (traffic_server:x86_64+0x8495e5) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #3 EThread::EThread(ThreadType, Event*) UnixEThread.cc:107 (traffic_server:x86_64+0x84d7cb) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #4 EThread::EThread(ThreadType, Event*) UnixEThread.cc:108 (traffic_server:x86_64+0x84da71) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #5 EventProcessor::spawn_thread(Continuation*, char const*, unsigned long) UnixEventProcessor.cc:534 (traffic_server:x86_64+0x856566) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #6 NetAccept::init_accept_loop() UnixNetAccept.cc:168 (traffic_server:x86_64+0x7dc6bc) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #7 UnixNetProcessor::accept_internal(Continuation*, int, NetProcessor::AcceptOptions const&) UnixNetProcessor.cc:146 (traffic_server:x86_64+0x7e61e6) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #8 NetProcessor::main_accept(Continuation*, int, NetProcessor::AcceptOptions const&) UnixNetProcessor.cc:86 (traffic_server:x86_64+0x7e50ee) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #9 start_HttpProxyServer() HttpProxyServerMain.cc:373 (traffic_server:x86_64+0x174fda) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #10 main traffic_server.cc:2185 (traffic_server:x86_64+0xbc1b8) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)

  Previous write of size 8 at 0x0000041330f8 by thread T12 (mutexes: write M14):
    #0 Thread::get_hrtime_updated() I_Thread.h:194 (traffic_server:x86_64+0x80513a) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #1 AIOThreadInfo::aio_thread_main(AIOThreadInfo*) AIO.cc:529 (traffic_server:x86_64+0x6292c2) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #2 AIOThreadInfo::start(int, Event*) AIO.cc:244 (traffic_server:x86_64+0x62b485) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #3 Continuation::handleEvent(int, void*) I_Continuation.h:227 (traffic_server:x86_64+0x187a0) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #4 EThread::execute() UnixEThread.cc:333 (traffic_server:x86_64+0x84f94e) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)
    #5 spawn_thread_internal(void*) Thread.cc:79 (traffic_server:x86_64+0x84c611) (BuildId: c245d2fe2dd53a87af825d77f24750ba32000000200000000100000000000c00)

  Location is global 'Thread::cur_time' at 0x0000041330f8 (traffic_server+0xad30f8)

@masaori335 masaori335 added the TSan label Nov 1, 2022
@masaori335 masaori335 added this to the 10.0.0 milestone Nov 1, 2022
@masaori335 masaori335 self-assigned this Nov 1, 2022
@masaori335 masaori335 force-pushed the tsan-0 branch 2 times, most recently from 19640f1 to eed8bae Compare November 2, 2022 02:49
Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

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

Please fix the tests

Thread();

static ink_hrtime cur_time;
static std::atomic<ink_hrtime> cur_time;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thread::cur_time is shared across threads.

@note The cached copy shared among threads which means the cached copy is updated
for all threads if any thread updates it.

@bneradt
Copy link
Contributor

bneradt commented Nov 8, 2022

@masaori335 agreed to do some performance testing of this patch compared to using chrono::now.

@masaori335
Copy link
Contributor Author

  1. ink_get_hrtime_internal vs std::chrono::system_clock::now

My very basic benchmark says ink_get_hrtime_internal (20ns) is slightly faster than std::chrono::system_clock::now (24ns).
https://gist.github.com/masaori335/82472e4ba471591edbc5e760db34631f

Anyway, both of them need around 20ns. It still makes sense to have a cache for frequent access.

  1. std::atomic vs thread local

Performance perspective, thread local seems better (#9184) because it shares nothing.
A concern is it requires every thread to update the cur_time periodically by itself, but some threads assume it's updated by another thread with current code - e.g. an issue was found on LOG_FLUSH thread. ( #9184 fixes this too )

@github-actions
Copy link

github-actions bot commented Feb 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.

@github-actions github-actions bot added the Stale label Feb 7, 2023
@masaori335 masaori335 closed this Feb 14, 2023
@zwoop zwoop removed this from the 10.0.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants