Overuse of Interlocked.exchange?

Question

I am trying to understand correct usage of Interlocked.Exchange, so I am implementing a simple sorted LinkedList with add and remove functionality.

If this was not a threadsafe list, obviously to find the insertion point, you'd have something like the below to find the correct point to insert then new node.

    public void Insert(int newValue)
    {
        var prev = _header;
        Node curr = _header.Next;

        while(curr != null && curr.value > newValue )
        {
            prev = curr;
            curr = curr.Next;
        }
        var newNode = new Node(newValue, curr);
        prev.Next = newNode;
    }

Below is my take on how you'd have to do this for a concurrent list. Is there too much Interlocked.Exchange's going on? Without this, would the insert still be threadsafe? Would hundreds or thousands of Interlocked operations cause bad performance?

    public void InsertAsync(int newValue)
    {
        var prev = _header;
        Node curr = new Node(0, null);
        Interlocked.Exchange(ref curr, _header.Next);

        while (curr != null && curr.value > newValue)
        {
            prev = Interlocked.Exchange(ref curr, curr.Next);
        }
        //need some locking around prev.next first, ensure not modified/deleted, etc..
        //not in the scope of this question.
        var newNode = new Node(newValue, prev.Next);
        prev.Next = newNode;
    }

I understand that, for example, curr = curr.next is an atomic read, but can I be sure that a specific thread will read the most up to date value of curr.next, without the Interlocked?

Servy · Accepted Answer · 2012-10-19T18:11:41.613

6

Using an Interlocked method does two things:

It performs some series of operations that normally wouldn't be atomic, and makes them effectively atomic. In the case of Exchange you doing the equivalent of: var temp = first; first=second; return temp; but without risk of either variable being modified by another thread while you're doing that.
It introduces a memory barrier. It's possible for compiler, runtime, and or hardware optimizations to result in different threads having a local "copy" of a value that is technically in shared memory (normally as a result of caching variables). This can result in it taking a long time for one thread to "see" the results of a write in another thread. A memory barrier essentially syncs up all of these different versions of the same variable.

So, onto your code specifically. Your second solution isn't actually thread safe. Each individual Interlocked operation is atomic, but any number of calls to various Interlocked calls aren't atomic. Given everything that you're methods are doing, your critical sections are actually much larger; you'll need to use lock or another similar mechanism (i.e. a semaphore or monitor) to limit access to sections of code to only a single thread. In your particular case I'd imagine that the entirety of the method is a critical section. If you're really, really careful you may be able to have several smaller critical blocks, but it will be very difficult to ensure that there are no possible race conditions as a result.

As for performance, well, as it is the code doesn't work, and so performance is irrelevant.

edited Oct 19 '12 at 18:11

answered Oct 19 '12 at 17:38

Servy

202,030
26
332
449

Why can I be assured that current.Next has not been modified by another thread? curr is a local variable that points to some node in a linked list. That node's Next can be modified by any thread. – user981225 Oct 19 '12 at 17:47
`Why can I be assured that current.Next has not been modified by another thread?` You can't until you actually prevent anyone from accessing it. If you just want a thread safe linked list, as I said, you probably just want to wrap the entirety of all of your methods in `lock` statements. Doing anything more complex would be very hard, and it's unlikely you'd do it correctly. – Servy Oct 19 '12 at 17:51
Ok, understood. Just to make sure I actually am not 100% off base here - it would be theoretically possible for 'curr = curr.next' and 'Interlocked.Exchange(ref curr, curr.Next)' to result in different assignments to curr, correct? – user981225 Oct 19 '12 at 17:56
@user981225 No, that is not correct. You're essentially just doing a straight assignment, and that is already atomic. – Servy Oct 19 '12 at 18:04
But why can't the value of curr.next be stale/cached? Curr is a local variable that points to some node in the linked list. That node's 'next' can be modified by another thread. edit: I am not trying to argue with you, just trying to understand :) – user981225 Oct 19 '12 at 18:07
@user981225 It can, but that can happen in both cases. `curr.Next` will be resolved before `Interlocked.Exchange` is called; the second parameter isn't passed in by reference. – Servy Oct 19 '12 at 18:11

score 4 · Answer 2 · answered Oct 19 '12 at 17:41

Implementing a lock-free linked list is quite a bit more complicated than this. I strongly recommend that you don't do it. Just use plain old monitors instead. If this is just for interests sake, than you need to do some research into lock-free algorithms.

To more directly answer your question, the cost of an interlocked operation depends on the amount of contention over the variables. In a contention free scenario (e.g. single threaded), the cost is extremely minimal. As the number of concurrent accesses rise, the cost increases. The reason is that an interlocked operation isn't really lock free. It's simply using a hardware lock instead of a software one. For this reason, lock-free algorithms often do not perform as well as you would intuitively expect.

score 3 · Answer 3 · answered Oct 19 '12 at 17:36

3

You are only protecting the assignments, but they are atomic anyway. You should check if the Exchanged actually succeeded.

But that still wouldn't make your list thread-safe, a concurrent thread could be inserting a new value larger than your newValue before your insert point. I think the list could even end up being broken.

answered Oct 19 '12 at 17:36

H H

263,252
30
330
514

I understand the assignment is atomic. But what I am trying to accomplish is ensuring that curr.next is up to date. – user981225 Oct 19 '12 at 17:45

Overuse of Interlocked.exchange?

3 Answers3