2004-04-16 18:24:59

by Brian King

[permalink] [raw]
Subject: Re: [PATCH][RELEASE]: megaraid unified driver version 2.20.0.B1

Jeff Garzik wrote:
> 6) Kill uses of "volatile". _Most_ of the time, this indicates buggy
> code. You should have the proper barriers in place: mb(), wmb(),
> rmb(), barrier(), and cpu_relax(). This has been mentioned before :)

I'm not sure I totally agree with this statement. I took a quick look at the driver
and the volatile appears to be used to point to host memory that is modified
by the adapter. Correct me if I am wrong, but memory barriers will not accomplish
what the volatile will in this situation.



--
Brian King
eServer Storage I/O
IBM Linux Technology Center


2004-04-16 18:30:57

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH][RELEASE]: megaraid unified driver version 2.20.0.B1

Brian King wrote:
> Jeff Garzik wrote:
>
>> 6) Kill uses of "volatile". _Most_ of the time, this indicates buggy
>> code. You should have the proper barriers in place: mb(), wmb(),
>> rmb(), barrier(), and cpu_relax(). This has been mentioned before :)
>
>
> I'm not sure I totally agree with this statement. I took a quick look at
> the driver
> and the volatile appears to be used to point to host memory that is
> modified
> by the adapter. Correct me if I am wrong, but memory barriers will not
> accomplish
> what the volatile will in this situation.


You are wrong :) Compare the underlying asm.

Proper use of barriers tells the compiler when it cannot cache certain
loads and stores in registers. 'volatile' is an atomic bomb to the
compiler, killing many valid optimizations while (sometimes) hiding
bugs. It almost always indicates that the programmer did not bother
thinking about when and where DMA memory needs to be accessed.

Jeff