You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/11/01 00:15:03 UTC
[GitHub] [trafficserver] masaori335 opened a new pull request, #9168: TSan: Fix data race of updating current time
masaori335 opened a new pull request, #9168:
URL: https://github.com/apache/trafficserver/pull/9168
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)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #9168: TSan: Fix data race of updating current time
Posted by GitBox <gi...@apache.org>.
masaori335 commented on PR #9168:
URL: https://github.com/apache/trafficserver/pull/9168#issuecomment-1297997251
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9168: TSan: Fix data race of updating current time
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9168:
URL: https://github.com/apache/trafficserver/pull/9168#issuecomment-1306410038
@masaori335 agreed to do some performance testing of this patch compared to using `chrono::now`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] github-actions[bot] commented on pull request #9168: TSan: Fix data race of updating current time
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9168:
URL: https://github.com/apache/trafficserver/pull/9168#issuecomment-1420058704
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #9168: TSan: Fix data race of updating current time
Posted by GitBox <gi...@apache.org>.
masaori335 commented on PR #9168:
URL: https://github.com/apache/trafficserver/pull/9168#issuecomment-1308001127
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.
2. 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 )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 closed pull request #9168: TSan: Fix data race of updating current time
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 closed pull request #9168: TSan: Fix data race of updating current time
URL: https://github.com/apache/trafficserver/pull/9168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a diff in pull request #9168: TSan: Fix data race of updating current time
Posted by GitBox <gi...@apache.org>.
masaori335 commented on code in PR #9168:
URL: https://github.com/apache/trafficserver/pull/9168#discussion_r1016012677
##########
iocore/eventsystem/I_Thread.h:
##########
@@ -177,7 +177,7 @@ class Thread
protected:
Thread();
- static ink_hrtime cur_time;
+ static std::atomic<ink_hrtime> cur_time;
Review Comment:
`Thread::cur_time` is shared across threads.
https://github.com/apache/trafficserver/blob/a5f647a2023d481600141fb124f93642c34e01c6/iocore/eventsystem/I_Thread.h#L159-L160
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org