[ovs-dev] [PATCH 03/12] lib/ovs-atomic: Add atomic compare_exchange.

[Ben Pfaff]

On Fri, Jul 04, 2014 at 06:03:53AM -0700, Jarno Rajahalme wrote:
> 
> On Jul 3, 2014, at 2:26 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > On Mon, Jun 30, 2014 at 08:17:19AM -0700, Jarno Rajahalme wrote:
> >> Add support for atomic compare_exchange operations.
> >> 
> >> Add ovs_refcount_try_ref(), which takes a reference only if the
> >> refcount is non-zero and returns true if a reference was taken, false
> >> otherwise.  This can be used in combined RCU/refcount scenarios where
> >> we have an RCU protected reference to an refcounted object, but which
> >> may be unref'ed at any time.  If ovs_refcount_try_ref() fails, the
> >> object may still be safely used until the current thread quiesces.
> >> 
> >> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> > 
> > I doubt that "relaxed" memory order is good enough here, given the
> > commentary on the previous patch.
> 
> I split this into two parts for v2, so that it is clear what we are
> referring to. The new ovs_refcount_try_ref() never unrefs, so it seems
> to me the commentary on taking a reference applies. Hence relaxed
> memory model should be appropriate. The caller already has protected
> access to the object via RCU, otherwise calling this on an object
> whose reference count is zero would be accessing freed memory. Maybe
> we should enforce RCU for this at least by naming the function as
> ?ovs_refcount_try_ref_rcu()??

OK, that makes sense.  I was reading the code without the mental context
that it was always used in an RCU environment, but that's the only place
that it really makes sense anyway.  I think that your naming suggestion
is appropriate, too.

> > Not every architecture supports compare-and-exchange, but it sounds
> > like probably the ones we care about do (do we need to support ARMv6
> > or PA-RISC?).
> 
> Do you know how GCC and clang are handling these? Do they provide some fallback implementations (maybe with locks?) for these platforms?
> 
>   Jarno
>