make atomic_read() behave consistently across all architectures

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read().  This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

Quoted 7 lines · Chris Snook

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read(). This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

Just an idea: since all architectures already include asm-generic/atomic.h, why not move the definitions of atomic_t and atomic64_t, as well as anything that does not involve architecture specific inline assembly into the generic header?

	Arnd <><

a) chicken and egg: asm-generic/atomic.h depends on definitions in asm/atomic.h

If you can find a way to reshuffle the code and make it simpler, I personally am all for it. I'm skeptical that you'll get much to show for the effort.

b) The definitions aren't precisely identical between all architectures, so it would be a mess of special cases, which gets us right back to where we are now.

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read(). This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

Quoted 6 lines · Chris Snook

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read(). This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

volatile is generally harmful even in atomic_read(). Barriers control visibility and AFAICT things are fine.

But barriers force a flush of *everything* in scope, which we generally don't want. On the other hand, we pretty much always want to flush atomic_* operations. One way or another, we should be restricting the volatile behavior to the thing that needs it. On most architectures, this patch set just moves that from the declaration, where it is considered harmful, to the use, where it is considered an occasional necessary evil.

Quoted 6 lines · Chris Snook

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read(). This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

volatile is generally harmful even in atomic_read(). Barriers control visibility and AFAICT things are fine.

Quoted 10 lines · Christoph Lameter

Quoted 6 lines · Chris Snook

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read(). This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

volatile is generally harmful even in atomic_read(). Barriers control visibility and AFAICT things are fine.

Frankly, I don't see the need for this series myself either. Personal opinion (others may differ), but I consider "volatile" to be a sad / unfortunate wart in C (numerous threads on this list and on the gcc lists/bugzilla over the years stand testimony to this) and if we _can_ steer clear of it, then why not -- why use this ill-defined primitive whose implementation has often differed over compiler versions and platforms? Granted, barrier() _is_ heavy-handed in that it makes the optimizer forget _everything_, but then somebody did post a forget() macro on this thread itself ...

[ BTW, why do we want the compiler to not optimize atomic_read()'s in
the first place? Atomic ops guarantee atomicity, which has nothing to do with "volatility" -- users that expect "volatility" from atomic ops are the ones who must be fixed instead, IMHO. ]

Because atomic operations are generally used for synchronization, which requires volatile behavior. Most such codepaths currently use an inefficient barrier(). Some forget to and we get bugs, because people assume that atomic_read() actually reads something, and atomic_write() actually writes something. Worse, these are architecture-specific, even compiler version-specific bugs that are often difficult to track down.

Because atomic operations are generally used for synchronization, which requires volatile behavior. Most such codepaths currently use an inefficient barrier().
Some forget to and we get bugs, because people assume that atomic_read()
actually reads something, and atomic_write() actually writes something. Worse, these are architecture-specific, even compiler version-specific bugs that are often difficult to track down.

Quoted 7 lines · Chris Snook

Because atomic operations are generally used for synchronization, which requires volatile behavior. Most such codepaths currently use an inefficient barrier().
Some forget to and we get bugs, because people assume that atomic_read()
actually reads something, and atomic_write() actually writes something. Worse, these are architecture-specific, even compiler version-specific bugs that are often difficult to track down.

I'm yet to see a single example from the current tree where this patch series is the correct solution. So far the only example has been a buggy piece of code which has since been fixed with a cpu_relax.

Quoted 9 lines · Herbert Xu

Quoted 7 lines · Chris Snook

Because atomic operations are generally used for synchronization, which requires volatile behavior. Most such codepaths currently use an inefficient barrier().
Some forget to and we get bugs, because people assume that atomic_read()
actually reads something, and atomic_write() actually writes something. Worse, these are architecture-specific, even compiler version-specific bugs that are often difficult to track down.

Quoted 4 lines

I'm yet to see a single example from the current tree where this patch series is the correct solution. So far the only example has been a buggy piece of code which has since been fixed with a cpu_relax.

Btw.: we still have

include/asm-i386/mach-es7000/mach_wakecpu.h: while (!atomic_read(deassert)); include/asm-i386/mach-default/mach_wakecpu.h: while (!atomic_read(deassert));

Looks like they need to be fixed as well.

Quoted 17 lines · Heiko Carstens

Quoted 9 lines · Herbert Xu

Quoted 7 lines · Chris Snook

Because atomic operations are generally used for synchronization, which requires volatile behavior. Most such codepaths currently use an inefficient barrier().
Some forget to and we get bugs, because people assume that atomic_read()
actually reads something, and atomic_write() actually writes something. Worse, these are architecture-specific, even compiler version-specific bugs that are often difficult to track down.

Quoted 4 lines

I'm yet to see a single example from the current tree where this patch series is the correct solution. So far the only example has been a buggy piece of code which has since been fixed with a cpu_relax.

Btw.: we still have

Quoted 4 lines

include/asm-i386/mach-es7000/mach_wakecpu.h: while (!atomic_read(deassert)); include/asm-i386/mach-default/mach_wakecpu.h: while (!atomic_read(deassert));

Looks like they need to be fixed as well.

I don't know if this here is affected:

/* drivers/ieee1394/ieee1394_core.h */
static inline unsigned int get_hpsb_generation(struct hpsb_host *host)

{
	return atomic_read(&host->generation);
}

/* drivers/ieee1394/nodemgr.c */
static int nodemgr_host_thread(void *__hi)

{
	[...]

	for (;;) {
		[... sleep until bus reset event ...]

		/* Pause for 1/4 second in 1/16 second intervals,
		 * to make sure things settle down. */
		g = get_hpsb_generation(host);
		for (i = 0; i < 4 ; i++) {
			if (msleep_interruptible(63) ||
			    kthread_should_stop())
				goto exit;

Quoted 2 lines

I don't know if this here is affected:

Yes, I think it is. You're clearly expecting the read to actually happen when you call get_hpsb_generation(). It's clearly not a busy-loop, so cpu_relax() sounds pointless / wrong solution for this case, so I'm now somewhat beginning to appreciate the motivation behind this series :-)

Quoted 1 line · Satyam Sharma

Quoted 1 line

I don't know if this here is affected:

Nope, we're calling schedule which is a rather heavy-weight barrier.

Quoted 2 lines · Herbert Xu

Quoted 1 line · Satyam Sharma

Quoted 1 line

I don't know if this here is affected:

[...something like]

	b = atomic_read(a);
	for (i = 0; i < 4; i++) {
		msleep_interruptible(63);
		c = atomic_read(a);
		if (c != b) {
			b = c;
			i = 0;
		}
	}

Quoted 2 lines

Nope, we're calling schedule which is a rather heavy-weight barrier.

How does the compiler know that msleep() has got barrier()s?

Quoted 18 lines · Stefan Richter

Quoted 2 lines · Herbert Xu

Quoted 1 line · Satyam Sharma

Quoted 1 line

I don't know if this here is affected:

[...something like]

	b = atomic_read(a);
	for (i = 0; i < 4; i++) {
		msleep_interruptible(63);
		c = atomic_read(a);
		if (c != b) {
			b = c;
			i = 0;
		}
	}

Quoted 2 lines

Nope, we're calling schedule which is a rather heavy-weight barrier.

Quoted 1 line

How does the compiler know that msleep() has got barrier()s?

Because msleep_interruptible() is in a separate compilation unit, the compiler has to assume that it might modify any arbitrary global. In many cases, the compiler also has to assume that msleep_interruptible() might call back into a function in the current compilation unit, thus possibly modifying global static variables.

Quoted 1 line

How does the compiler know that msleep() has got barrier()s?

Because msleep_interruptible() is in a separate compilation unit, the compiler has to assume that it might modify any arbitrary global.

In many cases, the compiler also has to assume that msleep_interruptible() might call back into a function in the current compilation unit, thus possibly modifying global static variables.

Quoted 4 lines

Quoted 1 line

How does the compiler know that msleep() has got barrier()s?

Because msleep_interruptible() is in a separate compilation unit, the compiler has to assume that it might modify any arbitrary global.

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

What you probably mean is that the compiler has to assume any code it cannot currently see can do anything (insofar as allowed by the relevant standards etc.)

Quoted 4 lines

In many cases, the compiler also has to assume that msleep_interruptible()
might call back into a function in the current compilation unit, thus possibly modifying global static variables.

It most often is smart enough to see what compilation-unit-local variables might be modified that way, though :-)

Quoted 5 lines · Segher Boessenkool

Quoted 4 lines

Quoted 1 line

How does the compiler know that msleep() has got barrier()s?

Because msleep_interruptible() is in a separate compilation unit, the compiler has to assume that it might modify any arbitrary global.

Quoted 3 lines

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

Last I checked, the Linux kernel build system did compile each .c file as a separate compilation unit.

Quoted 3 lines

What you probably mean is that the compiler has to assume any code it cannot currently see can do anything (insofar as allowed by the relevant standards etc.)

[ The Cc: list scares me. Should probably trim it. ]

Quoted 7 lines · Segher Boessenkool

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

How does the compiler know that msleep() has got barrier()s?

Because msleep_interruptible() is in a separate compilation unit, the compiler has to assume that it might modify any arbitrary global.

Quoted 10 lines

Quoted 3 lines

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

Last I checked, the Linux kernel build system did compile each .c file as a separate compilation unit.

Quoted 3 lines

What you probably mean is that the compiler has to assume any code it cannot currently see can do anything (insofar as allowed by the relevant standards etc.)

I think this was just terminology confusion here again. Isn't "any code that it cannot currently see" the same as "another compilation unit", and wouldn't the "compilation unit" itself expand if we ask gcc to compile more than one unit at once? Or is there some more specific "definition" for "compilation unit" (in gcc lingo, possibly?)

Quoted 5 lines

I think this was just terminology confusion here again. Isn't "any code that it cannot currently see" the same as "another compilation unit", and wouldn't the "compilation unit" itself expand if we ask gcc to compile more than one unit at once? Or is there some more specific "definition" for "compilation unit" (in gcc lingo, possibly?)

This is indeed my understanding -- "compilation unit" is whatever the compiler looks at in one go. I have heard the word "module" used for the minimal compilation unit covering a single .c file and everything that it #includes, but there might be a better name for this.

Quoted 11 lines

Quoted 6 lines

I think this was just terminology confusion here again. Isn't "any code
that it cannot currently see" the same as "another compilation unit", and wouldn't the "compilation unit" itself expand if we ask gcc to compile more than one unit at once? Or is there some more specific "definition" for "compilation unit" (in gcc lingo, possibly?)

This is indeed my understanding -- "compilation unit" is whatever the compiler looks at in one go. I have heard the word "module" used for the minimal compilation unit covering a single .c file and everything that it #includes, but there might be a better name for this.

Yes, that's what's called "compilation unit" :-)

[/me double checks]

Erm, the C standard actually calls it "translation unit".

To be exact, to avoid any more confusion:

5.1.1.1/1:
A C program need not all be translated at the same time. The text of the program is kept in units called source files, (or preprocessing files) in this International Standard. A source file together with all the headers and source files included via the preprocessing directive #include is known as a preprocessing translation unit. After preprocessing, a preprocessing translation unit is called a translation unit.

Quoted 3 lines

Quoted 3 lines

What you probably mean is that the compiler has to assume any code it cannot currently see can do anything (insofar as allowed by the relevant standards etc.)

I think this was just terminology confusion here again. Isn't "any code that it cannot currently see" the same as "another compilation unit",

and wouldn't the "compilation unit" itself expand if we ask gcc to compile more than one unit at once? Or is there some more specific "definition" for "compilation unit" (in gcc lingo, possibly?)

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

What you probably mean is that the compiler has to assume any code it cannot currently see can do anything (insofar as allowed by the relevant standards etc.)

I think this was just terminology confusion here again. Isn't "any code that it cannot currently see" the same as "another compilation unit",

It is not; try gcc -combine or the upcoming link-time optimisation stuff, for example.

Quoted 3 lines

and wouldn't the "compilation unit" itself expand if we ask gcc to compile more than one unit at once? Or is there some more specific "definition" for "compilation unit" (in gcc lingo, possibly?)

"compilation unit" is a C standard term. It typically boils down to "single .c file".

that are:

	while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
		mdelay(1);
		msecs--;
	}

where mdelay() becomes __const_udelay() which happens to be in another translation unit (arch/i386/lib/delay.c) and hence saves this callsite from being a bug :-)

Quoted 8 lines · Satyam Sharma

that are:

	while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
		mdelay(1);
		msecs--;
	}

Quoted 3 lines

where mdelay() becomes __const_udelay() which happens to be in another translation unit (arch/i386/lib/delay.c) and hence saves this callsite from being a bug :-)

The udelay itself certainly should have some form of cpu_relax in it.

Quoted 2 lines

The udelay itself certainly should have some form of cpu_relax in it.

Yes, a form of barrier() must be present in mdelay() or udelay() itself as you say, having it in __const_udelay() is *not* enough (superflous actually, considering it is already a separate translation unit and invisible to the compiler).

Quoted 7 lines · Satyam Sharma

Quoted 2 lines

The udelay itself certainly should have some form of cpu_relax in it.

Yes, a form of barrier() must be present in mdelay() or udelay() itself as you say, having it in __const_udelay() is *not* enough (superflous actually, considering it is already a separate translation unit and invisible to the compiler).

As long as __const_udelay does something which has the same effect as barrier it is enough even if it's in the same unit.

As a matter of fact it does on i386 where __delay either uses rep_nop or asm/volatile.

Quoted 2 lines

"compilation unit" is a C standard term. It typically boils down to "single .c file".

As you mentioned later, "single .c file with all the other files (headers or other .c files) that it pulls in via #include" is actually "translation unit", both in the C standard as well as gcc docs.

"Compilation unit"
doesn't seem to be nearly as standard a term, though in most places it is indeed meant to be same as "translation unit", but with the new gcc inter-module-analysis stuff that you referred to above, I suspect one may reasonably want to call a "compilation unit" as all that the compiler sees at a given instant.

Quoted 3 lines

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

Last I checked, the Linux kernel build system did compile each .c file as a separate compilation unit.

Quoted 7 lines

Quoted 4 lines

In many cases, the compiler also has to assume that msleep_interruptible()
might call back into a function in the current compilation unit, thus possibly modifying global static variables.

It most often is smart enough to see what compilation-unit-local variables might be modified that way, though :-)

Yep. For example, if it knows the current value of a given such local variable, and if all code paths that would change some other variable cannot be reached given that current value of the first variable.

At least given that gcc doesn't know about multiple threads of execution!

Quoted 4 lines

Quoted 3 lines

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

Quoted 2 lines

Last I checked, the Linux kernel build system did compile each .c file as a separate compilation unit.

I have some patches to use -combine -fwhole-program for Linux. Highly experimental, you need a patched bleeding edge toolchain. If there's interest I'll clean it up and put it online.

David Woodhouse had some similar patches about a year ago.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

In many cases, the compiler also has to assume that msleep_interruptible()
might call back into a function in the current compilation unit, thus possibly modifying global static variables.

It most often is smart enough to see what compilation-unit-local variables might be modified that way, though :-)

Quoted 3 lines

Yep. For example, if it knows the current value of a given such local variable, and if all code paths that would change some other variable cannot be reached given that current value of the first variable.

Or the most common thing: if neither the address of the translation- unit local variable nor the address of any function writing to that variable can "escape" from that translation unit, nothing outside the translation unit can write to the variable.

Quoted 2 lines

At least given that gcc doesn't know about multiple threads of execution!

Heh, only about the threads it creates itself (not relevant to the kernel, for sure :-) )

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

Last I checked, the Linux kernel build system did compile each .c file as a separate compilation unit.

Quoted 5 lines

I have some patches to use -combine -fwhole-program for Linux. Highly experimental, you need a patched bleeding edge toolchain. If there's interest I'll clean it up and put it online.

David Woodhouse had some similar patches about a year ago.

Sounds exciting... ;-)

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

In many cases, the compiler also has to assume that msleep_interruptible()
might call back into a function in the current compilation unit, thus possibly modifying global static variables.

It most often is smart enough to see what compilation-unit-local variables might be modified that way, though :-)

Yep. For example, if it knows the current value of a given such local variable, and if all code paths that would change some other variable cannot be reached given that current value of the first variable.

Quoted 4 lines

Or the most common thing: if neither the address of the translation- unit local variable nor the address of any function writing to that variable can "escape" from that translation unit, nothing outside the translation unit can write to the variable.

But there is usually at least one externally callable function in a .c file.

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

Quoted 3 lines

No; compilation units have nothing to do with it, GCC can optimise across compilation unit boundaries just fine, if you tell it to compile more than one compilation unit at once.

Last I checked, the Linux kernel build system did compile each .c file
as a separate compilation unit.

I have some patches to use -combine -fwhole-program for Linux. Highly experimental, you need a patched bleeding edge toolchain. If there's interest I'll clean it up and put it online.

Quoted 3 lines

Quoted 1 line

David Woodhouse had some similar patches about a year ago.

Sounds exciting... ;-)

Yeah, the breakage is *quite* spectacular :-)

Quoted 19 lines

Quoted 18 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

In many cases, the compiler also has to assume that msleep_interruptible()
might call back into a function in the current compilation unit, thus
possibly modifying global static variables.

Quoted 7 lines

Quoted 2 lines

It most often is smart enough to see what compilation-unit-local variables might be modified that way, though :-)

Yep. For example, if it knows the current value of a given such local
variable, and if all code paths that would change some other variable cannot be reached given that current value of the first variable.

Or the most common thing: if neither the address of the translation- unit local variable nor the address of any function writing to that variable can "escape" from that translation unit, nothing outside the translation unit can write to the variable.

Quoted 2 lines

But there is usually at least one externally callable function in a .c file.

Of course, but often none of those will (indirectly) write a certain static variable.

[PATCH] ieee1394: Fix kthread stopping in nodemgr_host_thread

The nodemgr host thread can exit on its own even when kthread_should_stop is not true, on receiving a signal (might never happen in practice, as it ignores signals). But considering kthread_stop() must not be mixed with kthreads that can exit on their own, I think changing the code like this is clearer. This change means the thread can cut its sleep short when receive a signal but looking at the code around, that sounds okay (and again, it might never actually recieve a signal in practice).

[...]
Btw.: we still have

include/asm-i386/mach-es7000/mach_wakecpu.h: while (!atomic_read(deassert)); include/asm-i386/mach-default/mach_wakecpu.h: while (!atomic_read(deassert));

Looks like they need to be fixed as well.

include/asm-i386/mach-default/mach_wakecpu.h line 21

Quoted 8 lines · Satyam Sharma

static inline void wait_for_init_deassert(atomic_t *deassert) {

include/asm-i386/mach-default/mach_wakecpu.h +2 -1

-	while (!atomic_read(deassert));
+	while (!atomic_read(deassert))
+		cpu_relax();
 	return;
 }

For less-than-briliant people like me, it's totally non-obvious that cpu_relax() is needed for correctness here, not just to make P4 happy.

IOW: "atomic_read" name quite unambiguously means "I will read this variable from main memory". Which is not true and creates potential for confusion and bugs.

include/asm-i386/mach-default/mach_wakecpu.h line 21

Quoted 8 lines

static inline void wait_for_init_deassert(atomic_t *deassert) {

include/asm-i386/mach-default/mach_wakecpu.h +2 -1

-	while (!atomic_read(deassert));
+	while (!atomic_read(deassert))
+		cpu_relax();
 	return;
 }

For less-than-briliant people like me, it's totally non-obvious that cpu_relax() is needed for correctness here, not just to make P4 happy.

IOW: "atomic_read" name quite unambiguously means "I will read this variable from main memory". Which is not true and creates potential for confusion and bugs.

Quoted 12 lines · Satyam Sharma

include/asm-i386/mach-default/mach_wakecpu.h line 21

Quoted 7 lines

static inline void wait_for_init_deassert(atomic_t *deassert) {

include/asm-i386/mach-default/mach_wakecpu.h +2 -1

-	while (!atomic_read(deassert));
+	while (!atomic_read(deassert))
+		cpu_relax();
 	return;
 }

For less-than-briliant people like me, it's totally non-obvious that cpu_relax() is needed for correctness here, not just to make P4 happy.

Quoted 3 lines

IOW: "atomic_read" name quite unambiguously means "I will read this variable from main memory". Which is not true and creates potential for confusion and bugs.

To me, "atomic_read" means a read which is synchronized with other changes to the variable (using the atomic_XXX functions) in such a way that I will always only see the "before" or "after" state of the variable - never an intermediate state while a modification is happening. It doesn't imply that I have to see the "after" state immediately after another thread modifies it.

So you are ok with compiler propagating n1 to n2 here:

n1 += atomic_read(x);
other_variable++;
n2 += atomic_read(x);

without accessing x second time. What's the point? Any sane coder will say that explicitly anyway:

include/asm-i386/mach-default/mach_wakecpu.h line 21

Quoted 8 lines · Satyam Sharma

static inline void wait_for_init_deassert(atomic_t *deassert) {

include/asm-i386/mach-default/mach_wakecpu.h +2 -1

-	while (!atomic_read(deassert));
+	while (!atomic_read(deassert))
+		cpu_relax();
 	return;
 }

For less-than-briliant people like me, it's totally non-obvious that cpu_relax() is needed for correctness here, not just to make P4 happy.

IOW: "atomic_read" name quite unambiguously means "I will read this variable from main memory". Which is not true and creates potential for confusion and bugs.

But if people do seem to have a mixed / confused notion of atomicity and barriers, and if there's consensus, then as I'd said earlier, I have no issues in going with the consensus (eg. having API variants). Linus would be more difficult to convince, however, I suspect :-)

Quoted 4 lines · Satyam Sharma

But if people do seem to have a mixed / confused notion of atomicity and barriers, and if there's consensus, then as I'd said earlier, I have no issues in going with the consensus (eg. having API variants). Linus would be more difficult to convince, however, I suspect :-)

The confusion may be the result of us having barrier semantics in atomic_read. If we take that out then we may avoid future confusions.

Quoted 7 lines · Christoph Lameter

Quoted 4 lines · Satyam Sharma

But if people do seem to have a mixed / confused notion of atomicity and barriers, and if there's consensus, then as I'd said earlier, I have no issues in going with the consensus (eg. having API variants). Linus would be more difficult to convince, however, I suspect :-)

The confusion may be the result of us having barrier semantics in atomic_read. If we take that out then we may avoid future confusions.

I think better name may help. Nuke atomic_read() altogether.

n = atomic_value(x); // doesnt hint as strongly at reading as "atomic_read"
n = atomic_fetch(x); // yes, we _do_ touch RAM
n = atomic_read_uncached(x); // or this

How does that sound?

include/asm-i386/mach-default/mach_wakecpu.h line 21

Quoted 7 lines

static inline void wait_for_init_deassert(atomic_t *deassert) {

include/asm-i386/mach-default/mach_wakecpu.h +2 -1

-	while (!atomic_read(deassert));
+	while (!atomic_read(deassert))
+		cpu_relax();
 	return;
 }

For less-than-briliant people like me, it's totally non-obvious that cpu_relax() is needed for correctness here, not just to make P4 happy.

Quoted 3 lines · Chris Snook

Because atomic operations are generally used for synchronization, which requires volatile behavior. Most such codepaths currently use an inefficient barrier().
Some forget to and we get bugs, because people assume that atomic_read()
actually reads something, and atomic_write() actually writes something. Worse, these are architecture-specific, even compiler version-specific bugs that are often difficult to track down.

I'm yet to see a single example from the current tree where this patch series is the correct solution. So far the only example has been a buggy piece of code which has since been fixed with a cpu_relax.

Part of the motivation here is to fix heisenbugs. If I knew where they

Quoted 2 lines · Chris Snook

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Quoted 4 lines

Quoted 1 line

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Precisely the point -- use of volatile (whether in casts or on asms) in these cases are intended to disable those optimizations likely to result in heisenbugs.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Precisely the point -- use of volatile (whether in casts or on asms) in these cases are intended to disable those optimizations likely to result in heisenbugs.

The only thing volatile on an asm does is create a side effect on the asm statement; in effect, it tells the compiler "do not remove this asm even if you don't need any of its outputs".

It's not disabling optimisation likely to result in bugs, heisen- or otherwise; _not_ putting the volatile on an asm that needs it simply _is_ a bug :-)

Quoted 7 lines

The only thing volatile on an asm does is create a side effect on the asm statement; in effect, it tells the compiler "do not remove this asm even if you don't need any of its outputs".

It's not disabling optimisation likely to result in bugs, heisen- or otherwise; _not_ putting the volatile on an asm that needs it simply _is_ a bug :-)

Yep. And the reason it is a bug is that it fails to disable the relevant compiler optimizations. So I suspect that we might actually be saying the same thing here.

Quoted 1 line

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Quoted 5 lines

Quoted 1 line

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Almost everything is a tradeoff; and so is this. I don't believe most people would find disabling all compiler optimisations an acceptable price to pay for some peace of mind.

Quoted 9 lines

Quoted 5 lines

Quoted 1 line

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Almost everything is a tradeoff; and so is this. I don't believe most people would find disabling all compiler optimisations an acceptable price to pay for some peace of mind.

So why is this a good tradeoff?

I also think that just adding things to APIs in the hope it might fix up some bugs isn't really a good road to go down. Where do you stop?

Quoted 13 lines

Quoted 11 lines

Quoted 6 lines

Quoted 1 line

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Almost everything is a tradeoff; and so is this. I don't believe most people would find disabling all compiler optimisations an acceptable price to pay for some peace of mind.

Quoted 1 line

So why is this a good tradeoff?

It certainly is better than disabling all compiler optimisations!

Quoted 2 lines

I also think that just adding things to APIs in the hope it might fix up some bugs isn't really a good road to go down. Where do you stop?

I look at it the other way: keeping the "volatile" semantics in atomic_XXX() (or adding them to it, whatever) helps _prevent_ bugs;

certainly most people expect that behaviour, and also that behaviour is *needed* in some places and no other interface provides that functionality.

[some confusion about barriers wrt atomics snipped]

So i386 and x86-64 don't have volatiles there, and it saves them a few K of kernel text. What you need to justify is why it is a good

Quoted 2 lines · Nick Piggin

So i386 and x86-64 don't have volatiles there, and it saves them a few K of kernel text. What you need to justify is why it is a good

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

Paul.

I'm surprised too. Numbers were from the "...use asm() like the other atomic operations already do" thread. According to them,

text data bss dec hex filename

3434150  249176  176128 3859454  3ae3fe atomic_normal/vmlinux
3436203  249176  176128 3861507  3aec03 atomic_volatile/vmlinux

The first one is a stock kenel, the second is with atomic_read/set cast to volatile. gcc-4.1 -- maybe if you have an earlier gcc it won't optimise as much?

Just try it yourself:

	volatile int i;
	int j;

	int testme(void)
	{
	        return i <= 1;
	}

	int testme2(void)
	{
	        return j <= 1;
	}

and compile with all the optimizations you can.

I get:

testme:
movl i(%rip), %eax
subl $1, %eax
setle %al
movzbl %al, %eax
ret

vs

	testme2:
	        xorl    %eax, %eax
	        cmpl    $1, j(%rip)
	        setle   %al
	        ret

(now, whether that "xorl + setle" is better than "setle + movzbl", I don't really know - maybe it is. But that's not thepoint. The point is the difference between

movl i(%rip), %eax
subl $1, %eax

and

                cmpl    $1, j(%rip)

gcc bugzilla bug #33102, for whatever that ends up being worth. ;-)

Quoted 2 lines · Paul E. McKenney

gcc bugzilla bug #33102, for whatever that ends up being worth. ;-)

I had totally forgotten that I'd already filed that bug more than six years ago until they just closed yours as a duplicate of mine :)

Good luck in getting it fixed!

Quoted 8 lines · Herbert Xu

Quoted 2 lines · Paul E. McKenney

gcc bugzilla bug #33102, for whatever that ends up being worth. ;-)

I had totally forgotten that I'd already filed that bug more than six years ago until they just closed yours as a duplicate of mine :)

Quoted 1 line

Good luck in getting it fixed!

Well, just got done re-opening it for the third time. And a local gcc community member advised me not to give up too easily. But I must admit that I am impressed with the speed that it was identified as duplicate.

Should be entertaining! ;-)

Quoted 16 lines · Paul E. McKenney

Quoted 9 lines · Herbert Xu

Quoted 2 lines · Paul E. McKenney

gcc bugzilla bug #33102, for whatever that ends up being worth. ;-)

I had totally forgotten that I'd already filed that bug more than six years ago until they just closed yours as a duplicate of mine :)

Good luck in getting it fixed!

Well, just got done re-opening it for the third time. And a local gcc community member advised me not to give up too easily. But I must admit that I am impressed with the speed that it was identified as duplicate.

Quoted 1 line

Should be entertaining! ;-)

Right. ROTFL... volatile actually breaks atomic_t instead of making it safe. x++ becomes a register load, increment and a register store. Without volatile we can increment the memory directly.

No code does (or would do, or should do):

	x.counter++;

on an "atomic_t x;" anyway.

Quoted 6 lines · Satyam Sharma

No code does (or would do, or should do):

	x.counter++;

on an "atomic_t x;" anyway.

That's just an example of a general problem.

No, you don't use "x.counter++". But you *do* use

	if (atomic_read(&x) <= 1)

and loading into a register is stupid and pointless, when you could just do it as a regular memory-operand to the cmp instruction.

And as far as the compiler is concerned, the problem is the 100% same:
combining operations with the volatile memop.

The fact is, a compiler that thinks that

movl mem,reg
cmpl $val,reg

is any better than

cmpl $val,mem

is just not a very good compiler.

Quoted 6 lines · Satyam Sharma

No code does (or would do, or should do):

	x.counter++;

on an "atomic_t x;" anyway.

That's just an example of a general problem.

No, you don't use "x.counter++". But you *do* use

	if (atomic_read(&x) <= 1)

and loading into a register is stupid and pointless, when you could just do it as a regular memory-operand to the cmp instruction.

And as far as the compiler is concerned, the problem is the 100% same:
combining operations with the volatile memop.

The fact is, a compiler that thinks that

movl mem,reg
cmpl $val,reg

is any better than

cmpl $val,mem

is just not a very good compiler. But when talking about "volatile", that's exactly what ytou always get (and always have gotten - this is not a regression, and I doubt gcc is alone in this).

One of the gcc guys claimed that he thought that the two-instruction sequence would be faster on some x86 machines. I pointed out that there might be a concern about code size. I chose not to point out that people might also care about the other x86 machines. ;-)

Quoted 5 lines · Paul E. McKenney

One of the gcc guys claimed that he thought that the two-instruction sequence would be faster on some x86 machines. I pointed out that there might be a concern about code size. I chose not to point out that people might also care about the other x86 machines. ;-)

Some (very few) x86 uarchs do tend to prefer "load-store" like code generation, and doing a "mov [mem],reg + op reg" instead of "op [mem]" can actually be faster on some of them. Not any that are relevant today, though.

Also, that has nothing to do with volatile, and should be controlled by optimization flags (like -mtune). In fact, I thought there was a separate flag to do that (ie something like "-mload-store"), but I can't find it, so maybe that's just my fevered brain..

Quoted 5 lines · Satyam Sharma

No code does (or would do, or should do):

	x.counter++;

on an "atomic_t x;" anyway.

That's just an example of a general problem.

No, you don't use "x.counter++". But you *do* use

	if (atomic_read(&x) <= 1)

and loading into a register is stupid and pointless, when you could just do it as a regular memory-operand to the cmp instruction.

And as far as the compiler is concerned, the problem is the 100% same:
combining operations with the volatile memop.

The fact is, a compiler that thinks that

movl mem,reg
cmpl $val,reg

is any better than

cmpl $val,mem

is just not a very good compiler.

Quoted 7 lines

No, you don't use "x.counter++". But you *do* use

	if (atomic_read(&x) <= 1)

and loading into a register is stupid and pointless, when you could just do it as a regular memory-operand to the cmp instruction.

It doesn't mean that (volatile int*) cast is bad, it means that current gcc is bad (or "not good enough"). IOW: instead of avoiding volatile cast, it's better to fix the compiler.

Linus, in all honesty gcc has many more cases of suboptimal code, case of "volatile" is just one of many.

Off the top of my head:

Quoted 9 lines · Paul E. McKenney

Quoted 8 lines · Herbert Xu

Quoted 2 lines · Paul E. McKenney

gcc bugzilla bug #33102, for whatever that ends up being worth. ;-)

I had totally forgotten that I'd already filed that bug more than six years ago until they just closed yours as a duplicate of mine :)

Quoted 8 lines

Quoted 1 line

Good luck in getting it fixed!

Well, just got done re-opening it for the third time. And a local gcc community member advised me not to give up too easily. But I must admit that I am impressed with the speed that it was identified as duplicate.

Should be entertaining! ;-)

Right. ROTFL... volatile actually breaks atomic_t instead of making it safe. x++ becomes a register load, increment and a register store. Without volatile we can increment the memory directly. It seems that volatile requires that the variable is loaded into a register first and then operated upon. Understandable when you think about volatile being used to access memory mapped I/O registers where a RMW operation could be problematic.

See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=3506

Quoted 8 lines · Paul E. McKenney

Quoted 7 lines · Herbert Xu

Quoted 1 line · Paul E. McKenney

gcc bugzilla bug #33102, for whatever that ends up being worth. ;-)

I had totally forgotten that I'd already filed that bug more than six years ago until they just closed yours as a duplicate of mine :)

Good luck in getting it fixed!

Quoted 6 lines

Well, just got done re-opening it for the third time. And a local gcc community member advised me not to give up too easily. But I must admit that I am impressed with the speed that it was identified as duplicate.

Should be entertaining! ;-)

Right. ROTFL... volatile actually breaks atomic_t instead of making it safe. x++ becomes a register load, increment and a register store. Without volatile we can increment the memory directly. It seems that volatile requires that the variable is loaded into a register first and then operated upon. Understandable when you think about volatile being used to access memory mapped I/O registers where a RMW operation could be problematic.

Quoted 1 line

Right. ROTFL... volatile actually breaks atomic_t instead of making it safe. x++ becomes a register load, increment and a register store. Without volatile we can increment the memory directly. It seems that volatile requires that the variable is loaded into a register first and then operated upon. Understandable when you think about volatile being used to access memory mapped I/O registers where a RMW operation could be problematic.

So, if we want consistent behavior, we're pretty much screwed unless we use inline assembler everywhere?

And no, RMW on MMIO isn't "problematic" at all, either.

An RMW op is a read op, a modify op, and a write op, all rolled into one opcode. But three actual operations.

Quoted 4 lines

And no, RMW on MMIO isn't "problematic" at all, either.

An RMW op is a read op, a modify op, and a write op, all rolled into one opcode. But three actual operations.

Maybe for some CPUs, but not all. ARM for instance can't use the load exclusive and store exclusive instructions to MMIO space.

Quoted 7 lines

Quoted 4 lines

And no, RMW on MMIO isn't "problematic" at all, either.

An RMW op is a read op, a modify op, and a write op, all rolled into one opcode. But three actual operations.

Maybe for some CPUs, but not all. ARM for instance can't use the load exclusive and store exclusive instructions to MMIO space.

Sure, your CPU doesn't have RMW instructions -- how to emulate those if you don't have them is a totally different thing.

Quoted 8 lines · Segher Boessenkool

Quoted 7 lines

Quoted 4 lines

And no, RMW on MMIO isn't "problematic" at all, either.

An RMW op is a read op, a modify op, and a write op, all rolled into one opcode. But three actual operations.

Maybe for some CPUs, but not all. ARM for instance can't use the load exclusive and store exclusive instructions to MMIO space.

Quoted 2 lines

Sure, your CPU doesn't have RMW instructions -- how to emulate those if you don't have them is a totally different thing.

I thought that ARM's load exclusive and store exclusive instructions were its equivalent of LL and SC, which RISC machines typically use to build atomic sequences of instructions -- and which normally cannot be applied to MMIO space.

Quoted 7 lines

Quoted 4 lines

And no, RMW on MMIO isn't "problematic" at all, either.

An RMW op is a read op, a modify op, and a write op, all rolled into one opcode. But three actual operations.

Maybe for some CPUs, but not all. ARM for instance can't use the load exclusive and store exclusive instructions to MMIO space.

Sure, your CPU doesn't have RMW instructions -- how to emulate those if you don't have them is a totally different thing.

Let me say it more clearly: On ARM, it is impossible to perform atomic operations on MMIO space.

So the whole discussion is irrelevant to ARM, PowerPC and any other architecture except x86[-64].

Quoted 2 lines

Let me say it more clearly: On ARM, it is impossible to perform atomic operations on MMIO space.

Actually, no one is suggesting that we try to do that at all.

The discussion about RMW ops on MMIO space started with a comment attributed to the gcc developers that one reason why gcc on x86 doesn't use instructions that do RMW ops on volatile variables is that volatile is used to mark MMIO addresses, and there was some uncertainty about whether (non-atomic) RMW ops on x86 could be used on MMIO. This is in regard to the question about why gcc on x86 always moves a volatile variable into a register before doing anything to it.

So the whole discussion is irrelevant to ARM, PowerPC and any other architecture except x86[-64].

Quoted 5 lines

Quoted 2 lines

Let me say it more clearly: On ARM, it is impossible to perform atomic operations on MMIO space.

Actually, no one is suggesting that we try to do that at all.

Quoted 7 lines

The discussion about RMW ops on MMIO space started with a comment attributed to the gcc developers that one reason why gcc on x86 doesn't use instructions that do RMW ops on volatile variables is that volatile is used to mark MMIO addresses, and there was some uncertainty about whether (non-atomic) RMW ops on x86 could be used on MMIO. This is in regard to the question about why gcc on x86 always moves a volatile variable into a register before doing anything to it.

This question is GCC PR33102, which was incorrectly closed as a duplicate
of PR3506 -- and *that* PR was closed because its reporter seemed to claim the GCC generated code for an increment on a volatile (namely, three
machine instructions: load, modify, store) was incorrect, and it has to be one machine instruction.

Quoted 2 lines

So the whole discussion is irrelevant to ARM, PowerPC and any other architecture except x86[-64].

And even there, it's not something the kernel can take advantage of before GCC 4.4 is in widespread use, if then. Let's move on.

I agree that instant gratification is hard to come by when synching up compiler and kernel versions. Nonetheless, it should be possible to create APIs that are are conditioned on the compiler version.

Quoted 3 lines · Paul E. McKenney

I agree that instant gratification is hard to come by when synching up compiler and kernel versions. Nonetheless, it should be possible to create APIs that are are conditioned on the compiler version.

We've tried that, sort of. See the mess surrounding the whole extern/static/inline/__whatever boondogle, which seems to have changed semantics in every single gcc release since 2.95 or so.

And recently mention was made that gcc4.4 will have *new* semantics in this area. Yee. Hah.

Quoted 3 lines · Paul E. McKenney

I agree that instant gratification is hard to come by when synching up compiler and kernel versions. Nonetheless, it should be possible to create APIs that are are conditioned on the compiler version.

We've tried that, sort of. See the mess surrounding the whole extern/static/inline/__whatever boondogle, which seems to have changed semantics in every single gcc release since 2.95 or so.
...

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

And no, RMW on MMIO isn't "problematic" at all, either.

An RMW op is a read op, a modify op, and a write op, all rolled into one opcode. But three actual operations.

Quoted 5 lines

Quoted 2 lines

Maybe for some CPUs, but not all. ARM for instance can't use the load exclusive and store exclusive instructions to MMIO space.

Sure, your CPU doesn't have RMW instructions -- how to emulate those if you don't have them is a totally different thing.

Let me say it more clearly: On ARM, it is impossible to perform atomic operations on MMIO space.

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

Quoted 3 lines · Paul Mackerras

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

One of the things that "volatile" generally screws up is a simple

	volatile int i;

	i++;

which a compiler will generally get horribly, horribly wrong.

In a reasonable world, gcc should just make that be (on x86)

	addl $1,i(%rip)

on x86-64, which is indeed what it does without the volatile. But with the volatile, the compiler gets really nervous, and doesn't dare do it in one instruction, and thus generates crap like

        movl    i(%rip), %eax
        addl    $1, %eax
        movl    %eax, i(%rip)

instead. For no good reason, except that "volatile" just doesn't have any good/clear semantics for the compiler, so most compilers will just make it be "I will not touch this access in any way, shape, or form". Including even trivially correct instruction optimization/combination.

This is one of the reasons why we should never use "volatile". It pessimises code generation for no good reason - just because compilers don't know what the heck it even means!

Now, people don't do "i++" on atomics (you'd use "atomic_inc()" for that), but people *do* do things like

	if (atomic_read(..) <= 1)
		..

On ppc, things like that probably don't much matter. But on x86, it makes a *huge* difference whether you do

	movl i(%rip),%eax
	cmpl $1,%eax

or if you can just use the value directly for the operation, like this:

	cmpl $1,i(%rip)

which is again a totally obvious and totally safe optimization, but is (again) something that gcc doesn't dare do, since "i" is volatile.

In other words: "volatile" is a horribly horribly bad way of doing things, because it generates *worse*code*, for no good reason. You just don't see it on powerpc, because it's already a load-store architecture, so there is no "good code" for doing direct-to-memory operations.

Linus
-
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

Quoted 3 lines · Paul Mackerras

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

One of the things that "volatile" generally screws up is a simple

	volatile int i;

	i++;

which a compiler will generally get horribly, horribly wrong.

[...] For no good reason, except that "volatile" just doesn't have any good/clear semantics for the compiler, so most compilers will just make it be "I will not touch this access in any way, shape, or form". Including even trivially correct instruction optimization/combination.

This is one of the reasons why we should never use "volatile". It pessimises code generation for no good reason - just because compilers don't know what the heck it even means!
[...]
In other words: "volatile" is a horribly horribly bad way of doing things, because it generates *worse*code*, for no good reason. You just don't see it on powerpc, because it's already a load-store architecture, so there is no "good code" for doing direct-to-memory operations.

#define atomic_read_volatile(v)				\
	({						\
		forget((v)->counter);			\
		((v)->counter);				\
	})

where:

#define forget(a) __asm__ __volatile__ ("" :"=m" (a) :"m" (a))

Quoted 7 lines · Satyam Sharma

#define atomic_read_volatile(v)				\
	({						\
		forget((v)->counter);			\
		((v)->counter);				\
	})

where:

*vomit* :)

Not only do I hate the keyword volatile, but the barrier is only a one-sided affair so its probable this is going to have slightly different allowed reorderings than a real volatile access.

Also, why would you want to make these insane accessors for atomic_t types? Just make sure everybody knows the basics of barriers, and they can apply that knowledge to atomic_t and all other lockless memory accesses as well.

Quoted 1 line

#define forget(a) __asm__ __volatile__ ("" :"=m" (a) :"m" (a))

I like order(x) better, but it's not the most perfect name either.

Quoted 4 lines · Nick Piggin

Also, why would you want to make these insane accessors for atomic_t types? Just make sure everybody knows the basics of barriers, and they can apply that knowledge to atomic_t and all other lockless memory accesses as well.

Code that looks like:

	while (!atomic_read(&v)) {
		...
		cpu_relax_no_barrier();
		forget(v.counter);
		        ^^^^^^^^
	}

would be uglier. Also think about code such as:

	a = atomic_read();
	if (!a)
		do_something();

	forget();
	a = atomic_read();
	... /* some code that depends on value of a, obviously */

	forget();
	a = atomic_read();
	...

So much explicit sprinkling of "forget()" looks ugly.

on the other hand, looks neater. The "_volatile()" suffix makes it also no less explicit than an explicit barrier-like macro that this primitive is something "special", for code clarity purposes.

Quoted 18 lines · Satyam Sharma

Quoted 4 lines · Nick Piggin

Also, why would you want to make these insane accessors for atomic_t types? Just make sure everybody knows the basics of barriers, and they can apply that knowledge to atomic_t and all other lockless memory accesses as well.

Code that looks like:

	while (!atomic_read(&v)) {
		...
		cpu_relax_no_barrier();
		forget(v.counter);
		        ^^^^^^^^
	}

Quoted 1 line

would be uglier. Also think about code such as:

I think they would both be equally ugly,

but the atomic_read_volatile
variant would be more prone to subtle bugs because of the weird implementation.

And it would be more ugly than introducing an order(x) statement for all memory operations, and adding an order_atomic() wrapper for it for atomic types.

Quoted 12 lines

	a = atomic_read();
	if (!a)
		do_something();

	forget();
	a = atomic_read();
	... /* some code that depends on value of a, obviously */

	forget();
	a = atomic_read();
	...

Quoted 1 line

So much explicit sprinkling of "forget()" looks ugly.

Firstly, why is it ugly? It's nice because of those nice explicit statements there that give us a good heads up and would have some comments attached to them

(also, lack of the word "volatile" is always a plus).

Secondly, what sort of code would do such a thing?

Quoted 3 lines

on the other hand, looks neater. The "_volatile()" suffix makes it also no less explicit than an explicit barrier-like macro that this primitive is something "special", for code clarity purposes.

Just don't use the word volatile,

and have barriers both before and after the memory operation,

[...] I don't see
the point though, when you could just have a single barrier(x) barrier function defined for all memory locations,

rather than
this odd thing that only works for atomics

(and would have to
be duplicated for atomic_set.

Quoted 2 lines · Nick Piggin

I think they would both be equally ugly,

You think both these are equivalent in terms of "looks":

					|
while (!atomic_read(&v)) {		|	while (!atomic_read_xxx(&v)) {
	...				|		...
	cpu_relax_no_barrier();		|		cpu_relax_no_barrier();
	order_atomic(&v);		|	}
}					|

(where order_atomic() is an atomic_t
specific wrapper as you mentioned below)

?

Quoted 4 lines

but the atomic_read_volatile
variant would be more prone to subtle bugs because of the weird implementation.

What bugs?

Quoted 2 lines

Secondly, what sort of code would do such a thing?

See the nodemgr_host_thread() that does something similar, though not exactly same.

Quoted 2 lines

and have barriers both before and after the memory operation,

How could that lead to bugs? (if you can point to existing code, but just some testcase / sample code would be fine as well).

As I said, barrier() is too heavy-handed.

Satyam Sharma wrote:
[...]

Quoted 10 lines

You think both these are equivalent in terms of "looks":

					|
while (!atomic_read(&v)) {		|	while (!atomic_read_xxx(&v)) {
	...				|		...
	cpu_relax_no_barrier();		|
cpu_relax_no_barrier();
	order_atomic(&v);		|	}
}					|

Quoted 4 lines

(where order_atomic() is an atomic_t
specific wrapper as you mentioned below)

?

I think the LHS is better if your atomic_read_xxx primitive is using the crazy one-sided barrier,

because the LHS code you immediately know what barriers are happening, and with the RHS you have to look at the atomic_read_xxx
definition.

If your atomic_read_xxx implementation was more intuitive, then both are pretty well equal. More lines != ugly code.

Quoted 2 lines

[...]
What bugs?

You can't think for yourself? Your atomic_read_volatile contains a compiler barrier to the atomic variable before the load. 2 such reads from different locations look like this:

asm volatile("" : "+m" (v1));
atomic_read(&v1);
asm volatile("" : "+m" (v2));
atomic_read(&v2);

Which implies that the load of v1 can be reordered to occur after the load of v2.

Quoted 4 lines

Quoted 1 line

Secondly, what sort of code would do such a thing?

See the nodemgr_host_thread() that does something similar, though not exactly same.

I'm sorry, all this waffling about made up code which might do this and that is just a waste of time.

I wonder if this'll generate smaller and better code than _both_ the other atomic_read_volatile() variants. Would need to build allyesconfig on lots of diff arch's etc to test the theory though.

Quoted 3 lines · Satyam Sharma

I wonder if this'll generate smaller and better code than _both_ the other atomic_read_volatile() variants. Would need to build allyesconfig on lots of diff arch's etc to test the theory though.

I'm sure it would be a tiny effect.

This whole thread is arguing about effects that are quite insignificant.

#define forget(a) __asm__ __volatile__ ("" :"=m" (a) :"m" (a))

[ This is exactly equivalent to using "+m" in the constraints, as recently
explained on a GCC list somewhere, in response to the patch in my
bitops
series a few weeks back where I thought "+m" was bogus. ]

Quoted 5 lines

#define forget(a) __asm__ __volatile__ ("" :"=m" (a) :"m" (a))

[ This is exactly equivalent to using "+m" in the constraints, as recently
explained on a GCC list somewhere, in response to the patch in my bitops series a few weeks back where I thought "+m" was bogus. ]

[It wasn't explained on a GCC list in response to your patch, as far as I can see -- if I missed it, please point me to an archived version of it].

One last time: it isn't equivalent on older (but still supported by Linux) versions of GCC. Current versions of GCC allow it, but it has no documented behaviour at all, so use it at your own risk.

Quoted 7 lines

Quoted 6 lines

#define forget(a) __asm__ __volatile__ ("" :"=m" (a) :"m" (a))

[ This is exactly equivalent to using "+m" in the constraints, as recently
explained on a GCC list somewhere, in response to the patch in my
bitops
series a few weeks back where I thought "+m" was bogus. ]

Quoted 3 lines

[It wasn't explained on a GCC list in response to your patch, as far as I can see -- if I missed it, please point me to an archived version of it].

http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01758.html

That's when _I_ came to know how GCC interprets "+m", but probably this has been explained on those lists multiple times. Who cares, anyway?

Quoted 2 lines · Paul Mackerras

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

One of the things that "volatile" generally screws up is a simple

	volatile int i;

	i++;

which a compiler will generally get horribly, horribly wrong.

In a reasonable world, gcc should just make that be (on x86)

	addl $1,i(%rip)

on x86-64, which is indeed what it does without the volatile. But with the volatile, the compiler gets really nervous, and doesn't dare do it in one instruction, and thus generates crap like

        movl    i(%rip), %eax
        addl    $1, %eax
        movl    %eax, i(%rip)

instead. For no good reason, except that "volatile" just doesn't have any good/clear semantics for the compiler, so most compilers will just make it be "I will not touch this access in any way, shape, or form". Including even trivially correct instruction optimization/combination.

Quoted 2 lines · Paul Mackerras

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

One of the things that "volatile" generally screws up is a simple

	volatile int i;

	i++;

Quoted 8 lines · Linus Torvalds

Quoted 2 lines · Paul Mackerras

I'm really surprised it's as much as a few K. I tried it on powerpc and it only saved 40 bytes (10 instructions) for a G5 config.

One of the things that "volatile" generally screws up is a simple

	volatile int i;

Quoted 1 line

	i++;

But for atomic_t people use atomic_inc() anyways which does this correctly. It shouldn't really matter for atomic_t.

I'm worrying a bit that the volatile atomic_t change caused subtle code breakage like these delay read loops people here pointed out.

Wouldn't it be safer to just re-add the volatile to atomic_read() for 2.6.23? Or alternatively make it asm(), but volatile seems more proven.

In a reasonable world, gcc should just make that be (on x86)

	addl $1,i(%rip)

on x86-64, which is indeed what it does without the volatile. But with the volatile, the compiler gets really nervous, and doesn't dare do it in one instruction, and thus generates crap like

        movl    i(%rip), %eax
        addl    $1, %eax
        movl    %eax, i(%rip)

instead. For no good reason, except that "volatile" just doesn't have any good/clear semantics for the compiler, so most compilers will just make it be "I will not touch this access in any way, shape, or form". Including even trivially correct instruction optimization/combination.

but is
(again) something that gcc doesn't dare do, since "i" is volatile.

Quoted 11 lines

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

Part of the motivation here is to fix heisenbugs. If I knew where they

By the same token we should probably disable optimisations altogether since that too can create heisenbugs.

Almost everything is a tradeoff; and so is this. I don't believe most people would find disabling all compiler optimisations an acceptable price to pay for some peace of mind.

Quoted 2 lines

Quoted 1 line

So why is this a good tradeoff?

It certainly is better than disabling all compiler optimisations!

It's easy to be better than something really stupid :)

So i386 and x86-64 don't have volatiles there, and it saves them a few K of kernel text.

What you need to justify is why it is a good
tradeoff to make them volatile (which btw, is much harder to go the other way after we let people make those assumptions).

Quoted 4 lines

Quoted 2 lines

I also think that just adding things to APIs in the hope it might fix up some bugs isn't really a good road to go down. Where do you stop?

I look at it the other way: keeping the "volatile" semantics in atomic_XXX() (or adding them to it, whatever) helps _prevent_ bugs;

Yeah, but we could add lots of things to help prevent bugs and would never be included. I would also contend that it helps _hide_ bugs and encourages people to be lazy when thinking about these things.

Also, you dismiss the fact that we'd actually be *adding* volatile semantics back to the 2 most widely tested architectures (in terms of test time, number of testers, variety of configurations, and coverage of driver code).

This is a very important different from
just keeping volatile semantics because it is basically a one-way API change.

Quoted 3 lines

certainly most people expect that behaviour, and also that behaviour is *needed* in some places and no other interface provides that functionality.

I don't know that most people would expect that behaviour.

Is there any documentation anywhere that would suggest this?

Quoted 1 line

[some confusion about barriers wrt atomics snipped]

What were you confused about?

Quoted 11 lines · Christoph Lameter

Quoted 6 lines · Chris Snook

This patchset makes the behavior of atomic_read uniform by removing the volatile keyword from all atomic_t and atomic64_t definitions that currently have it, and instead explicitly casts the variable as volatile in atomic_read(). This leaves little room for creative optimization by the compiler, and is in keeping with the principles behind "volatile considered harmful".

volatile is generally harmful even in atomic_read(). Barriers control visibility and AFAICT things are fine.

Frankly, I don't see the need for this series myself either. Personal opinion (others may differ), but I consider "volatile" to be a sad / unfortunate wart in C (numerous threads on this list and on the gcc lists/bugzilla over the years stand testimony to this) and if we _can_ steer clear of it, then why not -- why use this ill-defined primitive whose implementation has often differed over compiler versions and platforms? Granted, barrier() _is_ heavy-handed in that it makes the optimizer forget _everything_, but then somebody did post a forget() macro on this thread itself ...

[ BTW, why do we want the compiler to not optimize atomic_read()'s in
the first place? Atomic ops guarantee atomicity, which has nothing to do with "volatility" -- users that expect "volatility" from atomic ops are the ones who must be fixed instead, IMHO. ]

[ BTW, why do we want the compiler to not optimize atomic_read()'s in
the first place? Atomic ops guarantee atomicity, which has nothing to do with "volatility" -- users that expect "volatility" from atomic ops are the ones who must be fixed instead, IMHO. ]

LDD3 says on page 125: "The following operations are defined for the type [atomic_t] and are guaranteed to be atomic with respect to all processors of an SMP computer."

Doesn't "atomic WRT all processors" require volatility?

Quoted 4 lines · Satyam Sharma

[ BTW, why do we want the compiler to not optimize atomic_read()'s in
the first place? Atomic ops guarantee atomicity, which has nothing to do with "volatility" -- users that expect "volatility" from atomic ops are the ones who must be fixed instead, IMHO. ]

LDD3 says on page 125: "The following operations are defined for the type [atomic_t] and are guaranteed to be atomic with respect to all processors of an SMP computer."

Doesn't "atomic WRT all processors" require volatility?

Quoted 1 line · Stefan Richter

Doesn't "atomic WRT all processors" require volatility?

No, it definitely doesn't. Why should it?

"Atomic w.r.t. all processors" is just your normal, simple "atomicity" for SMP systems (ensure that that object is modified / set / replaced in main memory atomically) and has nothing to do with "volatile" behaviour.

"Volatile behaviour" itself isn't consistently defined (at least definitely not consistently implemented in various gcc versions across platforms), but it is /expected/ to mean something like: "ensure that every such access actually goes all the way to memory, and is not re-ordered w.r.t. to other accesses, as far as the compiler can take care of these". The last "as far as compiler can take care" disclaimer comes about due to CPUs doing their own re-ordering nowadays.

For example (say on i386):

In (A) the compiler optimized "a = 10;" away, but the actual store of the final value "20" to "a" was still "atomic". (B) and (C) also exhibit "volatile" behaviour apart from the "atomicity".

But as others replied, it seems some callers out there depend upon atomic ops exhibiting "volatile" behaviour as well, so that answers my initial question, actually. I haven't looked at the code Paul pointed me at, but I wonder if that "forget(x)" macro would help those cases. I'd wish to avoid the "volatile" primitive, personally.

static inline void A(atomic_t *a)

{
	int b = atomic_read(a);
	if (b)
		do_something_time_consuming();
}

static inline void B(atomic_t *a)

{
	int b = atomic_read(a);
	if (b)
		do_something_more();
}

static void C(atomic_t *a)

{
	A(a);
	B(b);

}

Would this need forget(a) after A(a)?

(Is the latter actually answered in C99 or is it compiler-dependent?)

Quoted 19 lines · Satyam Sharma

Quoted 1 line · Stefan Richter

Doesn't "atomic WRT all processors" require volatility?

No, it definitely doesn't. Why should it?

"Atomic w.r.t. all processors" is just your normal, simple "atomicity" for SMP systems (ensure that that object is modified / set / replaced in main memory atomically) and has nothing to do with "volatile" behaviour.

"Volatile behaviour" itself isn't consistently defined (at least definitely not consistently implemented in various gcc versions across platforms), but it is /expected/ to mean something like: "ensure that every such access actually goes all the way to memory, and is not re-ordered w.r.t. to other accesses, as far as the compiler can take care of these". The last "as far as compiler can take care" disclaimer comes about due to CPUs doing their own re-ordering nowadays.

For example (say on i386):

[...]

Quoted 9 lines

In (A) the compiler optimized "a = 10;" away, but the actual store of the final value "20" to "a" was still "atomic". (B) and (C) also exhibit "volatile" behaviour apart from the "atomicity".

But as others replied, it seems some callers out there depend upon atomic ops exhibiting "volatile" behaviour as well, so that answers my initial question, actually. I haven't looked at the code Paul pointed me at, but I wonder if that "forget(x)" macro would help those cases. I'd wish to avoid the "volatile" primitive, personally.

So, looking at load instead of store, understand I correctly that in your opinion

	int b;

	b = atomic_read(&a);
	if (b)
		do_something_time_consuming();

	b = atomic_read(&a);
	if (b)
		do_something_more();

should be changed to explicitly forget(&a) after
do_something_time_consuming?

Quoted 46 lines · Stefan Richter

Quoted 19 lines · Satyam Sharma

Quoted 1 line · Stefan Richter

Doesn't "atomic WRT all processors" require volatility?

No, it definitely doesn't. Why should it?

"Atomic w.r.t. all processors" is just your normal, simple "atomicity" for SMP systems (ensure that that object is modified / set / replaced in main memory atomically) and has nothing to do with "volatile" behaviour.

"Volatile behaviour" itself isn't consistently defined (at least definitely not consistently implemented in various gcc versions across platforms), but it is /expected/ to mean something like: "ensure that every such access actually goes all the way to memory, and is not re-ordered w.r.t. to other accesses, as far as the compiler can take care of these". The last "as far as compiler can take care" disclaimer comes about due to CPUs doing their own re-ordering nowadays.

For example (say on i386):

[...]

Quoted 9 lines

In (A) the compiler optimized "a = 10;" away, but the actual store of the final value "20" to "a" was still "atomic". (B) and (C) also exhibit "volatile" behaviour apart from the "atomicity".

But as others replied, it seems some callers out there depend upon atomic ops exhibiting "volatile" behaviour as well, so that answers my initial question, actually. I haven't looked at the code Paul pointed me at, but I wonder if that "forget(x)" macro would help those cases. I'd wish to avoid the "volatile" primitive, personally.

So, looking at load instead of store, understand I correctly that in your opinion

	int b;

	b = atomic_read(&a);
	if (b)
		do_something_time_consuming();

	b = atomic_read(&a);
	if (b)
		do_something_more();

Quoted 2 lines

should be changed to explicitly forget(&a) after
do_something_time_consuming?

No, I'd actually prefer something like what Christoph Lameter suggested, i.e. users (such as above) who want "volatile"-like behaviour from atomic ops can use alternative functions. How about something like:

#define atomic_read_volatile(v)			\
	({					\
		forget(&(v)->counter);		\
		((v)->counter);			\
	})

Or possibly, implement these "volatile" atomic ops variants in inline asm like the patch that Sebastian Siewior has submitted on another thread just a while back.

Of course, if we find there are more callers in the kernel who want the volatility behaviour than those who don't care, we can re-define the existing ops to such variants, and re-name the existing definitions to somethine else, say "atomic_read_nonvolatile" for all I care.

Do we really need another set of APIs? Can you give even one example where the pre-existing volatile semantics are causing enough of a problem to justify adding yet more atomic_*() APIs?

Quoted 4 lines · Paul E. McKenney

Do we really need another set of APIs? Can you give even one example where the pre-existing volatile semantics are causing enough of a problem to justify adding yet more atomic_*() APIs?

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Quoted 8 lines · Herbert Xu

Quoted 4 lines · Paul E. McKenney

Do we really need another set of APIs? Can you give even one example where the pre-existing volatile semantics are causing enough of a problem to justify adding yet more atomic_*() APIs?

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Sorry, but you are the one advocating for the change.

Quoted 7 lines · Paul E. McKenney

Quoted 6 lines · Herbert Xu

Quoted 4 lines · Paul E. McKenney

Do we really need another set of APIs? Can you give even one example where the pre-existing volatile semantics are causing enough of a problem to justify adding yet more atomic_*() APIs?

Quoted 4 lines

Quoted 2 lines

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Sorry, but you are the one advocating for the change.

Not for i386 and x86_64 -- those have atomic ops without any "volatile" semantics (currently as per existing definitions).

Quoted 3 lines · Satyam Sharma

[...]
Not for i386 and x86_64 -- those have atomic ops without any "volatile" semantics (currently as per existing definitions).

I claim unit volumes with arm, and the majority of the architectures, but I cannot deny the popularity of i386 and x86_64 with many developers. ;-)

However, I am not aware of code in the kernel that would benefit from the compiler coalescing multiple atomic_set() and atomic_read() invocations, thus I don't see the downside to volatility in this case. Are there some performance-critical code fragments that I am missing?

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Quoted 2 lines · Herbert Xu

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Accessing H/W registers? But apart from that...

Quoted 6 lines · David Howells

Quoted 2 lines · Herbert Xu

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Accessing H/W registers? But apart from that...

Communicating between process context and interrupt/NMI handlers using per-CPU variables.

Quoted 6 lines · Paul E. McKenney

Quoted 5 lines · David Howells

Quoted 2 lines · Herbert Xu

Let's turn this around. Can you give a single example where the volatile semantics is needed in a legitimate way?

Quoted 4 lines

Quoted 1 line

Accessing H/W registers? But apart from that...

Communicating between process context and interrupt/NMI handlers using per-CPU variables.

Remeber we're talking about atomic_read/atomic_set. Please cite the actual file/function name you have in mind.

Quoted 6 lines

Quoted 3 lines

Communicating between process context and interrupt/NMI handlers using per-CPU variables.

Remeber we're talking about atomic_read/atomic_set. Please cite the actual file/function name you have in mind.

Yep, we are indeed talking about atomic_read()/atomic_set().

We have been through this issue already in this thread.

Quoted 7 lines · Paul E. McKenney

Quoted 6 lines

Quoted 3 lines

Communicating between process context and interrupt/NMI handlers using per-CPU variables.

Remeber we're talking about atomic_read/atomic_set. Please cite the actual file/function name you have in mind.

Quoted 3 lines

Yep, we are indeed talking about atomic_read()/atomic_set().

We have been through this issue already in this thread.

Sorry, but I must've missed it. Could you cite the file or function for my benefit?

Quoted 10 lines · Herbert Xu

Quoted 9 lines · Paul E. McKenney

Quoted 6 lines

Quoted 3 lines

Communicating between process context and interrupt/NMI handlers using per-CPU variables.

Remeber we're talking about atomic_read/atomic_set. Please cite the actual file/function name you have in mind.

Yep, we are indeed talking about atomic_read()/atomic_set().

Quoted 4 lines

Quoted 1 line

We have been through this issue already in this thread.

Sorry, but I must've missed it. Could you cite the file or function for my benefit?

I might summarize the thread if there is interest, but I am not able to do so right this minute.

Quoted 16 lines · Paul E. McKenney

Quoted 14 lines · Herbert Xu

Quoted 10 lines · Paul E. McKenney

Quoted 6 lines

Quoted 3 lines

Communicating between process context and interrupt/NMI handlers using per-CPU variables.

Remeber we're talking about atomic_read/atomic_set. Please cite the actual file/function name you have in mind.

Yep, we are indeed talking about atomic_read()/atomic_set().

We have been through this issue already in this thread.

Sorry, but I must've missed it. Could you cite the file or function for my benefit?

Quoted 2 lines

I might summarize the thread if there is interest, but I am not able to do so right this minute.

Thanks. But I don't need a summary of the thread, I'm asking for an extant code snippet in our kernel that benefits from the volatile change and is not part of a busy-wait.

Quoted 3 lines · Herbert Xu

Thanks. But I don't need a summary of the thread, I'm asking for an extant code snippet in our kernel that benefits from the volatile change and is not part of a busy-wait.

Sorry, can't help you there. I really do believe that the information you need (as opposed to the specific item you are asking for) really has been put forth in this thread.

Quoted 6 lines · Paul E. McKenney

Quoted 3 lines · Herbert Xu

Thanks. But I don't need a summary of the thread, I'm asking for an extant code snippet in our kernel that benefits from the volatile change and is not part of a busy-wait.

Quoted 3 lines

Sorry, can't help you there. I really do believe that the information you need (as opposed to the specific item you are asking for) really has been put forth in this thread.

That only leads me to believe that such a code snippet simply does not exist.

Quoted 5 lines · Satyam Sharma

[...]
No, I'd actually prefer something like what Christoph Lameter suggested, i.e. users (such as above) who want "volatile"-like behaviour from atomic ops can use alternative functions. How about something like:

Quoted 5 lines

#define atomic_read_volatile(v)			\
	({					\
		forget(&(v)->counter);		\
		((v)->counter);			\
	})

Wouldn't the above "forget" the value, throw it away, then forget that it forgot it, giving non-volatile semantics?

Quoted 3 lines

Or possibly, implement these "volatile" atomic ops variants in inline asm like the patch that Sebastian Siewior has submitted on another thread just a while back.

Given that you are advocating a change (please keep in mind that atomic_read() and atomic_set() had volatile semantics on almost all platforms), care to give some example where these historical volatile semantics are causing a problem?
[...]
Can you give even one example
where the pre-existing volatile semantics are causing enough of a problem to justify adding yet more atomic_*() APIs?

Quoted 4 lines

Of course, if we find there are more callers in the kernel who want the volatility behaviour than those who don't care, we can re-define the existing ops to such variants, and re-name the existing definitions to somethine else, say "atomic_read_nonvolatile" for all I care.

Do we really need another set of APIs?

Hi Paul,

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Satyam Sharma

[...]
No, I'd actually prefer something like what Christoph Lameter suggested, i.e. users (such as above) who want "volatile"-like behaviour from atomic ops can use alternative functions. How about something like:

#define atomic_read_volatile(v)			\
	({					\
		forget(&(v)->counter);		\
		((v)->counter);			\
	})

Quoted 2 lines

Wouldn't the above "forget" the value, throw it away, then forget that it forgot it, giving non-volatile semantics?

Nope, I don't think so. I wrote the following trivial testcases:
[ See especially tp4.c and tp4.s (last example). ]

==============================================================================

$ cat tp1.c # Using volatile access casts

#define atomic_read(a) (*(volatile int *)&a)

int a;

void func(void)

{
	a = 0;
	while (atomic_read(a))
		;
}
==============================================================================
$ gcc -Os -S tp1.c; cat tp1.s

func:

	pushl	%ebp
	movl	%esp, %ebp
	movl	$0, a

.L2:
movl a, %eax
testl %eax, %eax
jne .L2
popl %ebp
ret
==============================================================================

$ cat tp2.c # Using nothing; gcc will optimize the whole loop away

#define forget(x)

#define atomic_read(a)		\
	({			\
		forget(&(a));	\
		(a);		\
	})

int a;

void func(void)

{
	a = 0;
	while (atomic_read(a))
		;
}
==============================================================================
$ gcc -Os -S tp2.c; cat tp2.s

func:

	pushl	%ebp
	movl	%esp, %ebp
	popl	%ebp
	movl	$0, a
	ret
==============================================================================
$ cat tp3.c # Using a full memory clobber barrier

#define forget(x) asm volatile ("":::"memory")

#define atomic_read(a)		\
	({			\
		forget(&(a));	\
		(a);		\
	})

int a;

void func(void)

{
	a = 0;
	while (atomic_read(a))
		;
}
==============================================================================
$ gcc -Os -S tp3.c; cat tp3.s

func:

	pushl	%ebp
	movl	%esp, %ebp
	movl	$0, a
.L2:
	cmpl	$0, a
	jne	.L2
	popl	%ebp
	ret
==============================================================================
$ cat tp4.c # Using a forget(var) macro

#define forget(a) __asm__ __volatile__ ("" :"=m" (a) :"m" (a))

#define atomic_read(a)		\
	({			\
		forget(a);	\
		(a);		\
	})

int a;

void func(void)

{
	a = 0;
	while (atomic_read(a))
		;
}
==============================================================================
$ gcc -Os -S tp4.c; cat tp4.s

func:

	pushl	%ebp
	movl	%esp, %ebp
	movl	$0, a
.L2:
	cmpl	$0, a
	jne	.L2
	popl	%ebp
	ret

==============================================================================

Possibly these were too trivial to expose any potential problems that you may have been referring to, so would be helpful if you could write a more concrete example / sample code.

Quoted 4 lines

Quoted 3 lines

Or possibly, implement these "volatile" atomic ops variants in inline asm like the patch that Sebastian Siewior has submitted on another thread just a while back.

Quoted 8 lines

Given that you are advocating a change (please keep in mind that atomic_read() and atomic_set() had volatile semantics on almost all platforms), care to give some example where these historical volatile semantics are causing a problem?
[...]
Can you give even one example
where the pre-existing volatile semantics are causing enough of a problem to justify adding yet more atomic_*() APIs?

Will take this to the other sub-thread ...

Quoted 6 lines

Quoted 4 lines

Of course, if we find there are more callers in the kernel who want the volatility behaviour than those who don't care, we can re-define the existing ops to such variants, and re-name the existing definitions to somethine else, say "atomic_read_nonvolatile" for all I care.

Do we really need another set of APIs?

Well, if there's one set of users who do care about volatile behaviour, and another set that doesn't, it only sounds correct to provide both those APIs, instead of forcing one behaviour on all users.

Quoted 3 lines

Possibly these were too trivial to expose any potential problems that you may have been referring to, so would be helpful if you could write a more concrete example / sample code.

The trick is to have a sufficiently complicated expression to force the compiler to run out of registers.

Quoted 6 lines

Quoted 4 lines

Of course, if we find there are more callers in the kernel who want the volatility behaviour than those who don't care, we can re-define the existing ops to such variants, and re-name the existing definitions to somethine else, say "atomic_read_nonvolatile" for all I care.

Do we really need another set of APIs?

Well, if there's one set of users who do care about volatile behaviour, and another set that doesn't, it only sounds correct to provide both those APIs, instead of forcing one behaviour on all users.

"Volatile behaviour" itself isn't consistently defined (at least definitely not consistently implemented in various gcc versions across platforms),

but it is /expected/ to mean something like: "ensure that every such access actually goes all the way to memory, and is not re-ordered w.r.t. to other accesses, as far as the compiler can take care of these". The last "as far as compiler can take care" disclaimer comes about due to CPUs doing their own re-ordering nowadays.

Quoted 3 lines

"Volatile behaviour" itself isn't consistently defined (at least definitely not consistently implemented in various gcc versions across platforms),

It should be consistent across platforms; if not, file a bug please.

Quoted 3 lines

but it is /expected/ to mean something like: "ensure that every such access actually goes all the way to memory, and is not re-ordered w.r.t. to other accesses, as far as the compiler can take

Quoted 2 lines

care of these". The last "as far as compiler can take care" disclaimer comes about due to CPUs doing their own re-ordering nowadays.

You can *expect* whatever you want, but this isn't in line with reality at all.

volatile _does not_ prevent reordering wrt other accesses. [...]
What volatile does are a) never optimise away a read (or write) to the object, since the data can change in ways the compiler cannot see; and b) never move stores to the object across a sequence point. This does not mean other accesses cannot be reordered wrt the volatile access.

If the abstract machine would do an access to a volatile- qualified object, the generated machine code will do that access too. But, for example, it can still be optimised away by the compiler, if it can prove it is allowed to.

volatile _does not_ make accesses go all the way to memory. [...]
If you want stuff to go all the way to memory, you need some architecture-specific flush sequence; to make a store globally visible before another store, you need mb(); before some following read, you need mb(); to prevent reordering you need a barrier.

Quoted 6 lines

What volatile does are a) never optimise away a read (or write) to the object, since the data can change in ways the compiler cannot see; and b) never move stores to the object across a sequence point. This does not mean other accesses cannot be reordered wrt the volatile access.

Quoted 4 lines

If the abstract machine would do an access to a volatile- qualified object, the generated machine code will do that access too. But, for example, it can still be optimised away by the compiler, if it can prove it is allowed to.

As (now) indicated above, I had meant multiple volatile accesses to the same object, obviously.

BTW:

#define atomic_read(a) (*(volatile int *)&(a))
#define atomic_set(a,i) (*(volatile int *)&(a) = (i))

int a;

void func(void)

{
	int b;

	b = atomic_read(a);
	atomic_set(a, 20);
	b = atomic_read(a);
}

gives:

func:

	pushl	%ebp
	movl	a, %eax
	movl	%esp, %ebp
	movl	$20, a
	movl	a, %eax
	popl	%ebp
	ret

so the first atomic_read() wasn't optimized away.

[...]

Quoted 27 lines

BTW:

#define atomic_read(a) (*(volatile int *)&(a))
#define atomic_set(a,i) (*(volatile int *)&(a) = (i))

int a;

void func(void)

{
	int b;

	b = atomic_read(a);
	atomic_set(a, 20);
	b = atomic_read(a);
}

gives:

func:

	pushl	%ebp
	movl	a, %eax
	movl	%esp, %ebp
	movl	$20, a
	movl	a, %eax
	popl	%ebp
	ret

Quoted 1 line

so the first atomic_read() wasn't optimized away.

Of course. It is executed by the abstract machine, so it will be executed by the actual machine. On the other hand, try

	b = 0;
	if (b)
		b = atomic_read(a);

or similar.

Quoted 1 line

Doesn't "atomic WRT all processors" require volatility?

No, it definitely doesn't. Why should it?

"Atomic w.r.t. all processors" is just your normal, simple "atomicity" for SMP systems (ensure that that object is modified / set / replaced in main memory atomically) and has nothing to do with "volatile" behaviour.

In the kernel we use atomic variables in precisely those situations where a variable is potentially accessed concurrently by multiple CPUs, and where each CPU needs to see updates done by other CPUs in a timely fashion. That is what they are for. Therefore the compiler must not cache values of atomic variables in registers; each atomic_read must result in a load and each atomic_set must result in a store. Anything else will just lead to subtle bugs.

Quoted 7 lines · Paul Mackerras

In the kernel we use atomic variables in precisely those situations where a variable is potentially accessed concurrently by multiple CPUs, and where each CPU needs to see updates done by other CPUs in a timely fashion. That is what they are for. Therefore the compiler must not cache values of atomic variables in registers; each atomic_read must result in a load and each atomic_set must result in a store. Anything else will just lead to subtle bugs.

This may have been the intend. However, today the visibility is controlled using barriers. And we have barriers that we use with atomic operations.

Those barriers are for when we need ordering between atomic variables and other memory locations. An atomic variable by itself doesn't and shouldn't need any barriers for other CPUs to be able to see what's happening to it.

Quoted 7 lines · Paul Mackerras

In the kernel we use atomic variables in precisely those situations where a variable is potentially accessed concurrently by multiple CPUs, and where each CPU needs to see updates done by other CPUs in a timely fashion. That is what they are for. Therefore the compiler must not cache values of atomic variables in registers; each atomic_read must result in a load and each atomic_set must result in a store. Anything else will just lead to subtle bugs.

This may have been the intend. However, today the visibility is controlled using barriers. And we have barriers that we use with atomic operations. Having volatile be the default just lead to confusion. Atomic read should just read with no extras. Extras can be added by using variants like atomic_read_volatile or so.

Seems to me that we face greater chance of confusion without the volatile than with, particularly as compiler optimizations become more aggressive. Yes, we could simply disable optimization, but optimization can be quite helpful.

Quoted 4 lines · Paul E. McKenney

Seems to me that we face greater chance of confusion without the volatile than with, particularly as compiler optimizations become more aggressive. Yes, we could simply disable optimization, but optimization can be quite helpful.

A volatile default would disable optimizations for atomic_read. atomic_read without volatile would allow for full optimization by the compiler. Seems that this is what one wants in many cases.

The volatile cast should not disable all that many optimizations, for example, it is much less hurtful than barrier(). Furthermore, the main optimizations disabled (pulling atomic_read() and atomic_set() out of loops) really do need to be disabled.

Quoted 4 lines · Paul E. McKenney

The volatile cast should not disable all that many optimizations, for example, it is much less hurtful than barrier(). Furthermore, the main optimizations disabled (pulling atomic_read() and atomic_set() out of loops) really do need to be disabled.

In many cases you do not need a barrier. Having volatile there *will* impact optimization because the compiler cannot use a register that may contain the value that was fetched earlier. And the compiler cannot choose freely when to fetch the value. The order of memory accesses are fixed if you use volatile. If the variable is not volatile then the compiler can arrange memory accesses any way they fit and thus generate better code.

Understood. My point is not that the impact is precisely zero, but rather that the impact on optimization is much less hurtful than the problems that could arise otherwise, particularly as compilers become more aggressive in their optimizations.

Quoted 4 lines · Paul E. McKenney

Understood. My point is not that the impact is precisely zero, but rather that the impact on optimization is much less hurtful than the problems that could arise otherwise, particularly as compilers become more aggressive in their optimizations.

The problems arise because barriers are not used as required. Volatile has wishy washy semantics and somehow marries memory barriers with data access. It is clearer to separate the two. Conceptual cleanness usually translates into better code. If one really wants the volatile then lets make it explicit and use

atomic_read_volatile()

Completely agreed, again. To summarize again (had done so about ~100 mails earlier in this thread too :-) ...

atomic_{read,set}_volatile() -- guarantees volatility also along with atomicity (the two _are_ different concepts after all, irrespective of whether callsites normally want one with the other or not)

atomic_{read,set}_nonvolatile() -- only guarantees atomicity, compiler free to elid / coalesce / optimize such accesses, can keep the object in question cached in a local register, leads to smaller text, etc.

As to which one should be the default atomic_read() is a question of whether majority of callsites (more weightage to important / hot codepaths, lesser to obscure callsites) want a particular behaviour.

Do we have a consensus here? (hoping against hope, probably :-)

Quoted 1 line

Do we have a consensus here? (hoping against hope, probably :-)

I can certainly agree with this.

But I have to say that I still don't know of a single place where one would actually use the volatile variant.

Quoted 2 lines

But I have to say that I still don't know of a single place where one would actually use the volatile variant.

I suspect that what you say is true after we have looked at all callers.

In particular, atomic_read seems to lend itself to buggy uses. People seem to do things like:

	atomic_add(&v, something);
	if (atomic_read(&v) > something_else) ...

I'd go so far as to say that anywhere where you want a non-"volatile" atomic_read, either your code is buggy, or else an int would work just as well.

If you're referring to the code in sk_stream_mem_schedule then it's working as intended. The atomicity guarantees

that the atomic_add/atomic_sub won't be seen in parts by other readers.

We certainly do not need to see other atomic_add/atomic_sub operations immediately.

If you're referring to another code snippet please cite.

Quoted 3 lines

I'd go so far as to say that anywhere where you want a non-"volatile" atomic_read, either your code is buggy, or else an int would work just as well.

An int won't work here because += and -= do not have the atomicity guarantees that atomic_add/atomic_sub do. In particular, this may cause an atomic_read on another CPU to give a bogus reading.

Quoted 2 lines · Herbert Xu

If you're referring to the code in sk_stream_mem_schedule then it's working as intended. The atomicity guarantees

You mean it's intended that *sk->sk_prot->memory_pressure can end up as 1 when sk->sk_prot->memory_allocated is small (less than ->sysctl_mem[0]), or as 0 when ->memory_allocated is large (greater than ->sysctl_mem[2])? Because that's the effect of the current code. If so I wonder why you bother computing it.

Quoted 5 lines

You mean it's intended that *sk->sk_prot->memory_pressure can end up as 1 when sk->sk_prot->memory_allocated is small (less than ->sysctl_mem[0]), or as 0 when ->memory_allocated is large (greater than ->sysctl_mem[2])? Because that's the effect of the current code. If so I wonder why you bother computing it.

You need to remember that there are three different limits:
minimum, pressure, and maximum. By default we should never be in a situation where what you say can occur.

If you set all three limits to the same thing, then yes it won't work as intended but it's still well-behaved.

I'm talking about this situation:

CPU 0 comes into __sk_stream_mem_reclaim, reads memory_allocated, but then before it can do the store to *memory_pressure, CPUs 1-1023 all go through sk_stream_mem_schedule, collectively increase memory_allocated to more than sysctl_mem[2] and set *memory_pressure. Finally CPU 0 gets to do its store and it sets *memory_pressure back to 0, but by this stage memory_allocated is way larger than sysctl_mem[2].

Now, maybe it's the case that it doesn't really matter whether *->memory_pressure is 0 or 1. But if so, why bother computing it at all?

People seem to think that using atomic_t means they don't need to use a spinlock. That's fine if there is only one variable involved, but as soon as there's more than one, there's the possibility of a race, whether or not you use atomic_t, and whether or not atomic_read has "volatile" behaviour.

It doesn't matter. The memory pressure flag is an *advisory* flag. If we get it wrong the worst that'll happen is that we'd waste some time doing work that'll be thrown away.

In any case, this actually illustrates why the addition of volatile is completely pointless. Even if this code was broken, which it definitely is not, having the volatile there wouldn't have helped at all.

Quoted 3 lines · Herbert Xu

It doesn't matter. The memory pressure flag is an *advisory* flag. If we get it wrong the worst that'll happen is that we'd waste some time doing work that'll be thrown away.

Ah, so it's the "racy but I don't care because it's only an optimization" case. That's fine. Somehow I find it hard to believe that all the racy uses of atomic_read in the kernel are like that, though. :)

It seems that there could be a lot of places where atomic_t is used in a non-atomic fashion, and that those uses are either buggy, or there is some lock held at the time which guarantees that other CPUs aren't changing the value. In both cases there is no point in using atomic_t; we might as well just use an ordinary int.

I'd go so far as to say that anywhere where you want a non-"volatile" atomic_read, either your code is buggy, or else an int would work just as well.

But I have to say that I still don't know of a single place where one would actually use the volatile variant.

Quoted 4 lines · Paul E. McKenney

Understood. My point is not that the impact is precisely zero, but rather that the impact on optimization is much less hurtful than the problems that could arise otherwise, particularly as compilers become more aggressive in their optimizations.

The problems arise because barriers are not used as required. Volatile has wishy washy semantics and somehow marries memory barriers with data access. It is clearer to separate the two. Conceptual cleanness usually translates into better code. If one really wants the volatile then lets make it explicit and use

atomic_read_volatile()

A volatile default would disable optimizations for atomic_read. atomic_read without volatile would allow for full optimization by the compiler. Seems that this is what one wants in many cases.

Name one such case.

See sk_stream_mem_schedule in net/core/stream.c:

        /* Under limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) < sk->sk_prot->sysctl_mem[0]) {
                if (*sk->sk_prot->memory_pressure)
                        *sk->sk_prot->memory_pressure = 0;
                return 1;
        }

        /* Over hard limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) > sk->sk_prot->sysctl_mem[2]) {
                sk->sk_prot->enter_memory_pressure();
                goto suppress_allocation;
        }

We don't need to reload sk->sk_prot->memory_allocated here.

Quoted 15 lines · Herbert Xu

See sk_stream_mem_schedule in net/core/stream.c:

        /* Under limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) < sk->sk_prot->sysctl_mem[0]) {
                if (*sk->sk_prot->memory_pressure)
                        *sk->sk_prot->memory_pressure = 0;
                return 1;
        }

        /* Over hard limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) > sk->sk_prot->sysctl_mem[2]) {
                sk->sk_prot->enter_memory_pressure();
                goto suppress_allocation;
        }

Quoted 1 line

We don't need to reload sk->sk_prot->memory_allocated here.

Are you sure? How do you know some other CPU hasn't changed the value in between?

Quoted 19 lines · Paul Mackerras

Quoted 16 lines · Herbert Xu

See sk_stream_mem_schedule in net/core/stream.c:

        /* Under limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) < sk->sk_prot->sysctl_mem[0]) {
                if (*sk->sk_prot->memory_pressure)
                        *sk->sk_prot->memory_pressure = 0;
                return 1;
        }

        /* Over hard limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) > sk->sk_prot->sysctl_mem[2]) {
                sk->sk_prot->enter_memory_pressure();
                goto suppress_allocation;
        }

We don't need to reload sk->sk_prot->memory_allocated here.

Quoted 2 lines

Are you sure? How do you know some other CPU hasn't changed the value in between?

Yes I'm sure, because we don't care if others have increased the reservation.

Note that even if we did we'd be using barriers so volatile won't do us any good here.

Quoted 2 lines

Are you sure? How do you know some other CPU hasn't changed the value in between?

Yes I'm sure, because we don't care if others have increased the reservation.

But others can also reduce the reservation. Also, the code sets and clears *sk->sk_prot->memory_pressure nonatomically with respect to the reads of sk->sk_prot->memory_allocated, so in fact the code doesn't guarantee any particular relationship between the two.

Quoted 1 line

We don't need to reload sk->sk_prot->memory_allocated here.

Are you sure? How do you know some other CPU hasn't changed the value in between?

Quoted 4 lines · Paul Mackerras

Quoted 1 line

We don't need to reload sk->sk_prot->memory_allocated here.

Are you sure? How do you know some other CPU hasn't changed the value in between?

The cpu knows because the cacheline was not invalidated.

Quoted 15 lines · Herbert Xu

See sk_stream_mem_schedule in net/core/stream.c:

        /* Under limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) < sk->sk_prot->sysctl_mem[0]) {
                if (*sk->sk_prot->memory_pressure)
                        *sk->sk_prot->memory_pressure = 0;
                return 1;
        }

        /* Over hard limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) > sk->sk_prot->sysctl_mem[2]) {
                sk->sk_prot->enter_memory_pressure();
                goto suppress_allocation;
        }

Quoted 1 line

We don't need to reload sk->sk_prot->memory_allocated here.

Are you sure? How do you know some other CPU hasn't changed the value in between?

Quoted 19 lines · Paul Mackerras

Quoted 16 lines · Herbert Xu

See sk_stream_mem_schedule in net/core/stream.c:

        /* Under limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) < sk->sk_prot->sysctl_mem[0]) {
                if (*sk->sk_prot->memory_pressure)
                        *sk->sk_prot->memory_pressure = 0;
                return 1;
        }

        /* Over hard limit. */
        if (atomic_read(sk->sk_prot->memory_allocated) > sk->sk_prot->sysctl_mem[2]) {
                sk->sk_prot->enter_memory_pressure();
                goto suppress_allocation;
        }

We don't need to reload sk->sk_prot->memory_allocated here.

Quoted 2 lines

Are you sure? How do you know some other CPU hasn't changed the value in between?

I can't speak for this particular case, but there could be similar code examples elsewhere, where we do the atomic ops on an atomic_t object inside a higher-level locking scheme that would take care of the kind of problem you're referring to here. It would be useful for such or similar code if the compiler kept the value of that atomic object in a register.

Anyway, the problem, of course, is that this conversion to a stronger / safer-by-default behaviour doesn't happen with zero cost to performance. Converting atomic ops to "volatile" behaviour did add ~2K to kernel text for archs such as i386 (possibly to important codepaths) that didn't have those semantics already so it would be constructive to actually look at those differences and see if there were really any heisenbugs that got rectified. Or if there were legitimate optimizations that got wrongly disabled. Onus lies on those proposing the modifications, I'd say ;-)

The uses of atomic_read where one might want it to allow caching of the result seem to me to fall into 3 categories:

1. Places that are buggy because of a race arising from the way it's used.

2. Places where there is a race but it doesn't matter because we're doing some clever trick.

3. Places where there is some locking in place that eliminates any potential race.

In case 1, adding volatile won't solve the race, of course, but it's hard to argue that we shouldn't do something because it will slow down buggy code. Case 2 is hopefully pretty rare and accompanied by large comment blocks, and in those cases caching the result of atomic_read explicitly in a local variable would probably make the code clearer. And in case 3 there is no reason to use atomic_t at all; we might as well just use an int.

Quoted 4 lines · Paul Mackerras

The uses of atomic_read where one might want it to allow caching of the result seem to me to fall into 3 categories:

Quoted 8 lines

1. Places that are buggy because of a race arising from the way it's used.

2. Places where there is a race but it doesn't matter because we're doing some clever trick.

3. Places where there is some locking in place that eliminates any potential race.

Agreed.

Quoted 7 lines

In case 1, adding volatile won't solve the race, of course, but it's hard to argue that we shouldn't do something because it will slow down buggy code. Case 2 is hopefully pretty rare and accompanied by large comment blocks, and in those cases caching the result of atomic_read explicitly in a local variable would probably make the code clearer. And in case 3 there is no reason to use atomic_t at all; we might as well just use an int.

Since adding volatile doesn't help any of the 3 cases, and takes away optimisations from both 2 and 3, I wonder what is the point of the addition after all?

Note that I said these are the cases _where one might want to allow caching_, so of course adding volatile doesn't help _these_ cases. There are of course other cases where one definitely doesn't want to allow the compiler to cache the value, such as when polling an atomic variable waiting for another CPU to change it, and from my inspection so far these cases seem to be the majority.

- It matches the normal expectation based on the name "atomic_read"
- It matches the behaviour of the other atomic_* primitives

- It avoids bugs in the cases where "volatile" behaviour is required

Quoted 7 lines · Paul Mackerras

Note that I said these are the cases _where one might want to allow caching_, so of course adding volatile doesn't help _these_ cases. There are of course other cases where one definitely doesn't want to allow the compiler to cache the value, such as when polling an atomic variable waiting for another CPU to change it, and from my inspection so far these cases seem to be the majority.

We've been through that already. If it's a busy-wait it should use cpu_relax. If it's scheduling away that already forces the compiler to reread anyway.

Do you have an actual example where volatile is needed?

Quoted 2 lines

- It matches the normal expectation based on the name "atomic_read"
- It matches the behaviour of the other atomic_* primitives

Can't argue since you left out what those expectations or properties are.

Quoted 1 line

- It avoids bugs in the cases where "volatile" behaviour is required

Do you (or anyone else for that matter) have an example of this?

Quoted 2 lines

Do you (or anyone else for that matter) have an example of this?

The only code I somewhat know, the ieee1394 subsystem, was perhaps authored and is currently maintained with the expectation that each occurrence of atomic_read actually results in a load operation, i.e. is not optimized away. This means all atomic_t (bus generation, packet and buffer refcounts, and some other state variables)* and likewise all atomic bitops in that subsystem.

Quoted 9 lines · Stefan Richter

Quoted 2 lines

Do you (or anyone else for that matter) have an example of this?

The only code I somewhat know, the ieee1394 subsystem, was perhaps authored and is currently maintained with the expectation that each occurrence of atomic_read actually results in a load operation, i.e. is not optimized away. This means all atomic_t (bus generation, packet and buffer refcounts, and some other state variables)* and likewise all atomic bitops in that subsystem.

Can you find an actual atomic_read code snippet there that is broken without the volatile modifier?

Quoted 1 line · Herbert Xu

On Thu, Aug 16, 2007 at 10:06:31AM +0200, Stefan Richter wrote:

Quoted 3 lines

Quoted 3 lines

expectation that each
occurrence of atomic_read actually results in a load operation, i.e. is not optimized away.

Quoted 2 lines

Can you find an actual atomic_read code snippet there that is broken without the volatile modifier?

PS: Just to clarify, I'm not speaking for the volatile modifier. I'm not speaking for any particular implementation of atomic_t and its accessors at all. All I am saying is that
- we use atomically accessed data types because we concurrently but locklessly access this data,
- hence a read access to this data that could be optimized away makes *no sense at all*.

So, the architecture guys can implement atomic_read however they want --- as long as it cannot be optimized away.*

Quoted 8 lines · Stefan Richter

PS: Just to clarify, I'm not speaking for the volatile modifier. I'm not speaking for any particular implementation of atomic_t and its accessors at all. All I am saying is that
- we use atomically accessed data types because we concurrently but locklessly access this data,
- hence a read access to this data that could be optimized away makes *no sense at all*.

No sane compiler can optimise away an atomic_read per se. That's only possible if there's a preceding atomic_set or atomic_read, with no barriers in the middle.

If that's the case, then one has to conclude that doing away with the second read is acceptable, as otherwise a memory (or at least a compiler) barrier should have been used.

In fact, volatile doesn't guarantee that the memory gets read anyway. You might be reading some stale value out of the cache. Granted this doesn't happen on x86 but when you're coding for the kernel you can't make such assumptions.

So the point here is that if you don't mind getting a stale value from the CPU cache when doing an atomic_read, then surely you won't mind getting a stale value from the compiler "cache".

Quoted 2 lines

So, the architecture guys can implement atomic_read however they want --- as long as it cannot be optimized away.*

They can implement it however they want as long as it stays atomic.

The compiler can also reorder non-volatile accesses. For an example patch that cares about this, please see:

	http://lkml.org/lkml/2007/8/7/280

This patch uses an ORDERED_WRT_IRQ() in rcu_read_lock() and rcu_read_unlock() to ensure that accesses aren't reordered with respect to interrupt handlers and NMIs/SMIs running on that same CPU.

Quoted 4 lines · Paul E. McKenney

The compiler can also reorder non-volatile accesses. For an example patch that cares about this, please see:

Quoted 5 lines

	http://lkml.org/lkml/2007/8/7/280

This patch uses an ORDERED_WRT_IRQ() in rcu_read_lock() and rcu_read_unlock() to ensure that accesses aren't reordered with respect to interrupt handlers and NMIs/SMIs running on that same CPU.

Good, finally we have some code to discuss (even though it's not actually in the kernel yet).

First of all, I think this illustrates that what you want here has nothing to do with atomic ops. The ORDERED_WRT_IRQ macro occurs a lot more times in your patch than atomic reads/sets. So *assuming* that it was necessary at all, then having an ordered variant of the atomic_read/atomic_set ops could do just as well.

However, I still don't know which atomic_read/atomic_set in your patch would be broken if there were no volatile. Could you please point them out?