possible scsi driver bugs with atomic_set/atomic_read and missing barrier
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();
Quoted 10 lines · Mike Christie
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.
Signature · 4 lines
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."
Quoted 5 lines · Matthew Wilcox
Quoted 4 lines · Mike Christie
atomic_set(&hba->state, SOME_STATE_VALUE);in a interrupt or thread or tasklet then in another thread they will do
Quoted 8 lines
Quoted 6 lines
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.
Signature · 3 lines
Stefan Richter -=====-==--= --== -=-== http://arcgraph.de/sr/
Mike Christie wrote:
...
Quoted 11 lines
In the
Documentation/atomic_ops.txtit says:
atomic_readdoes 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.)
Signature · 3 lines
Stefan Richter -=====-==--= --== -=-== http://arcgraph.de/sr/