Page 1 of 1
one puzzle about function 'atomic_read' in linux source code
Posted: Sun Dec 29, 2013 11:26 am
by 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'?
Re: one puzzle about function 'atomic_read' in linux source
Posted: Sun Dec 29, 2013 7:18 pm
by 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.
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 12:08 am
by 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.
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 1:47 am
by 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
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 5:49 am
by Antti
miaowei wrote: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.
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 6:07 am
by iansjack
So how would you split that line up to make it easier to read (whilst retaining the same functionality)?
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 7:22 am
by 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.
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 3:10 pm
by 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:
If that create issue from optimizer, the problem should be fixed elsewhere but not making hacks that render the code un-readable.
Re: one puzzle about function 'atomic_read' in linux source
Posted: Mon Dec 30, 2013 8:44 pm
by 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 }
Re: one puzzle about function 'atomic_read' in linux source
Posted: Tue Dec 31, 2013 1:10 am
by miaowei
lihawl wrote: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.
Re: one puzzle about function 'atomic_read' in linux source
Posted: Thu Nov 03, 2016 9:42 pm
by 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.