This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCHv3] PowerPC: Fix a race condition when eliding a lock


On Tue, 2015-08-25 at 19:08 -0300, Tulio Magno Quites Machado Filho
wrote:
> Torvald Riegel <triegel@redhat.com> writes:
> 
> > On Mon, 2015-08-24 at 15:11 -0300, Tulio Magno Quites Machado Filho
> >> +   threads, either during lock acquisition, or elision.  In order to avoid
> >> +   this evaluation from becoming a data race the access of is_lock_free
> >
> > It could be a data race because you're not using atomics there, but
> > that's not the whole point.  (We use the term "data race" specifically
> > to refer to the C11/C++11 memory model concept of the same name.)
> > You want to ensure the lock elision synchronization scheme, and thus are
> > moving it inside the txn.
> 
> Are you complaining about the usage of the term "data race"?

Yes.

> If so, what about "race condition"?

Well, that would be better, but a race condition is not necessarily a
bad thing.  It's usually better to say which execution or interleaving
you need to avoid, than just saying "race condition".

> >> +   is placed *inside* the transaction.  Within the transaction is_lock_free
> >> +   can be accessed with relaxed ordering.  We are assured by the transaction
> >
> > Please use "memory order" to be specific, or abbreviate with MO as
> > mentioned on the Concurrency wiki page.
> 
> Ack.
> 
> >> +   that the value of is_lock_free is consistent with the state of the lock,
> >> +   otherwise the hardware will abort the transaction.  */
> >
> > That's not the really why this lock elisions synchronization scheme
> > works :)  (At least you're not giving sufficient reason why it is.)
> 
> I do agree with you that comment needs some adjustments but its intention is
> only to describe the concurrency details of the next macro.  It has no intention
> of describing how lock elision or this macro work.
> It was added per request [1] [2].
>
> What do you think about the following?
> 
>    Within the transaction we are assured that all memory accesses are atomic
>    and is_lock_free can be evaluated with relaxed memory order.  That way, the
>    value of is_lock_free is consistent with the state of the lock until the
>    end of the transaction.

That's good enough for this patch in that it hints at the right thing.  

I'd still like to see this getting improved eventually, and I'd
appreciate if you could participate in that.  A shared comment for both
the Intel and IBM lock elision implementations would be useful, I
believe, as the general principle used in both is the same.
I'd like to see this documented properly just so that future glibc
developers know how to work with this, and that it get's less likely
that bugs are introduced in the future.

> >> +		{							\
> >> +		  ret = 1;						\
> >> +		  break;						\
> >> +		}							\
> >> +	      __builtin_tabort (_ABORT_LOCK_BUSY);			\
> >> +	    }								\
> >> +	  else								\
> >> +	    if (!__get_new_count(&adapt_count))				\
> >> +	      break;							\
> >> +	}								\
> >> +    ret;								\
> >> +  })
> >>  
> >>  # define ELIDE_TRYLOCK(adapt_count, is_lock_free, write)	\
> >> -  __elide_trylock (&(adapt_count), is_lock_free, write)
> >> +  ({								\
> >> +    int ret = 0;						\
> >> +    if (__elision_aconf.try_tbegin > 0)				\
> >> +      {								\
> >
> > I'd prefer a comment here why you abort if write is true.  Or a
> > reference to another source file / function where this is explained.
> > Can be in a follow-up patch.
> 
> Let's leave this to another patch.  I want to unblock this bugfix.  ;-)

OK.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]