On Fri, May 25, 2018 at 08:50:23AM -0400, Bob Copeland wrote: > On Fri, May 25, 2018 at 02:36:56PM +0200, Niklas Cassel wrote: > > A spin lock does have the advantage of ordering: memory operations issued > > before the spin_unlock_bh() will be completed before the spin_unlock_bh() > > operation has completed. > > > > However, ath10k_htt_tx_dec_pending() was called earlier in the same function, > > which decreases htt->num_pending_tx, so that write will be completed before > > our read. That is the only ordering we care about here (if we should call > > ath10k_mac_tx_push_pending() or not). > > Sure. I also understand that reading inside a lock and operating on the > value outside the lock isn't really the definition of synchronization > (doesn't really matter in this case though). > > I was just suggesting that the implicit memory barrier in the spin unlock > that we are already paying for would be sufficient here too, and it matches > the semantic of "tx fields under tx_lock." On the other hand, maybe it's > just me, but I tend to look askance at just-in-case READ_ONCEs sprinkled > about. I agree, because of the implicit memory barrier from spin_unlock_bh(), READ_ONCE shouldn't really be needed in this case. I think that it's a good thing to be critical of all "just-in-case" things, however, it's not always that obvious if you actually need READ_ONCE or not. E.g. you might need to use it even when you are holding a spin_lock. Some people recommend to use it for all concurrent non-read-only shared memory accesses: https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE Is there a better guideline somewhere..? Kind regards, Niklas
