Messages in this thread | | | Date | Fri, 15 Mar 2013 13:17:39 -0700 | From | "Paul E. McKenney" <> | Subject | Re: + atomic-improve-atomic_inc_unless_negative-atomic_dec_unless_positive .patch added to -mm tree |
| |
On Fri, Mar 15, 2013 at 07:34:32PM +0100, Frederic Weisbecker wrote: > 2013/3/15 Oleg Nesterov <oleg@redhat.com>: > > On 03/15, Frederic Weisbecker wrote: > >> > >> > The lack of the barrier? > >> > > >> > I thought about this, this should be fine? atomic_add_unless() has the same > >> > "problem", but this is documented in atomic_ops.txt: > >> > > >> > atomic_add_unless requires explicit memory barriers around the operation > >> > unless it fails (returns 0). > >> > > >> > I thought that atomic_add_unless_negative() should have the same > >> > guarantees? > >> > >> I feel very uncomfortable with that. The memory barrier is needed > >> anyway to make sure we don't deal with a stale value of the atomic val > >> (wrt. ordering against another object). > >> The following should really be expected to work without added barrier: > >> > >> void put_object(foo *obj) > >> { > >> if (atomic_dec_return(obj->ref) == -1) > >> free_rcu(obj); > >> } > >> > >> bool try_get_object(foo *obj) > >> { > >> if (atomic_add_unless_negative(obj, 1)) > >> return true; > >> return false; > >> } > >> > >> = CPU 0 = = CPU 1 > >> rcu_read_lock() > >> put_object(obj0); > >> obj = rcu_derefr(obj0); > >> rcu_assign_ptr(obj0, NULL); > > > > (I guess you meant rcu_assign_ptr() then put_object()) > > Right. > > > > >> if (try_get_object(obj)) > >> do_something... > >> else > >> object is dying > >> rcu_read_unlock() > > > > I must have missed something. > > > > do_something() looks fine, if atomic_add_unless_negative() succeeds > > we do have a barrier? > > Ok, I guess the guarantee of a barrier in case of failure is probably > not needed. But since the only way to safely read the atomic value is > a cmpxchg like operation, I guess a barrier must be involved in any > case. > > Using atomic_read() may return some stale value. > > > > > Anyway, I understand that it is possible to write the code which > > won't work without the uncoditional mb(). > > Yeah that's my fear. > > > > > My point was: should we fix atomic_add_unless() then? If not, why > > should atomic_add_unless_negative() differ? > > They shouldn't differ I guess.
Completely agreed. It is not like memory ordering is simple, so we should keep the rules simple. Atomic primitives that sometimes imply a memory barrier seems a bit over the top.
The rule is that if an atomic primitive returns non-void, then there is a full memory barrier before and after. This applies to primitives returning boolean as well, with atomic_dec_and_test() setting this precedent from what I can see.
Thanx, Paul
| |