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.

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

[mempool] A new (atomic) way to deal with data races #5255

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@cherusker
Contributor

cherusker commented Jul 24, 2017

While slowly discovering all the secrets of Mono, I also discovered atomic.h which provides extremely useful functions to deal with a ton of Mono's currently known data races. I know, only recently did I ask to blacklist the reporting counter races in mempool.c (#5191). However, having atomic.h at hand, I think it's way better to fix the races (lock-free) than simply blacklisting them (and their surroundings)?

In addition, I added InterlockedSubtract () as well as InterlockedSubtract64 () to atomic.h as these functions do not seem to exist but, IMO, they would really help with the readability. E.g. when converting races like foo -= bar we could use InterlockedSubtract (&foo, bar) instead of using InterlockedAdd (&foo, -1 * bar)). I tried my best to research all (cross-platform) aspects and compared everything to InterlockedAdd () and InterlockedAdd64 (). However, of course, I would very much appreciate it, if experts in this field have a close look at this.

As soon as these InterlockedSubtract* () functions are approved, I will add a follow-up PR where I can finally tackle a large amount of data races - in a clean and very satisfactory way.

@cherusker cherusker requested a review from alexrp as a code owner Jul 24, 2017

@dnfclas

This comment has been minimized.

dnfclas commented Jul 24, 2017

@cherusker,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@vargaz

This comment has been minimized.

Member

vargaz commented Jul 24, 2017

The atomic functions do synchronization, so they are way slower than a normal addition. We do use atomic operations to update counters in non-perf critical places, but mempool is perf critical.

@cherusker

This comment has been minimized.

Contributor

cherusker commented Jul 24, 2017

I have also talked to the maintainers of TSan (I originally asked about the blacklisting of variables) and they seem really dedicated to finding solutions for ALL races ( https://groups.google.com/forum/#!topic/thread-sanitizer/ZfB_Q5xA790 ). When I told them about the general issue of atomic counters in combination with performance-critical code (like the one we have in mempool), Dmitry proposed a C++ solution - stat.store(stat.load(std::memory_order_relaxed) + 1, std::memory_order_relaxed). Does anyone know about a similar thing in C that we could / want to use?

@cherusker

This comment has been minimized.

Contributor

cherusker commented Aug 8, 2017

#5310 seems like a better solution that fits with for more current situations. We might come back to this topic (updates of atomic.h) at some point; it will be within a different context most likely though.

@cherusker cherusker closed this Aug 8, 2017

@cherusker cherusker deleted the cherusker:cherusker-2017-07-24-atmoic-inc-dec-add-sub branch Aug 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment