Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24012 )
Change subject: KUDU-3735: fix data race in SignalData init ...................................................................... Patch Set 1: (5 comments) Thank you for taking a look at this. A couple of things: 1) I'm not sure that KUDU-3735 isn't a false positive -- they might be just having issues to let ANNOTATE_HAPPENS_BEFORE work as expected and help TSAN with figuring out what's going on. 2) If the report in KUDU-3735 isn't a false positive, then I'm not sure I understand how this is going to address the underlying problem. BTW, how did you verify that this patch addresses the reported TSAN race, assuming it's not a false positive? http://gerrit.cloudera.org:8080/#/c/24012/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24012/1//COMMIT_MSG@16 PS1, Line 16: The race was reported in Impala which vendors this code and builds with : GCC 10.4.0 + libstdc++. Does the compiler emit proper macros to enable ANNOTATE_HAPPENS_BEFORE and ANNOTATE_HAPPENS_AFTER annotations in that enviroment? http://gerrit.cloudera.org:8080/#/c/24012/1//COMMIT_MSG@17 PS1, Line 17: Kudu's own TSAN build uses Clang 11 + libc++, : which emits an atomic store in the atomic<T> constructor and therefore : does not trigger the report. Could you post a couple of references to show that the underlying 'atomic' store in the constructor in libc++ is different from libstdc++ 'non-atomic' store for the corresponding constructors of std::atomic<int64>? Thank you! http://gerrit.cloudera.org:8080/#/c/24012/1//COMMIT_MSG@22 PS1, Line 22: - Replacing the in-class initializer with an explicit atomic store in : a user-defined constructor, ensuring the initialization is always : an atomic operation regardless of stdlib implementation. If there were a race as reported in KUDU-3735, then I'd expect the same race with this update as well since I don't see how this patch addresses the underlying issue, but I might be missing something. http://gerrit.cloudera.org:8080/#/c/24012/1//COMMIT_MSG@25 PS1, Line 25: - Setting data->stack before publishing data->queued_to_tid, and using : memory_order_release on the store so the signal handler is guaranteed : to observe a valid stack pointer whenever it observes the tid. BTW, the original memory ordering is even stronger (std::memory_order_seq_cst), and it automatically guarantees access to the valid stack pointer, IIUC. https://en.cppreference.com/w/cpp/atomic/atomic/operator=.html http://gerrit.cloudera.org:8080/#/c/24012/1/src/kudu/util/debug-util.cc File src/kudu/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/24012/1/src/kudu/util/debug-util.cc@455 PS1, Line 455: : // 2. The store is an atomic operation, consistent with all other accesses to : // this field, avoiding the non-atomic/atomic mixed-access UB that the : // in-class initializer { kNotInUse } would have caused. BTW, the original code is an atomic operation as well with even stronger memory ordering: it's the assignment operator for std::atomic with memory_order_seq_cst ordering. -- To view, visit http://gerrit.cloudera.org:8080/24012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I566c427aa835732af8c0ef686346a8cd40a1eca1 Gerrit-Change-Number: 24012 Gerrit-PatchSet: 1 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Sat, 21 Feb 2026 00:07:36 +0000 Gerrit-HasComments: Yes
