Skip to content

TSAN fix for fake_global_for_ink_queue. #9183

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.

Sign up for GitHub

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

Jump to bottom
Merged
ywkaras merged 1 commit into from
Mar 3, 2023
Merged

TSAN fix for fake_global_for_ink_queue. #9183

ywkaras merged 1 commit into from
Mar 3, 2023

Conversation

@ywkaras
Copy link
Contributor

@ywkaras ywkaras commented Nov 8, 2022

No description provided.

@ywkaras ywkaras self-assigned this Nov 8, 2022
@ywkaras
Copy link
Contributor Author

ywkaras commented Nov 8, 2022 •
edited
Loading

Fixes this warning:

==================
WARNING: ThreadSanitizer: data race (pid=32645)
  Write of size 4 at 0x7f35405bc780 by thread T3:
    #0 freelist_new /home/wkaras/REPOS/TS/src/tscore/ink_queue.cc:251 (libtscore.so.10+0x1938e6)
    #1 ink_freelist_new /home/wkaras/REPOS/TS/src/tscore/ink_queue.cc:187 (libtscore.so.10+0x193341)
    #2 FreelistAllocator::alloc_void() ../../include/tscore/Allocator.h:63 (traffic_server+0x7107ef)
    #3 Event* ClassAllocator<Event, false>::alloc<>() ../../include/tscore/Allocator.h:241 (traffic_server+0x71631a)
    #4 EventProcessor::initThreadState(EThread*) /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEventProcessor.cc:457 (traffic_server+0xcf6ba4)
    #5 EventProcessor::ThreadInit::init(int, Event*) /home/wkaras/REPOS/TS/iocore/eventsystem/I_EventProcessor.h:388 (traffic_server+0xcf7a76)
    #6 Continuation::handleEvent(int, void*) /home/wkaras/REPOS/TS/iocore/eventsystem/I_Continuation.h:227 (traffic_server+0x711d65)
    #7 EThread::execute() /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEThread.cc:333 (traffic_server+0xcf290b)
    #8 spawn_thread_internal /home/wkaras/REPOS/TS/iocore/eventsystem/Thread.cc:79 (traffic_server+0xcf0278)

  Previous write of size 4 at 0x7f35405bc780 by thread T2:
    #0 freelist_new /home/wkaras/REPOS/TS/src/tscore/ink_queue.cc:251 (libtscore.so.10+0x1938e6)
    #1 ink_freelist_new /home/wkaras/REPOS/TS/src/tscore/ink_queue.cc:187 (libtscore.so.10+0x193341)
    #2 FreelistAllocator::alloc_void() ../../include/tscore/Allocator.h:63 (traffic_server+0x7107ef)
    #3 Event* ClassAllocator<Event, false>::alloc<>() ../../include/tscore/Allocator.h:241 (traffic_server+0x71631a)
    #4 EventProcessor::initThreadState(EThread*) /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEventProcessor.cc:457 (traffic_server+0xcf6ba4)
    #5 EventProcessor::ThreadInit::init(int, Event*) /home/wkaras/REPOS/TS/iocore/eventsystem/I_EventProcessor.h:388 (traffic_server+0xcf7a76)
    #6 Continuation::handleEvent(int, void*) /home/wkaras/REPOS/TS/iocore/eventsystem/I_Continuation.h:227 (traffic_server+0x711d65)
    #7 EThread::execute() /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEThread.cc:333 (traffic_server+0xcf290b)
    #8 spawn_thread_internal /home/wkaras/REPOS/TS/iocore/eventsystem/Thread.cc:79 (traffic_server+0xcf0278)

  Location is global 'fake_global_for_ink_queue' of size 4 at 0x7f35405bc780 (libtscore.so.10+0x0000004aa780)

  Thread T3 '[ET_NET 1]' (tid=32656, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x615d8)
    #1 ink_thread_create ../../include/tscore/ink_thread.h:137 (traffic_server+0xcefc7b)
    #2 Thread::start(char const*, void*, unsigned long, std::function<void ()> const&) /home/wkaras/REPOS/TS/iocore/eventsystem/Thread.cc:96 (traffic_server+0xcf03a6)
    #3 EventProcessor::spawn_event_threads(int, int, unsigned long) /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEventProcessor.cc:439 (traffic_server+0xcf68c1)
    #4 EventProcessor::start(int, unsigned long) /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEventProcessor.cc:502 (traffic_server+0xcf6f6c)
    #5 main traffic_server/traffic_server.cc:2025 (traffic_server+0x77f23a)

  Thread T2 '[ET_NET 0]' (tid=32655, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.0+0x615d8)
    #1 ink_thread_create ../../include/tscore/ink_thread.h:137 (traffic_server+0xcefc7b)
    #2 Thread::start(char const*, void*, unsigned long, std::function<void ()> const&) /home/wkaras/REPOS/TS/iocore/eventsystem/Thread.cc:96 (traffic_server+0xcf03a6)
    #3 EventProcessor::spawn_event_threads(int, int, unsigned long) /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEventProcessor.cc:439 (traffic_server+0xcf68c1)
    #4 EventProcessor::start(int, unsigned long) /home/wkaras/REPOS/TS/iocore/eventsystem/UnixEventProcessor.cc:502 (traffic_server+0xcf6f6c)
    #5 main traffic_server/traffic_server.cc:2025 (traffic_server+0x77f23a)

SUMMARY: ThreadSanitizer: data race /home/wkaras/REPOS/TS/src/tscore/ink_queue.cc:251 in freelist_new
==================

@ywkaras
Copy link
Contributor Author

ywkaras commented Nov 8, 2022 •
edited
Loading

Looks like this overlaps with #9168 .

@ywkaras ywkaras requested a review from bryancall November 8, 2022 00:37
@ywkaras ywkaras marked this pull request as draft December 27, 2022 17:55
@zwoop zwoop added this to the 10.0.0 milestone Jan 17, 2023
@ywkaras ywkaras force-pushed the two_tsan branch 3 times, most recently from 9605fd3 to 0e66ca8 Compare February 16, 2023 04:16
@ywkaras ywkaras changed the title TSAN fixes for Thread::cur_time and fake_global_for_ink_queue. TSAN fix for fake_global_for_ink_queue. Feb 16, 2023
@ywkaras ywkaras marked this pull request as ready for review February 16, 2023 05:19
SolidWallOfCode
SolidWallOfCode reviewed Mar 1, 2023
View reviewed changes
src/tscore/ink_queue.cc
inline void
dummy_forced_read(void *mem)
{
static_cast<void>(*const_cast<int volatile *>(reinterpret_cast<int *>(mem)));
Copy link
Member

@SolidWallOfCode SolidWallOfCode Mar 1, 2023

Choose a reason for hiding this comment

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

Use [[maybe_unused]] instead of static_cast<void>()?

Also, you can static_cast to int * because it's a void *.

Finally, force_read might be a better name. There's no longer a dummy variable involved

Copy link
Contributor Author

@ywkaras ywkaras Mar 1, 2023

Choose a reason for hiding this comment

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

I prefer code that's less rather than more obscure. In any case, seems misleading to say something is maybe unused when it's definitely unused.

We should never miss a chance to use a static_cast apparently.

It's dummy in the sense that we are reading a memory location as some sort of fragile, ratchet, half-assed memory fence, rather than to read the value of a memory location. Instead of using the more portable code I wrote to replace this mess.

Copy link
Member

@SolidWallOfCode SolidWallOfCode Mar 2, 2023

Choose a reason for hiding this comment

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

[[maybe_unused]] is the official replacement in C++17 for casting to void to avoid discarding computation due to optimization.

Copy link
Contributor Author

@ywkaras ywkaras Mar 3, 2023

Choose a reason for hiding this comment

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

cppreference.com as much as I does not seem to have gotten the memo you got about [[maybe_unused]]: https://en.cppreference.com/w/cpp/language/attributes/maybe_unused

@SolidWallOfCode SolidWallOfCode self-requested a review March 2, 2023 19:11
@ywkaras ywkaras merged commit 7a5ee19 into apache:master Mar 3, 2023
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
6f2cf2f 
* asf/master: (28 commits)
  Human readable timestamp for traffic_ctl config status (apache#9440)
  traffic_ctl - Remove legacy/pretty format output options (apache#9471)
  TSAN fix for fake_global_for_ink_queue. (apache#9183)
  libswoc: replace TextView in src/tscore/HostLookup.cc (apache#9437)
  records.yaml: Make sure when a string field is set to NULL then this (apache#9472)
  QUIC: Make sure the some of the quic configs get set into the quiche impl. (apache#9477)
  Replace httpbin with go-httpbin (apache#9475)
  Ethread::process_event(): make sure event mutex is unlocked before freeing event. (apache#9433)
  Use deprecated OpenSSL APIs for MD5 and SHA256 if available (apache#9469)
  Adds a --enable-lto option (Link Time Optimization) (apache#9464)
  traffic_ctl: Add support to monitor metrics. (apache#9423)
  QUIC: Make some adjustment to the qlog configuration (apache#9461)
  Added more debugging output for the xdebug plugin (apache#9467)
  Build: remove configure check for sys/mount.h, use C++17 builtin. (apache#9463)
  Fix SSLSessionDup for old OpenSSL and BoringSSL (apache#9444)
  Run autest tls_hooks17 and tls_hooks18 on BoringSSL build (apache#9455)
  Stabilize autest tls_hook18 (apache#9454)
  Fix parameter parser in ssl_hook_test plugin (apache#9453)
  Use SSL_get1_peer_certificate on OpenSSL3 build (apache#9460)
  records.yaml - Make some changes on the convert script. (apache#9466)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bryancall bryancall Awaiting requested review from bryancall

1 more reviewer

@SolidWallOfCode SolidWallOfCode SolidWallOfCode approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ywkaras ywkaras

Labels

Cleanup TSan

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@ywkaras @SolidWallOfCode @zwoop