make atomic_read() behave consistently on alpha

From: Chris Snook <[email protected]>

Purify volatile use for atomic[64]_t on alpha.

Signed-off-by Chris Snook <[email protected]>

include/asm-alpha/atomic.h +5 -6

@@ -14,18 +14,22 @@
 
 
 /*
- * Counter is volatile to make sure gcc doesn't try to be clever
- * and move things around on us. We need to use _exactly_ the address
- * the user gave us, not some alias that contains the same information.
+ * Make sure gcc doesn't try to be clever and move things around
+ * on us. We need to use _exactly_ the address the user gave us,
+ * not some alias that contains the same information.
  */
-typedef struct { volatile int counter; } atomic_t;
-typedef struct { volatile long counter; } atomic64_t;
+typedef struct { int counter; } atomic_t;
+typedef struct { long counter; } atomic64_t;
 
 #define ATOMIC_INIT(i)		( (atomic_t) { (i) } )
 #define ATOMIC64_INIT(i)	( (atomic64_t) { (i) } )
 
-#define atomic_read(v)		((v)->counter + 0)

-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Why not the same access-once semantics for atomic_set() as for atomic_read()? As this patch stands, it might introduce architecture-specific compiler-induced bugs due to the fact that atomic_set() used to imply volatile behavior but no longer does.

Quoted 4 lines · Paul E. McKenney

Why not the same access-once semantics for atomic_set() as for atomic_read()? As this patch stands, it might introduce architecture-specific compiler-induced bugs due to the fact that atomic_set() used to imply volatile behavior but no longer does.

When we make the volatile cast in atomic_read(), we're casting an rvalue to volatile. This unambiguously tells the compiler that we want to re-load that register from memory. What's "volatile behavior" for an lvalue?

A
write to an lvalue already implies an eventual write to memory, so this would be a no-op. Maybe you'll write to the register a few times before flushing it to memory, but it will happen eventually. With an rvalue, there's no guarantee that it will *ever* load from memory, which is what volatile fixes.

I think what you have in mind is LOCK_PREFIX behavior, which is not the purpose of atomic_set. We use LOCK_PREFIX in the inline assembly for the atomic_* operations that read, modify, and write a value, only because it is necessary to perform that entire transaction atomically.

Quoted 6 lines · Chris Snook

Quoted 4 lines · Paul E. McKenney

Why not the same access-once semantics for atomic_set() as for atomic_read()? As this patch stands, it might introduce architecture-specific compiler-induced bugs due to the fact that atomic_set() used to imply volatile behavior but no longer does.

When we make the volatile cast in atomic_read(), we're casting an rvalue to volatile. This unambiguously tells the compiler that we want to re-load that register from memory. What's "volatile behavior" for an lvalue?

I was absolutely -not- suggesting volatile behavior for lvalues.

Instead, I am asking for volatile behavior from an -rvalue-. In the case of atomic_read(), it is the atomic_t being read from. In the case of atomic_set(), it is the atomic_t being written to. As suggested in my previous email:

#define atomic_set(v,i) ((*(volatile int *)&(v)->counter) = (i))
#define atomic64_set(v,i) ((*(volatile long *)&(v)->counter) = (i))

Again, the architectures that used to have their "counter" declared as volatile will lose volatile semantics on atomic_set() with your patch, which might result in bugs due to overly imaginative compiler optimizations. The above would prevent any such bugs from appearing.

Quoted 3 lines

A
write to an lvalue already implies an eventual write to memory, so this would be a no-op. Maybe you'll write to the register a few times before flushing it to memory, but it will happen eventually. With an rvalue, there's no guarantee that it will *ever* load from memory, which is what volatile fixes.

Quoted 1 line

I think what you have in mind is LOCK_PREFIX behavior, which is not the purpose of atomic_set. We use LOCK_PREFIX in the inline assembly for the atomic_* operations that read, modify, and write a value, only because it is necessary to perform that entire transaction atomically.

No LOCK_PREFIX, thank you!!! I just want to make sure that the compiler doesn't push the store down out of a loop, split the store, allow the store to happen twice (e.g., to allow different code paths to be merged), and all the other tricks that the C standard permits compilers to pull.

We can't have split stores because we don't use atomic64_t on 32-bit architectures.

Quoted 1 line

We can't have split stores because we don't use atomic64_t on 32-bit architectures.

That's not true; the compiler is free to split all stores (and reads) from memory however it wants. It is debatable whether "volatile" would prevent this as well, certainly it is unsafe if you want to be portable. GCC will do its best to not split volatile memory accesses, but bugs in this area do happen a lot (because the compiler code for volatile isn't as well exercised as most other compiler code, and because it is simply a hard subject; and there is no real formalised model for what GCC should do).

The only safe way to get atomic accesses is to write assembler code. Are there any downsides to that? I don't see any.

Quoted 3 lines

The only safe way to get atomic accesses is to write assembler code. Are there any downsides to that? I don't see any.

The assumption that aligned word reads and writes are atomic, and that words are aligned unless explicitly packed otherwise, is endemic in the kernel. No sane compiler violates this assumption. It's true that we're not portable to insane compilers after this patch, but we never were in the first place.

Quoted 5 lines

Quoted 3 lines

The only safe way to get atomic accesses is to write assembler code. Are there any downsides to that? I don't see any.

The assumption that aligned word reads and writes are atomic, and that words are aligned unless explicitly packed otherwise, is endemic in the kernel. No sane compiler violates this assumption. It's true that we're not portable to insane compilers after this patch, but we never were in the first place.

You didn't answer my question: are there any downsides to using explicit coded-in-assembler accesses for atomic accesses? You can handwave all you want that it should "just work" with volatile accesses, but volatility != atomicity, volatile in C is really badly defined, GCC never officially gave stronger guarantees, and we have a bugzilla full of PRs to show what a minefield it is.

So, why not use the well-defined alternative?

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

The only safe way to get atomic accesses is to write assembler code. Are there any downsides to that? I don't see any.

The assumption that aligned word reads and writes are atomic, and that words are aligned unless explicitly packed otherwise, is endemic in the kernel. No sane compiler violates this assumption. It's true that we're not portable to insane compilers after this patch, but we never were in the first place.

Quoted 8 lines

You didn't answer my question: are there any downsides to using explicit coded-in-assembler accesses for atomic accesses? You can handwave all you want that it should "just work" with volatile accesses, but volatility != atomicity, volatile in C is really badly defined, GCC never officially gave stronger guarantees, and we have a bugzilla full of PRs to show what a minefield it is.
So, why not use the well-defined alternative?

Because we don't need to,

and it hurts performance.

Quoted 18 lines · Segher Boessenkool

Quoted 9 lines

Quoted 3 lines

The only safe way to get atomic accesses is to write assembler code. Are there any downsides to that? I don't see any.

The assumption that aligned word reads and writes are atomic, and that words are aligned unless explicitly packed otherwise, is endemic in the kernel. No sane compiler violates this assumption. It's true that we're not portable to insane compilers after this patch, but we never were in the first place.

You didn't answer my question: are there any downsides to using explicit coded-in-assembler accesses for atomic accesses? You can handwave all you want that it should "just work" with volatile accesses, but volatility != atomicity, volatile in C is really badly defined, GCC never officially gave stronger guarantees, and we have a bugzilla full of PRs to show what a minefield it is.

Quoted 1 line

So, why not use the well-defined alternative?

Because we don't need to, and it hurts performance.

Quoted 17 lines · Chris Snook

Quoted 16 lines · Segher Boessenkool

Quoted 8 lines

Quoted 3 lines

The only safe way to get atomic accesses is to write assembler code. Are there any downsides to that? I don't see any.

The assumption that aligned word reads and writes are atomic, and that words are aligned unless explicitly packed otherwise, is endemic in the kernel. No sane compiler violates this assumption. It's true that we're not portable to insane compilers after this patch, but we never were in the first place.

You didn't answer my question: are there any downsides to using explicit coded-in-assembler accesses for atomic accesses? You can handwave all you want that it should "just work" with volatile accesses, but volatility != atomicity, volatile in C is really badly defined, GCC never officially gave stronger guarantees, and we have a bugzilla full of PRs to show what a minefield it is.

Quoted 2 lines

Quoted 1 line

So, why not use the well-defined alternative?

Because we don't need to, and it hurts performance.

It hurts performance by implementing 32-bit atomic reads in assembler?

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

So, why not use the well-defined alternative?

Because we don't need to, and it hurts performance.

It hurts performance by implementing 32-bit atomic reads in assembler?

No, I misunderstood the question. Implementing 32-bit atomic reads in assembler is redundant, because any sane compiler, *particularly* and optimizing compiler (and we're only in this mess because of optimizing compilers)

will give us that automatically without the assembler.

Yes, it is legal for a compiler to violate this assumption. It is also legal for us to refuse to maintain compatibility with compilers that suck this badly.

Quoted 18 lines · Paul E. McKenney

Quoted 9 lines · Chris Snook

Quoted 4 lines · Paul E. McKenney

Why not the same access-once semantics for atomic_set() as for atomic_read()? As this patch stands, it might introduce architecture-specific compiler-induced bugs due to the fact that atomic_set() used to imply volatile behavior but no longer does.

When we make the volatile cast in atomic_read(), we're casting an rvalue to volatile. This unambiguously tells the compiler that we want to re-load that register from memory. What's "volatile behavior" for an lvalue?

I was absolutely -not- suggesting volatile behavior for lvalues.

Instead, I am asking for volatile behavior from an -rvalue-. In the case of atomic_read(), it is the atomic_t being read from. In the case of atomic_set(), it is the atomic_t being written to. As suggested in my previous email:

Quoted 3 lines

#define atomic_set(v,i) ((*(volatile int *)&(v)->counter) =
(i))
#define atomic64_set(v,i) ((*(volatile long *)&(v)->counter) = (i))

That looks like a volatile lvalue to me. I confess I didn't exactly ace compilers. Care to explain this?

Quoted 17 lines

Again, the architectures that used to have their "counter" declared as volatile will lose volatile semantics on atomic_set() with your patch, which might result in bugs due to overly imaginative compiler optimizations. The above would prevent any such bugs from appearing.

Quoted 11 lines

A
write to an lvalue already implies an eventual write to memory, so this would be a no-op. Maybe you'll write to the register a few times before flushing it to memory, but it will happen eventually. With an rvalue, there's no guarantee that it will *ever* load from memory, which is what volatile fixes.

I think what you have in mind is LOCK_PREFIX behavior, which is not the purpose of atomic_set. We use LOCK_PREFIX in the inline assembly for the atomic_* operations that read, modify, and write a value, only because it is necessary to perform that entire transaction atomically.

Quoted 4 lines

No LOCK_PREFIX, thank you!!! I just want to make sure that the compiler doesn't push the store down out of a loop, split the store, allow the store to happen twice (e.g., to allow different code paths to be merged), and all the other tricks that the C standard permits compilers to pull.

We can't have split stores because we don't use atomic64_t on 32-bit architectures. volatile won't save you from pushing stores out of loops unless you're also doing reads. This is why we use reads to flush writes to mmio registers. In this case, an atomic_read() with volatile in it will suffice. Storing twice is perfectly legal here, though it's unlikely that an optimizing compiler smart enough to create the problem we're addressing would ever do that.

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Use of volatile does in fact save you from the compiler pushing stores out of loops regardless of whether you are also doing reads. The C standard has the notion of sequence points, which occur at various places including the ends of statements and the control expressions for "if" and "while" statements. The compiler is not permitted to move volatile references across a sequence point. Therefore, the compiler is not allowed to push a volatile store out of a loop. Now the CPU might well do such a reordering, but that is a separate issue to be dealt with via memory barriers. Note that it is the CPU and I/O system, not the compiler, that is forcing you to use reads to flush writes to MMIO registers.

And you would be amazed at what compiler writers will do in order to get an additional fraction of a percent out of SpecCPU...

In short, please retain atomic_set()'s volatility, especially on those architectures that declared the atomic_t's counter to be volatile.

Quoted 4 lines · Paul E. McKenney

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Yes, but we don't write code for these compilers. There are countless pieces of kernel code which would break in this condition, and there doesn't seem to be any interest in fixing this.

Quoted 10 lines

Use of volatile does in fact save you from the compiler pushing stores out of loops regardless of whether you are also doing reads. The C standard has the notion of sequence points, which occur at various places including the ends of statements and the control expressions for "if" and "while" statements. The compiler is not permitted to move volatile references across a sequence point. Therefore, the compiler is not allowed to push a volatile store out of a loop. Now the CPU might well do such a reordering, but that is a separate issue to be dealt with via memory barriers. Note that it is the CPU and I/O system, not the compiler, that is forcing you to use reads to flush writes to MMIO registers.

Sequence points enforce read-after-write ordering, not write-after-write. We flush writes with reads for MMIO because of this effect as well as the CPU/bus effects.

Quoted 2 lines

And you would be amazed at what compiler writers will do in order to get an additional fraction of a percent out of SpecCPU...

Probably not :)

Quoted 2 lines

In short, please retain atomic_set()'s volatility, especially on those architectures that declared the atomic_t's counter to be volatile.

Like i386 and x86_64? These used to have volatile in the atomic_t declaration. We removed it, and the sky did not fall.

Quoted 17 lines · Chris Snook

Quoted 4 lines · Paul E. McKenney

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Yes, but we don't write code for these compilers. There are countless pieces of kernel code which would break in this condition, and there doesn't seem to be any interest in fixing this.

Quoted 10 lines

Use of volatile does in fact save you from the compiler pushing stores out of loops regardless of whether you are also doing reads. The C standard has the notion of sequence points, which occur at various places including the ends of statements and the control expressions for "if" and "while" statements. The compiler is not permitted to move volatile references across a sequence point. Therefore, the compiler is not allowed to push a volatile store out of a loop. Now the CPU might well do such a reordering, but that is a separate issue to be dealt with via memory barriers. Note that it is the CPU and I/O system, not the compiler, that is forcing you to use reads to flush writes to MMIO registers.

Quoted 1 line

Sequence points enforce read-after-write ordering, not write-after-write. We flush writes with reads for MMIO because of this effect as well as the CPU/bus effects.

Neither volatile reads nor volatile writes may be moved across sequence points.

Quoted 7 lines

Quoted 2 lines

And you would be amazed at what compiler writers will do in order to get an additional fraction of a percent out of SpecCPU...

Probably not :)

Quoted 2 lines

In short, please retain atomic_set()'s volatility, especially on those architectures that declared the atomic_t's counter to be volatile.

Like i386 and x86_64? These used to have volatile in the atomic_t declaration. We removed it, and the sky did not fall.

Interesting. You tested all possible configs on all possible hardware?

Quoted 22 lines · Paul E. McKenney

Quoted 21 lines · Chris Snook

Quoted 4 lines · Paul E. McKenney

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Yes, but we don't write code for these compilers. There are countless pieces of kernel code which would break in this condition, and there doesn't seem to be any interest in fixing this.

Quoted 12 lines

Use of volatile does in fact save you from the compiler pushing stores out
of loops regardless of whether you are also doing reads. The C standard has the notion of sequence points, which occur at various places including
the ends of statements and the control expressions for "if" and "while" statements. The compiler is not permitted to move volatile references across a sequence point. Therefore, the compiler is not allowed to push a volatile store out of a loop. Now the CPU might well do such a reordering, but that is a separate issue to be dealt with via memory barriers. Note that it is the CPU and I/O system, not the compiler, that is forcing you to use reads to flush writes to MMIO registers.

Quoted 6 lines

Quoted 3 lines

Sequence points enforce read-after-write ordering, not write-after-write. We flush writes with reads for MMIO because of this effect as well as the CPU/bus effects.

Neither volatile reads nor volatile writes may be moved across sequence points.

By the compiler, or by the CPU?

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

Quoted 9 lines

Quoted 8 lines

Quoted 2 lines

And you would be amazed at what compiler writers will do in order to get an additional fraction of a percent out of SpecCPU...

Probably not :)

Quoted 2 lines

In short, please retain atomic_set()'s volatility, especially on those architectures that declared the atomic_t's counter to be volatile.

Like i386 and x86_64? These used to have volatile in the atomic_t declaration. We removed it, and the sky did not fall.

Quoted 1 line

Interesting. You tested all possible configs on all possible hardware?

No, but I can reason about it and be confident that this won't break anything that isn't already broken. At worst, this patch will make any existing subtly incorrect uses of atomic_t much more obvious and easier to track down.

Quoted 2 lines · Chris Snook

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

The case that it -can- change is interactions with interrupt handlers. And NMI/SMI handlers, for that matter.

Quoted 13 lines · Paul E. McKenney

Quoted 11 lines · Chris Snook

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

Quoted 2 lines

The case that it -can- change is interactions with interrupt handlers. And NMI/SMI handlers, for that matter.

You have a point here, but only if you can guarantee that the interrupt handler is running on a processor sharing the cache that has the not-yet-written volatile value. That implies a strictly non-SMP architecture. At the moment, none of those have volatile in their declaration of atomic_t, so this patch can't break any of them.

Quoted 4 lines · Chris Snook

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Chris Snook

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

Quoted 3 lines

Quoted 2 lines

The case that it -can- change is interactions with interrupt handlers. And NMI/SMI handlers, for that matter.

You have a point here, but only if you can guarantee that the interrupt handler is running on a processor sharing the cache that has the not-yet-written volatile value. That implies a strictly non-SMP architecture. At the moment, none of those have volatile in their declaration of atomic_t, so this patch can't break any of them.

This can also happen when using per-CPU variables. And there are a number of per-CPU variables that are either atomic themselves or are structures containing atomic fields.

Quoted 22 lines · Paul E. McKenney

Quoted 13 lines · Chris Snook

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Chris Snook

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

Quoted 7 lines

Quoted 2 lines

The case that it -can- change is interactions with interrupt handlers. And NMI/SMI handlers, for that matter.

You have a point here, but only if you can guarantee that the interrupt handler is running on a processor sharing the cache that has the not-yet-written volatile value. That implies a strictly non-SMP architecture. At the moment, none of those have volatile in their declaration of atomic_t, so this patch can't break any of them.

Quoted 3 lines

This can also happen when using per-CPU variables. And there are a number of per-CPU variables that are either atomic themselves or are structures containing atomic fields.

Accessing per-CPU variables in this fashion reliably already requires a suitable smp/non-smp read/write memory barrier. I maintain that if we break anything with this change, it was really already broken, if less obviously. Can you give a real or synthetic example of legitimate code that could break?

Quoted 6 lines · Chris Snook

Quoted 5 lines · Paul E. McKenney

Quoted 4 lines · Chris Snook

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Chris Snook

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

The case that it -can- change is interactions with interrupt handlers. And NMI/SMI handlers, for that matter.

You have a point here, but only if you can guarantee that the interrupt handler is running on a processor sharing the cache that has the not-yet-written volatile value. That implies a strictly non-SMP architecture. At the moment, none of those have volatile in their declaration of atomic_t, so this patch can't break any of them.

This can also happen when using per-CPU variables. And there are a number of per-CPU variables that are either atomic themselves or are structures containing atomic fields.

Accessing per-CPU variables in this fashion reliably already requires a suitable smp/non-smp read/write memory barrier. I maintain that if we break anything with this change, it was really already broken, if less obviously. Can you give a real or synthetic example of legitimate code that could break?

My main concern is actually the lack of symmetry -- I would expect that an atomic_set() would have the same properties as atomic_read(). It is easy and cheap to provide them with similar properties, so why not? Debugging even a single problem would consume far more time than simply giving them corresponding semantics.

But you asked for examples. These are synthetic, and of course legitimacy is in the eye of the beholder.

1. Watchdog variable.

atomic_t watchdog = ATOMIC_INIT(0);

...

int i;
while (!done) {

/* Do so stuff that doesn't take more than a few us. */ /* Could do atomic increment, but throughput penalty. */

i++;
atomic_set(&watchdog, i);
}
do_something_with(&watchdog);

/* Every so often on some other CPU... */

if ((new_watchdog = atomic_read(&watchdog)) == old_watchdog)
die_horribly();
old_watchdog = new_watchdog;

If atomic_set() did not have volatile semantics, the compiler would be within its rights optimizing it to simply get the final value of "i" after exit from the loop. This would cause the watchdog check to fail spuriously. Memory barriers are not required in this case, because the CPU cannot hang onto the value for very long -- we don't care about the exact value, or about exact synchronization, but rather about whether or not the value is changing.

In this (toy) example, one might replace the atomic_set() with an atomic increment (though that might be too expensive in some cases) or with something like:

atomic_set(&watchdog, atomic_read(&watchdog) + 1);

However, other cases might not permit this transformation, for example, an existing heavily used API might take int rather than atomic_t.

Some will no doubt argue that this example should use a macro or an asm similar to the "forget()" asm put forward elsewhere in this thread.

2. Communicating both with interrupt handler and with other CPUs.

    For example, data elements that are built up in a location visible
    to interrupts and NMIs, and then added as a unit to a data structure
    visible to other CPUs.  This more-realistic example is abbreviated
    to the point of pointlessness as follows:

	struct foo {
		atomic_t a;
		atomic_t b;
	};

	DEFINE_PER_CPU(struct foo *, staging) = NULL;

	/* Create element in staging area. */

	__get_cpu_var(staging) = kzalloc(sizeof(*p), GFP_WHATEVER);
	if (__get_cpu_var(staging) == NULL)
		die_horribly();
	/* allocate an element of some per-CPU array, get the result in "i" */
	atomic_set(__get_cpu_var(staging).a, i);
	/* allocate another element of a per-CPU array, with result in "i" */
	atomic_set(__get_cpu_var(staging).b, i);
	rcu_assign_pointer(some_global_place, __get_cpu_var(staging));

	If atomic_set() didn't have volatile semantics, then an interrupt
	or NMI handler could see the atomic_set() to .a and .b out of
	order due to compiler optimizations.

Remember, you -did- ask for these!!! ;-)

Quoted 118 lines · Paul E. McKenney

Quoted 22 lines · Chris Snook

Quoted 21 lines · Paul E. McKenney

Quoted 20 lines · Chris Snook

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Chris Snook

If you're depending on volatile writes
being visible to other CPUs, you're screwed either way, because the CPU can hold that data in cache as long as it wants before it writes it to memory. When this finally does happen, it will happen atomically, which is all that atomic_set guarantees. If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first (and a volatile cast in atomic_read will suffice for this) or you have to use LOCK_PREFIX instructions which will invalidate remote cache lines containing the same variable. This patch doesn't change either of these cases.

Quoted 2 lines

The case that it -can- change is interactions with interrupt handlers. And NMI/SMI handlers, for that matter.

You have a point here, but only if you can guarantee that the interrupt handler is running on a processor sharing the cache that has the not-yet-written volatile value. That implies a strictly non-SMP architecture. At the moment, none of those have volatile in their declaration of atomic_t, so this patch can't break any of them.

Quoted 8 lines

Quoted 3 lines

This can also happen when using per-CPU variables. And there are a number of per-CPU variables that are either atomic themselves or are structures containing atomic fields.

Accessing per-CPU variables in this fashion reliably already requires a suitable smp/non-smp read/write memory barrier. I maintain that if we break anything with this change, it was really already broken, if less obviously. Can you give a real or synthetic example of legitimate code that could break?

My main concern is actually the lack of symmetry -- I would expect that an atomic_set() would have the same properties as atomic_read(). It is easy and cheap to provide them with similar properties, so why not? Debugging even a single problem would consume far more time than simply giving them corresponding semantics.

But you asked for examples. These are synthetic, and of course legitimacy is in the eye of the beholder.

1. Watchdog variable.

atomic_t watchdog = ATOMIC_INIT(0);

...

int i;
while (!done) {

/* Do so stuff that doesn't take more than a few us. */ /* Could do atomic increment, but throughput penalty. */

i++;
atomic_set(&watchdog, i);
}
do_something_with(&watchdog);

/* Every so often on some other CPU... */

if ((new_watchdog = atomic_read(&watchdog)) == old_watchdog)
die_horribly();
old_watchdog = new_watchdog;

If atomic_set() did not have volatile semantics, the compiler would be within its rights optimizing it to simply get the final value of "i" after exit from the loop. This would cause the watchdog check to fail spuriously. Memory barriers are not required in this case, because the CPU cannot hang onto the value for very long -- we don't care about the exact value, or about exact synchronization, but rather about whether or not the value is changing.

In this (toy) example, one might replace the atomic_set() with an atomic increment (though that might be too expensive in some cases) or with something like:

atomic_set(&watchdog, atomic_read(&watchdog) + 1);

However, other cases might not permit this transformation, for example, an existing heavily used API might take int rather than atomic_t.

Some will no doubt argue that this example should use a macro or an asm similar to the "forget()" asm put forward elsewhere in this thread.

2. Communicating both with interrupt handler and with other CPUs. For example, data elements that are built up in a location visible to interrupts and NMIs, and then added as a unit to a data structure visible to other CPUs. This more-realistic example is abbreviated to the point of pointlessness as follows:

	struct foo {
		atomic_t a;
		atomic_t b;
	};

	DEFINE_PER_CPU(struct foo *, staging) = NULL;

	/* Create element in staging area. */

	__get_cpu_var(staging) = kzalloc(sizeof(*p), GFP_WHATEVER);
	if (__get_cpu_var(staging) == NULL)
		die_horribly();
	/* allocate an element of some per-CPU array, get the result in "i" 
	*/
	atomic_set(__get_cpu_var(staging).a, i);
	/* allocate another element of a per-CPU array, with result in "i" */
	atomic_set(__get_cpu_var(staging).b, i);
	rcu_assign_pointer(some_global_place, __get_cpu_var(staging));

	If atomic_set() didn't have volatile semantics, then an interrupt
	or NMI handler could see the atomic_set() to .a and .b out of
	order due to compiler optimizations.

Quoted 1 line

Remember, you -did- ask for these!!! ;-)

Ok, I'm convinced. Part of the motivation here is to avoid heisenbugs, so if people expect volatile atomic_set behavior, I'm inclined to give it to them. I don't really feel like indulging the compiler bug paranoiacs, but developer expectations are a legitimate motivation, and a major part of why I posted this in the first place. I'll resubmit the patchset with a volatile cast in atomic_set. Before I do, is there anything *else* that desperately needs such a cast? As far as I can tell, all the other functions are implemented with __asm__ __volatile__, or with spinlocks that use that under the hood.

If you need to guarantee that the value is written to memory at a particular time in your execution sequence, you either have to read it from memory to force the compiler to store it first

No, but I can reason about it and be confident that this won't break anything that isn't already broken. At worst, this patch will make any existing subtly incorrect uses of atomic_t much more obvious and easier to track down.

Quoted 4 lines

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Yes, but we don't write code for these compilers. There are countless pieces of kernel code which would break in this condition, and there doesn't seem to be any interest in fixing this.

Sequence points enforce read-after-write ordering, not write-after-write.

We flush writes with reads for MMIO because of this effect as well as the CPU/bus effects.

Quoted 2 lines

In short, please retain atomic_set()'s volatility, especially on those architectures that declared the atomic_t's counter to be volatile.

Like i386 and x86_64? These used to have volatile in the atomic_t declaration. We removed it, and the sky did not fall.

Quoted 6 lines

Quoted 4 lines

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Yes, but we don't write code for these compilers. There are countless pieces of kernel code which would break in this condition, and there doesn't seem to be any interest in fixing this.

"Other things are broken too". Great argument :-)

Quoted 4 lines

Quoted 2 lines

In short, please retain atomic_set()'s volatility, especially on those architectures that declared the atomic_t's counter to be volatile.

Like i386 and x86_64? These used to have volatile in the atomic_t declaration. We removed it, and the sky did not fall.

And this proves what? Lots of stuff "works" by accident.

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Quoted 5 lines · Paul E. McKenney

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Code all over the kernel assumes that 32-bit reads/writes are atomic so while such a compiler might be legal it certainly can't compile Linux.

Quoted 10 lines · On Friday 10 August 2007 10:21:46 Herbert Xu wrote:

Quoted 5 lines · Paul E. McKenney

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Code all over the kernel assumes that 32-bit reads/writes are atomic so while such a compiler might be legal it certainly can't compile Linux.

Yes, the kernel requirements are much stricter than ISO-C. And besides it is a heavy user of C extensions anyways. On the other hand some of the C99 extensions are not allowed. And then there is sparse, which enforces a language which sometimes is quite far from standard C. You could say it is written in Linux-C, not ISO C.

Quoted 4 lines

The compiler is within its rights to read a 32-bit quantity 16 bits at at time, even on a 32-bit machine. I would be glad to help pummel any compiler writer that pulls such a dirty trick, but the C standard really does permit this.

Code all over the kernel assumes that 32-bit reads/writes are atomic so while such a compiler might be legal it certainly can't compile Linux.

That means GCC cannot compile Linux; it already optimises some accesses to scalars to smaller accesses when it knows it is allowed to. Not often though, since it hardly ever helps in the cost model it employs.

Quoted 4 lines

That means GCC cannot compile Linux; it already optimises some accesses to scalars to smaller accesses when it knows it is allowed to. Not often though, since it hardly ever helps in the cost model it employs.

Please give an example code snippet + gcc version + arch to back this up.

Quoted 5 lines

Quoted 4 lines

That means GCC cannot compile Linux; it already optimises some accesses to scalars to smaller accesses when it knows it is allowed to. Not often though, since it hardly ever helps in the cost model it employs.

Quoted 2 lines

Please give an example code snippet + gcc version + arch to back this up.

	unsigned char f(unsigned long *p)
	{
	        return *p & 1;
	}

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

That means GCC cannot compile Linux; it already optimises some accesses to scalars to smaller accesses when it knows it is allowed to. Not often though, since it hardly ever helps in the cost model it employs.

Please give an example code snippet + gcc version + arch to back this up.

Quoted 4 lines

	unsigned char f(unsigned long *p)
	{
	        return *p & 1;
	}

This doesn't really matter since we only care about the LSB.

Do you have an example where gcc reads it non-atmoically and we care about all parts?

Quoted 5 lines

Quoted 4 lines

That means GCC cannot compile Linux; it already optimises some accesses to scalars to smaller accesses when it knows it is allowed to. Not often though, since it hardly ever helps in the cost model it employs.

Quoted 2 lines

Please give an example code snippet + gcc version + arch to back this up.

	unsigned char f(unsigned long *p)
	{
	        return *p & 1;
	}