[OT] volatile keyword

[OT] volatile keyword

[Vadim Lobanov]

Hi,

The recent discussion on the list concerning memory barriers and write
ordering took a side-trip to the volatile keyword, especially its
correct / incorrect usage. Someone posted a link to the LKML archives,
in which the argument is made that it is best to keep 'volatile' _out_
of variable and structure definitions, and _into_ the code that uses
them. I was curious, so I decided to try this out (looking at
kernel/posix-timers.c for inspiration)...

Here's sample userland program number one, written one way:
===========================
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

struct sync {
    volatile unsigned long lock;
    volatile unsigned long value;
};

struct sync data;

void * thread (void * arg) {
    sleep(5);
    data.value = 0;
    data.lock = 0;

    return NULL;
}

int main (void) {
    pthread_t other;

    data.lock = 1;
    data.value = 1;
    pthread_create(&other, NULL, thread, NULL);
    while (data.lock);
    printf("Value is %lu.\n", data.value);
    pthread_join(other, NULL);

    return 0;
}
===========================

And here's what should be the same program, written the "suggested" way:
===========================
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

struct sync {
    unsigned long lock;
    unsigned long value;
};

struct sync data;

void * thread (void * arg) {
    sleep(5);
    data.value = 0;
    data.lock = 0;

    return NULL;
}

int main (void) {
    pthread_t other;

    data.lock = 1;
    data.value = 1;
    pthread_create(&other, NULL, thread, NULL);
    while ((volatile unsigned long)(data.lock));
    printf("Value is %lu.\n", data.value);
    pthread_join(other, NULL);

    return 0;
}
===========================

The first program works exactly as expected. The second program,
however, never syncs with the sleeping thread. In fact, for the second
program, gcc compiles the while loop down to an infinite loop.

I'm positive I'm doing something wrong here. In fact, I bet it's the
volatile cast within the loop that's wrong; but I'm not sure how to do
it correctly. Any help / pointers / discussion would be appreciated.

Thanks. :-)
-VadimL

Re: [OT] volatile keyword

[Christopher Friesen]

Vadim Lobanov wrote:

> I'm positive I'm doing something wrong here. In fact, I bet it's the
> volatile cast within the loop that's wrong; but I'm not sure how to do
> it correctly. Any help / pointers / discussion would be appreciated.

You need to cast is as dereferencing a volatile pointer.

Chris

Re: [OT] volatile keyword

[Vadim Lobanov]

On Thu, 25 Aug 2005, Christopher Friesen wrote:

> Vadim Lobanov wrote:
>
> > I'm positive I'm doing something wrong here. In fact, I bet it's the
> > volatile cast within the loop that's wrong; but I'm not sure how to do
> > it correctly. Any help / pointers / discussion would be appreciated.
>
> You need to cast is as dereferencing a volatile pointer.
>
> Chris
>

I figured it was something along these lines. In that case, is the
following code (from kernel/posix-timers.c) really doing the right
thing?

do
    expires = timr->it_timer.expires;
while ((volatile long) (timr->it_timer.expires) != expires);

Seems it's casting the value, not the pointer.

-VadimL

Re: [OT] volatile keyword

[Christopher Friesen]

Vadim Lobanov wrote:

> I figured it was something along these lines. In that case, is the
> following code (from kernel/posix-timers.c) really doing the right
> thing?
> 
> do
>     expires = timr->it_timer.expires;
> while ((volatile long) (timr->it_timer.expires) != expires);
> 
> Seems it's casting the value, not the pointer.

Someone else will have to give the definitive answer, but it looks 
suspicious to me...

Chris

Re: [OT] volatile keyword

[Paolo Ornati]

On Thu, 25 Aug 2005 13:44:55 -0700 (PDT)
Vadim Lobanov <vlobanov@speakeasy.net> wrote:

> int main (void) {
>     pthread_t other;
> 
>     data.lock = 1;
>     data.value = 1;
>     pthread_create(&other, NULL, thread, NULL);
>     while ((volatile unsigned long)(data.lock));
>     printf("Value is %lu.\n", data.value);
>     pthread_join(other, NULL);
> 
>     return 0;
> }

The "correct" way should be:

	while (*(volatile unsigned long*)(&data.lock));

With only: "while ((volatile unsigned long)(data.lock))" GCC isn't
forced to read to memory simply because "data.lock" isn't volatile.
What than you do with "data.lock" value doesn't change anything. IOW
you should get the same assembly code with and without the cast.


SUMMARY

"(volatile unsigned long)(data.lock)" means:
	- take the value of "data.lock" (that isn't volatile so can be
	cached)
	- cast it to "volatile" (a no-op, since we already HAVE the
	value)


"*(volatile unsigned long*)(&data.lock)":
	- take the address of "data.lock"
	- cast it to "volatile"
	- read from _memory_ the value of data.lock (through the
	volatile pointer)


Other ways can be:
	- use read memory barrier:
		while (data.lock)
			rmb();
	- use everything that implies a memory barrier (eg: locking...)


PS: everything I've said is rigorously NOT tested. :-)

-- 
	Paolo Ornati
	Linux 2.6.13-rc7 on x86_64

Re: [OT] volatile keyword

[Alan Cox]

On Iau, 2005-08-25 at 15:26 -0600, Christopher Friesen wrote:
> > do
> >     expires = timr->it_timer.expires;
> > while ((volatile long) (timr->it_timer.expires) != expires);
> > 
> > Seems it's casting the value, not the pointer.
> 
> Someone else will have to give the definitive answer, but it looks 
> suspicious to me...

It really ought to be using rmb() in this case not volatile casting

Alan