Atomic cargo cults

Cargo cult programming refers to “ritual inclusion of code or program structures that serve no real purpose.” One annoying example of this that I see a lot in kernel code that I review is the inappropriate use of atomic_t, in the belief that it magically wards off races.

This type of bogosity is usually marked by variables or structure members of type atomic_t, which are only ever accessed through atomic_read() and atomic_set() without ever using a real atomic operation such as atomic_inc() or atomic_dec_and_test(). Such programming reaches its apotheosis in code like:

        atomic_set(&head, (atomic_read(&head) + 1) % size);

(and yes, this is essentially real code, although I’ve paraphrased it to protect the guilty from embarrassment).

The only point of atomic_t is that arithmetic operations (like atomic_inc() et al) are atomic in the sense that there is no window where two racing operations can step on each other. This helps with situations where an int variable might have ++i and --i race with each other; since these operations are probably implemented as read-modify-write. So if i starts out as 0, the value of i after the two operations might be 0, -1 or 1 depending on when each operation reads the value of i and what order they write back the final value.

If you never use any of these atomic operations, then there’s no point in using atomic_set() to assign a variable and atomic_read() to read it. Your code is no more or less safe than if would be just using normal int variables and assignments.

One way to think about atomic operations is that they might be an optimization instead of using a lock to protect access to a variable. Unfortunately I’m not sure if this will help anyone get things right, since I also see plenty of code like:

int x;

int foo(void)
{

        int y;

        spin_lock(&lock);
        y = x;
        spin_unlock(&lock);

        return y;
}

which is the analogous cargo-cult anti-pattern for spinlocks.

Maybe the best rule would be: if the only functions using atomic_t in your code are atomic_set() and atomic_read(), then you need to write a comment explaining why you’re using atomic at all. Since the vast majority of the time, such a comment will be impossible to write, maybe this will cut down on cargo cult programming a bit. Or more likely it will just make code review more fun by generating nonsensical comments for me to chuckle at.

6 Responses to “Atomic cargo cults”

  1. Josh Stone says:

    I won’t argue that your examples are acceptable code — the inc-mod especially makes me sad. But consider the following:

    The only reason atomic_set() and atomic_read() are useless here is because you’re assuming that reading or writing an int is always an atomic operation. This is probably true of the architectures you care about, but can you really say that this will be true for all architectures? Using atomic_t explicitly declares the need for the assignment to be atomic, where that abstraction may then do whatever is necessary on every architecture.

    The other side effect of using atomic_t is that your variable becomes volatile, which may have an important effect on the compiled code.

    So you gain abstracted atomicity and volatileness. Are you still willing to say that this isn’t worthwhile? Or is it just that this need is outside of what you consider “the vast majority of the time”?

    PS – I hope you’ll excuse my pedantry for the sake of argument 🙂

  2. roland says:

    Josh: thanks for the comments. Let me address them in reverse order.

    You say that using atomic_t makes your variable volatile. This is not true: on mainstream architectures such as i386 and x86_64, has

    typedef struct { int counter; } atomic_t;

    with no volatile in sight. In any case, the recent thread on LKML about adding documentation about “volatile considered harmful” to the kernel shows that in almost all cases, you don’t want your variable to be declared volatile anyway. When reviewing code, I can be pretty sure that an explicit “volatile” declaration is either unnecessary or marking a bug.

    As far as assuming that reading and writing an int are atomic, I think that assumption is pretty deeply ingrained in the Linux implementation. Just about every lock-free algorithm that uses memory barriers is implicitly assuming that it will never read a half-written value. And certainly every implementation of atomic_read() for all Linux architectures boils down to a simple read of an int.

    So no, I don’t think that “abstracted atomicity” is a reason to use atomic_t.

    (BTW, It is legitimate to use atomic_t and atomic_set() if you are also using atomic bit ops)

  3. Josh Stone says:

    Well, I was pretty sure atomic_t was volatile, so you got me to do some research. It looks like this was a fairly recent change. The log for f9e9dcb38f5106fa8cdac04a9e967d5487f1cd20 in git reads thus:

    x86[-64]:Remove ‘volatile’ from atomic_t
    Linus Torvalds [Wed, 6 Dec 2006 22:42:57 +0000 (14:42 -0800)]
    Any code that relies on the volatile would be a bug waiting to happen
    anyway.

    Don’t encourage people to think that putting ‘volatile’ on data
    structures somehow fixes problems. We should always use proper locking
    (and other serialization) techniques.

    Thanks for the pointer to the “volatile considered harmful” discussion. That addition to the documentation looks very worthwhile.

    Many other architectures still do have volatile on their atomic_t typedef. Is this because Linus hasn’t gotten around to fixing those yet, or is there some reason that volatile is more appropriate on other architectures?

    A final note about the atomicity of reading and writing ints — your post about __attribute__((packed)) shows how this assumption sometimes doesn’t hold. If someone put an atomic_t inside a packed structure, then all bets are off, right? That would surely be a bug, but perhaps atomic_t should have an __attribute__((aligned))?

  4. […] Zaitcev replied to my earlier post about misuse of atomic variables. I never really thought of myself as a crusader or as particularly […]

  5. Narty Atomic says:

    The other side effect of using atomic_t is that your variable becomes volatile, which may have an important effect on the compiled code.

  6. roland says:

    Narty: see the comments above, which discuss the fact that atomic_t is not volatile on x86 at least.