possible scsi driver bugs with atomic_set/atomic_read and missing barrier

possible scsi driver bugs with atomic_set/atomic_read and missing barrier

[Mike Christie]

Hi,

A couple scsi drivers will use a atomic_t for some host/device state.

They will do:

atomic_set(&hba->state, SOME_STATE_VALUE);

in a interrupt or thread or tasklet then in another thread they will do


if (atomic_read(&hba->state) == SOME_STATE_VALUE))



In the Documentation/atomic_ops.txt it says:

	atomic_read does not guarantee that the runtime
	initialization by any other thread is visible yet, so the user of the
	interface must take care of that with a proper implicit or 	explicit memory
	barrier.


Does this mean that the drivers should be doing a


atomic_set(&hba->state, SOME_STATE_VALUE);
smp_mb();

Re: possible scsi driver bugs with atomic_set/atomic_read and missing barrier

[Matthew Wilcox]

On Tue, Mar 10, 2009 at 10:25:44PM -0500, Mike Christie wrote:
> atomic_set(&hba->state, SOME_STATE_VALUE);
> 
> in a interrupt or thread or tasklet then in another thread they will do
> 
> if (atomic_read(&hba->state) == SOME_STATE_VALUE))
> 
> Does this mean that the drivers should be doing a
> 
> atomic_set(&hba->state, SOME_STATE_VALUE);
> smp_mb();

Possibly "smp_mb__after_atomic_inc()" might be correct.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: possible scsi driver bugs with atomic_set/atomic_read and missing barrier

[Stefan Richter]

Mike Christie wrote:
...
> In the Documentation/atomic_ops.txt it says:
> 
> 	atomic_read does not guarantee that the runtime
> 	initialization by any other thread is visible yet, so the user of the
> 	interface must take care of that with a proper implicit or
> 	explicit memory barrier.
> 
> Does this mean that the drivers should be doing a
> 
> atomic_set(&hba->state, SOME_STATE_VALUE);
> smp_mb();

Barriers --- or locks even --- are required if there are dependencies
between the state variable and other data.

(Use a barrier if you need to ensure ordering of accesses.  Use a lock
if you need to combine multiple operations into an atomic whole.  Lock/
unlock also imply barriers.)
-- 
Stefan Richter
-=====-==--= --== -=-==
http://arcgraph.de/sr/

Re: possible scsi driver bugs with atomic_set/atomic_read and missing barrier

[Stefan Richter]

Matthew Wilcox wrote:
> On Tue, Mar 10, 2009 at 10:25:44PM -0500, Mike Christie wrote:
>> atomic_set(&hba->state, SOME_STATE_VALUE);
>> 
>> in a interrupt or thread or tasklet then in another thread they will do
>> 
>> if (atomic_read(&hba->state) == SOME_STATE_VALUE))
>> 
>> Does this mean that the drivers should be doing a
>> 
>> atomic_set(&hba->state, SOME_STATE_VALUE);
>> smp_mb();
> 
> Possibly "smp_mb__after_atomic_inc()" might be correct.

Is this guaranteed to work with anything else than atomic_inc?

Mike,
it's also important to remember that barriers will be needed at the
reader place too (/if/ there actually are ordering requirements, that is).

E.g. in the writer:

	atomic_set(&hba->x, new_x);
	smp_wmb();
	hba->y = new_y;

And in the reader:

	y = hba->y;
	smp_rmb();
	x = atomic_read(&hba->x);

	do_something(x, y);

You would do this if you have a requirement that a reader needs 'x' to
always be at least as new as 'y', IOW would work incorrectly if an
outdated x would be used together with a current y.

This only works though if the reader can safely operate on an outdated
'y' + a current 'x'.  If neither current x + old y nor old x + current y
can be safely combined, then you need to wrap their write and read
accesses into lock- or mutex- protected sections.
-- 
Stefan Richter
-=====-==--= --== -=-==
http://arcgraph.de/sr/