one puzzle about function 'atomic_read' in linux source code

[miaowei]

the following excerpt comes from linux source code(x86/asm/include/atomic.h):
------------------------
static inline int atomic_read(const atomic_t *v)
{
return (*(volatile int *)&(v)->counter);
}
------------------------

must it be so complex? Can not we just write 'return v->counter'?

[Nable]

No, one cannot simplify this construction because compliler can inline functions and do some further optimizations (for example, reducing loop of reads of the same address to one read operation). 'volatile' keyword is required to ensure that all reads/write operations will be actually performed without any coalescing and reordering.

Btw, you could try to find the meaning of 'volatile' on your own.

[miaowei]

I know some abount 'volatile', but have you noticed that the declaration of type 'atomic_t'.
-------------
typedef struct{ volatile int counter; } atomic_t;
-------------
the structure member we are handling is already 'volatile'. Why must we add (volatile int *) again?
Could you give me an example that what bad thing will happen when we just write 'return v->counter'?
Thanks.

[iansjack]

It's all to do with preventing errors in optimisation, and is explained in the kernel documentation. Here is the patch to the documentation where that aspect is explained.

http://www.spinics.net/lists/linux-arch/msg02396.html

[Antti]

must it be so complex?

Yes. Linux kernel developers appreciate complex code. Instead of splitting it to simple lines that are easy to read, they want to use complex code to underline their programming skills. Of course, they deny this but it is still true.

[iansjack]

So how would you split that line up to make it easier to read (whilst retaining the same functionality)?

[Antti]

Code: Select all

static inline int atomic_read(const atomic_t *v)
{
	volatile int *counter = (volatile int *)&v->counter;

	return (*counter);
}

It is a tough one and I do not think this is any better. My previous post was more like a general note. I prefer simplicity even if it were more verbose. Simple statements are easier to follow and extra variables are not something to be afraid of.

[bluemoon]

I noticed the const modifier is lost between de-reference and referencing.
Perhaps compiler is smart enough to forgive that, but i think it should be:

Code: Select all

return (*(const volatile int *) &(v->counter) );

but since count is already declared as volatile int, unless you got a broken compiler, you may as well just do:

Code: Select all

return v->counter;

If that create issue from optimizer, the problem should be fixed elsewhere but not making hacks that render the code un-readable.

[lihawl]

Where did you find the definition of the atomic_t type? There is a definition at http://lxr.free-electrons.com/source/in ... pes.h#L177. And I copy it here

Code: Select all

175 typedef struct {
176         int counter;
177 } atomic_t;

The volatile has been removed from the declaration of counter.

But strange thing is here ：
Let we see definition of atomic_set() (http://lxr.free-electrons.com/source/ar ... omic.h#L23).

Code: Select all

 35 static inline void atomic_set(atomic_t *v, int i)
 36 {
 37         v->counter = i;//Why there is no volatile here?
 38 }

BTW:

Code: Select all

 23 static inline int atomic_read(const atomic_t *v)
 24 {
 25         return (*(volatile int *)&(v)->counter);
 26 }

[miaowei]

Where did you find the definition of the atomic_t type?

I met the definition in 《Linux Kernel Development》3rd written by Roberet Love which is based on version 2.6。
However,On my ubuntu 12.1:
wws@bogon:/usr/src/linux-headers-3.5.0-17/include$ cat linux/types.h
typedef struct {
int counter;
} atomic_t;

The volatile has been removed from the declaration of counter.

this may be a good explanation,no wonder the code use (volatile int *) to force a type convertion.

But strange thing is here ：
Let we see definition of atomic_set() (http://lxr.free-electrons.com/source/ar ... omic.h#L23).

Code: Select all

 35 static inline void atomic_set(atomic_t *v, int i)
 36 {
 37         v->counter = i;//Why there is no volatile here?
 38 }
[/quote]
Yes, this is another question. I write my opinion here:
the compiler's optimization will only do damage to the read procedure of a non-volatile variable,but not the write procedure. since 'counter' in structure atomic_t is declared as non-volatile type, We should avoid the compiler optimizing 'counter' to a register and always read from that.Write is different, gcc knows 'counter' is a memory location, it can write 'i' to a register as buffer,  that's its freedom,but it dares not avoid write it back to memory position where v->counter locates.

[miaowei]

I read this post 3 years later.
And I find a small mistake in your words.
==================================(
'volatile' keyword is required to ensure that all reads/write operations will be actually performed without any coalescing and reordering.
==================================)
volatile doesn't prevent the compiler to reorder the instruction.