| Message ID | 20211207094337.59300-1-cosmin.tanislav@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add ADXL367 driver | expand |
On 12/7/21 10:43 AM, Cosmin Tanislav wrote: > I have one question that is not actually specific to this driver but would > help me clear up some issues. > > I used mutex_lock and mutex_unlock when accessing anything in driver's > state that could potentially be written by another process in parallel. > > I heard mixed opinions about this. Some people said that it is not > necessary to lock everywhere because loads and stores for data with size > smaller or equal than register size would be done in one single atomic > instruction. > > On the other hand, I also heard that this is not true unless WRITE_ONCE > and READ_ONCE is used. > > It felt weird using WRITE_ONCE and READ_ONCE in this driver, so I kept > using mutexes. > > Could I get some opinions on this matter? What you wrote sums it up very well. READ_ONCE/WRITE_ONCE are required for correctness when no lock is used. The compiler is allowed to do all sorts of optimizations that could break multi-threading, when READ_ONCE/WRITE_ONCE is not used. E.g. if (x) foo->bar = 10; else foo->bar = 20; Could be implemented as foo->bar = 20; if (x) foo->bar = 10; In the absence of multi-threading the result will be the same. But if another thread reads foo->bar just at the right time it will read the incorrect 20. For simple things like `foo->bar = x;` it is unlikely that the compiler will do anything other than the single store. But it could and the code is not correct without the WRITE_ONCE. Using a mutex is OK, since non of this is performance critical.
> From: Lars-Peter Clausen <lars@metafoo.de> > Sent: Monday, December 13, 2021 12:34 PM > To: Cosmin Tanislav <demonsingur@gmail.com> > Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Hennerich, > Michael <Michael.Hennerich@analog.com>; Rob Herring > <robh+dt@kernel.org>; linux-iio@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Add ADXL367 driver > > [External] > > On 12/7/21 10:43 AM, Cosmin Tanislav wrote: > > I have one question that is not actually specific to this driver but > would > > help me clear up some issues. > > > > I used mutex_lock and mutex_unlock when accessing anything in > driver's > > state that could potentially be written by another process in parallel. > > > > I heard mixed opinions about this. Some people said that it is not > > necessary to lock everywhere because loads and stores for data with > size > > smaller or equal than register size would be done in one single > atomic > > instruction. > > > > On the other hand, I also heard that this is not true unless > WRITE_ONCE > > and READ_ONCE is used. > > > > It felt weird using WRITE_ONCE and READ_ONCE in this driver, so I > kept > > using mutexes. > > > > Could I get some opinions on this matter? > > What you wrote sums it up very well. READ_ONCE/WRITE_ONCE are > required > for correctness when no lock is used. The compiler is allowed to do all > sorts of optimizations that could break multi-threading, when > READ_ONCE/WRITE_ONCE is not used. E.g. > > if (x) > foo->bar = 10; > else > foo->bar = 20; > > Could be implemented as > > foo->bar = 20; > if (x) > foo->bar = 10; This example can even be more trickier than simple {WRITE|READ}_ONCE (not sure though) as we have a control dependency and compilers not always respect them apparently [but this is out of scope :D]... > In the absence of multi-threading the result will be the same. But if > another thread reads foo->bar just at the right time it will read the > incorrect 20. > > For simple things like `foo->bar = x;` it is unlikely that the compiler > will do anything other than the single store. But it could and the code > is not correct without the WRITE_ONCE. True and things like load/store tearing were already seen in the wild according to: https://lwn.net/Articles/793253/ Some time ago I was wondering if this could still be an issue for single byte stores and loads. Maybe for that case it's not but better not to assume we know what the compiler will do. The next bullet sums things pretty well and is a very nice guideline :) https://elixir.bootlin.com/linux/latest/source/Documentation/memory-barriers.txt#L269 - Nuno Sá