Messages in this thread | | | Date | Mon, 2 Jun 2014 14:12:30 -0700 | Subject | Re: [PATCH v2] introduce atomic_pointer to fix a race condition in cancelable mcs spinlocks | From | Linus Torvalds <> |
| |
On Mon, Jun 2, 2014 at 2:02 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > > In the ->qlen case, interrupts are disabled and the current CPU is > the only one who can write, so the read need not be volatile. In the > ->n_barrier_done, modifications are done holding ->barrier_mutex, so again > the read need not be volatile. In the sync_rcu_preempt_exp_count case, > modifications are done holding sync_rcu_preempt_exp_mutex, so once again, > the read need not be volatile. So I could do something like: > > ACCESS_ONCE(rdp->qlen) = rdp->qlen + 1; > > But that still makes gcc generate bad code > > The reason I was not all that worried about this is that these are not > in fastpaths, and the last two are especially not in fastpaths. > > Suggestions?
So I think it probably *works*, but even so splitting it up to use ACCESS_ONCE() on just the write is probably a better option, if only because it would then make it much easier to change if we do end up splitting reads and writes.
Because from a gcc code generation standpoint, using "volatile" will always be horrible, because gcc will never be able to turn it into a read-modify-write cycle. Arguable gcc _should_ be able to do that (it is certainly allowable within the virtual machine definition), but I understand why it doesn't ("volatile? Let's not optimize anything at all, because it's special").
So "ACCESS_ONCE() + R-M-W" operation is actually pretty much guaranteed to be "ACCESS_TWICE()", which may well be ok (performance may not matter, and even when it does most architectures don't actually have r-m-w instructions and when they do they aren't always even faster), but I think it's just horribly horribly bad from a conceptual and readability standpoint because it's so misleading.
So I'd actually rather see two explicit ACCESS_ONCE() calls - once to read, once to write. Because that at least describes what is happening, unlike the current situation.
Put another way: I can understand why you do it, and I can even agree that it is "correct" from a functionality standpoint. But even despite that all, I really don't like the construct very much..
Linus
| |