Page MenuHomePhabricator

yuri (Yuri Per)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 14 2018, 3:36 AM (9 w, 4 d)

Recent Activity

Fri, Jan 11

yuri added a comment to D54889: Fiber support for thread sanitizer.

I have found out which optimization is applied by gcc but not by clang and did it manually. Executions times for new version:

Fri, Jan 11, 5:23 AM · Restricted Project
yuri updated the diff for D54889: Fiber support for thread sanitizer.

Optimized MemoryAccessImpl1()

Fri, Jan 11, 5:19 AM · Restricted Project

Tue, Jan 8

yuri added a comment to D54889: Fiber support for thread sanitizer.

"current" is compiler-rt HEAD with no local changes, or with the previous version of this change? This makes clang-compiled runtime 12% faster?

Tue, Jan 8, 2:25 AM · Restricted Project

Mon, Jan 7

yuri added a comment to D54889: Fiber support for thread sanitizer.

I improved test execution time. On my system I got following execution times (compared to original version of code):

Mon, Jan 7, 12:59 PM · Restricted Project
yuri updated the diff for D54889: Fiber support for thread sanitizer.

Improved performance

Mon, Jan 7, 12:46 PM · Restricted Project

Dec 20 2018

yuri updated the diff for D54889: Fiber support for thread sanitizer.

Fix typo

Dec 20 2018, 8:26 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

I added default synchronization and flag to opt-out.

Can you swicth to it in your codebase? I would expect that you need all (or almost all) of this synchronization anyway?

Dec 20 2018, 8:15 AM · Restricted Project
yuri added inline comments to D54889: Fiber support for thread sanitizer.
Dec 20 2018, 8:05 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

I don't completely follow logic behind cur_thread/cur_thread_fast/cur_thread1 and how it does not introduce slowdown.
cur_thread_fast still includes an indirection, so it looks exactly the same as the first version.
cur_thread_fast does not check for NULL, so I would expect it to be used only in few carefully chosen functions. But it's used in most entry functions. Which makes me wonder: when cur_thread_fast cannot be used if it can be used in all these places?
It seems that you use cur_thread only to lazily initialize cur_thread1 in Initialize or ThreadStart. But then the question is: why don't we explicitly intialize cur_thread1 in these functions, and then just have a single cur_thread function as before with no 2 subtly different accessors?

Dec 20 2018, 7:52 AM · Restricted Project
yuri updated the diff for D54889: Fiber support for thread sanitizer.
  • introduce ThreadType enum
  • cleanup internal interfaces
  • add pthread_exit() interceptor
  • more tests
Dec 20 2018, 5:42 AM · Restricted Project

Dec 18 2018

yuri added a comment to D54889: Fiber support for thread sanitizer.

https://android-review.googlesource.com/c/platform/external/qemu/+/844675

Latest version of patch doesn't work with QEMU anymore, at least with those annotations. Error log:

Do you know what I'm doing wrong here?


Please check if adding __attribute__((always_inline)) for start_switch_fiber() and finish_switch_fiber() helps

Dec 18 2018, 8:42 AM · Restricted Project

Dec 17 2018

yuri added a comment to D54889: Fiber support for thread sanitizer.
  1. I think instead of _tsan_get_current_fiber we could use pointer 0. It prevents things like switching to context of another thread, which won't end well. And just simplifies API surface.

For us possibility to use context of another thread is required feature. We use it to call into library which use fibers from external finber-unaware code.
It is on-par with Windows API for fibers - ConvertThreadToFiber().

Tell me more.
You have context for all fibers, tsan_create_fiber returns it.
_tsan_get_current_fiber is only useful for the physical thread. Do you use
tsan_switch_to_fiber to switch from a fiber to a non-current physical thread? I don't see how this can work. Or what do you do?

Dec 17 2018, 7:38 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.
  1. This introduces slowdown into the hottest runtime functions (branch and indirection in cur_thread). I think check_analyze.sh test will fail. Kinda hard to say, because it's broken already at the moment, but this perturbs runtime functions enough:

I have checked parts of the patch separately and found:

  • Part of patch related to HACKY_CALL does not affect code of critical functions and benchmark results
  • Part of patch related to proc() does not affect code of critical functions and benchmark results
  • The only part that affects critical functions is implementation cur_thread(). I tried to minimize its effect in new version of a patch.

Yes, cur_thread() is the problem. It's used in all the hottest functions.
As far as I see the current code still contains an indirection in cur_thread(). We need to think of a way to not affect hot paths.

Dec 17 2018, 7:23 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

Hi Dmitry,
I will try to answer your questions

  1. Do we want to synchronize fibers on switch? If fibers use TLS data, or cooperative scheduling (e.g. mutex unlock directly schedules the next critical section owner), or pass some data from prev fiber to next fiber, in all these cases not synchronizing fibers on switch will lead to false positives. As far as I remember in my old experiments I concluded that it's almost impossible to get rid of false positives it tasks/fibers are not synchronized on switch.

In case of single-threaded scheduler, it is worth to synchronize fibers on each switch. Currently I do it in client code using _tsan_release()/_tsan_acquire(), but it is possible to add flag for _tsan_switch_to_fiber() that will do it.

In case of multithreaded scheduler, client probably has own sychronization primitives (for example, custom mutex), and it is client's responsibility to call corresponding TSAN functions.

I think that synchronization will be exactly in the wrong place.
Consider, a thread pushes a fiber for execution on a thread pool. It locks a mutex and adds the fiber onto run queue. Then a thread pool thread dequeues the fiber from the run queue under the same mutex. Now the first thread and the thread pool thread are synchronized. Now the thread pool switches to the fiber context, no synchronization happens, so the fiber is _not_ synchronized with the submitting thread. This means that any passed data will cause false positives.
Even if the fiber is just created, in the above scheme even reading fiber entry function arguments (and even just writing to the stack) will cause false positives.

If fiber switch will synchronize parent thread and the fiber, then we will get the desired transitive synchronization.

Looking at the race reports produces by the qemu prototype, it seems there are actually all kinds of these false reports. There are false races on TLS storage. There are races on fiber start function. There are numerous races on arguments passed to fibers.

You said that for your case you need the synchronization, and it seems that it's also needed for all other cases too. I think we need to do it. People will get annotations wrong, or don't think about acquire/release at all, and then come to mailing list with questions about all these races. If it will prove to be needed, we can always add an opt-out of synchronization with a new flag for the annotations.

Dec 17 2018, 7:15 AM · Restricted Project
yuri updated the diff for D54889: Fiber support for thread sanitizer.
  • By default sync fibers with each other on switch
  • Faster cur_thread(), no visible difference in benchmark results
  • Moved implementation to tsan_rtl_thread.cc
Dec 17 2018, 7:10 AM · Restricted Project

Dec 11 2018

yuri abandoned D54835: tsan: Support calling ThreadCreate()/ThreadStart()/ThreadFinish() for non-current thread.
Dec 11 2018, 5:01 AM · Restricted Project
yuri planned changes to D54835: tsan: Support calling ThreadCreate()/ThreadStart()/ThreadFinish() for non-current thread.

Problem solved differently in D54889

Dec 11 2018, 5:00 AM · Restricted Project

Dec 10 2018

yuri updated the diff for D54889: Fiber support for thread sanitizer.
  • Temporary switch into fiber context for start and stop instead of avoiding HACKY_CALL
  • Minimized perfomance impact of cur_thread() change
  • Added (currently unused) flags to _tsan_create_fiber() and _tsan_switch_to_fiber()
Dec 10 2018, 5:42 AM · Restricted Project
yuri added inline comments to D54889: Fiber support for thread sanitizer.
Dec 10 2018, 5:11 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

Hi Dmitry,
I will try to answer your questions

Dec 10 2018, 4:48 AM · Restricted Project

Dec 3 2018

yuri added a comment to D54889: Fiber support for thread sanitizer.

I think single threaded programs would also benefit from swapcontext support. At the very least it will give correct stacks. I think tsan shadow stack can also overflow on some fibers patterns (if a fiber has uneven number of function entries and exits).

Dec 3 2018, 4:23 AM · Restricted Project
yuri updated the diff for D54889: Fiber support for thread sanitizer.
  • Excluded macOS support in order to reduce patch size
  • Added comment in tsan_interface.h
  • Added tests
Dec 3 2018, 4:13 AM · Restricted Project

Nov 27 2018

yuri added a comment to D54889: Fiber support for thread sanitizer.

It's unclear how this is supposed to be used. I would expect to see an interceptor for swapcontext that will make use of this, but there is none...

Interceptor for swapcontext without additional sanitizer API is not enough because it is not known when context is created and destroyed. Actually, I am not sure if swapcontext is actually used by someone because it is slow and has strange limitations.

My interface assumes is that users should call __tsan_switch_to_fiber() immediately before switching context and then call swapcontext or whatever he want to perform actual switch.

What do people use nowadays?

Nov 27 2018, 6:32 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

I'd like to make sure that the work can be re-used to implement proper understanding of Darwin GCD queues.

What kind of API is required for GCD queues?
A while ago I sketched something for tasking support:
https://codereview.appspot.com/6904068/diff/6001/rtl/tsan_interface_task.h
It would be nice to have tasking support too (nobody programs in threads today), but I don't yet know how close this fibers support to tasking support.

Nov 27 2018, 5:51 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

It's unclear how this is supposed to be used. I would expect to see an interceptor for swapcontext that will make use of this, but there is none...

Nov 27 2018, 5:21 AM · Restricted Project
yuri added a comment to D54889: Fiber support for thread sanitizer.

Can you explain what this patch does a bit? The way I see it, it's just adding an extra level of indirection to each cur_thread() call. Is that it? Does that affect the performance of TSan?

Nov 27 2018, 5:07 AM · Restricted Project

Nov 26 2018

yuri added a comment to D54835: tsan: Support calling ThreadCreate()/ThreadStart()/ThreadFinish() for non-current thread.

Fibers support sounds great.
Regardless of how we proceed with submitting patches, I would like to see the whole change first. Otherwise it's hard to see reasoning behind individual changes and understand if it's right approach or not.
Please upload whole change into a separate review.

Nov 26 2018, 2:10 AM · Restricted Project
yuri created D54889: Fiber support for thread sanitizer.
Nov 26 2018, 2:08 AM · Restricted Project

Nov 22 2018

yuri added a comment to D54835: tsan: Support calling ThreadCreate()/ThreadStart()/ThreadFinish() for non-current thread.

I have working implemented of fibers (coroutines) support for thread sanitizer.
Taking in the account that fibers are supported by address sanitizer, support in thread sanitizer may be useful as well.

Nov 22 2018, 9:58 AM · Restricted Project
yuri created D54835: tsan: Support calling ThreadCreate()/ThreadStart()/ThreadFinish() for non-current thread.
Nov 22 2018, 9:50 AM · Restricted Project

Nov 21 2018

yuri added a comment to D54521: tsan: Add pthread_tryjoin_np and pthread_timedjoin_np interceptors.

Do you want me to merge this or you have commit access?

Nov 21 2018, 1:22 AM · Restricted Project
yuri updated the diff for D54521: tsan: Add pthread_tryjoin_np and pthread_timedjoin_np interceptors.

Improved tests

Nov 21 2018, 1:12 AM · Restricted Project
yuri added inline comments to D54521: tsan: Add pthread_tryjoin_np and pthread_timedjoin_np interceptors.
Nov 21 2018, 12:02 AM · Restricted Project

Nov 20 2018

yuri updated the diff for D54521: tsan: Add pthread_tryjoin_np and pthread_timedjoin_np interceptors.

Added tests and fixed code, now it really works

Nov 20 2018, 6:41 AM · Restricted Project

Nov 14 2018

yuri created D54521: tsan: Add pthread_tryjoin_np and pthread_timedjoin_np interceptors.
Nov 14 2018, 3:55 AM · Restricted Project