Re: [PATCH 2/3] PowerPC: Add adaptive elision to rwlocks

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

Hi Will,

Thanks for the review, I will send a patchset update soon.  Below some
comments about your points.


On 10-12-2014 17:40, Will Schmidt wrote:
>
>> ---
>>
>> diff --git a/sysdeps/powerpc/nptl/elide.h b/sysdeps/powerpc/nptl/elide.h
>> new file mode 100644
>> index 0000000..5cc47a3
>> --- /dev/null
>> +++ b/sysdeps/powerpc/nptl/elide.h
>> @@ -0,0 +1,111 @@
>> +/* elide.h: Generic lock elision support for powerpc.
>> +   Copyright (C) 2014 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef ELIDE_PPC_H
>> +# define ELIDE_PPC_H
>> +
>> +#ifdef ENABLE_LOCK_ELISION
>> +# include <htm.h>
>> +# include <elision-conf.h>
>> +
>> +/* Returns true if lock defined by IS_LOCK_FREE was elided.
> s/if lock/if the lock/
> s/IS_LOCK_FREE/is_lock_free/
Fixed
>
>>> +   ADAPT_COUNT is a pointer to per-lock state variable. */
>
>>> +
>>> +static inline bool
>>> +__elide_lock (uint8_t *adapt_count, int is_lock_free)
>>> +{
>>> +  if (*adapt_count > 0)
>>> +    {
>>> +      (*adapt_count)--;
>>> +      return false;
>>> +    }
>>> +
>>> +  for (int i = __elision_aconf.try_tbegin; i > 0; i--)
>>> +    {
>>> +      if (__builtin_tbegin (0))
>>> +	{
>>> +	  if (is_lock_free)
>>> +	    return true;
>>> +	  /* Lock was busy.  */
>>> +	  __builtin_tabort (_ABORT_LOCK_BUSY);
>>> +	}
>>> +      else
>>> +	{
>>> +	  /* A persistent failure indicates that a retry will probably
>>> +	     result in another failure.  Use normal locking now and
>>> +	     for the next couple of calls.  */
>>> +	  if (_TEXASRU_FAILURE_PERSISTENT (__builtin_get_texasru ()))
>>> +	    {
>>> +	      if (__elision_aconf.skip_lock_internal_abort > 0)
>>> +		*adapt_count = __elision_aconf.skip_lock_internal_abort;
>>> +	      break;
>>> +	    }
>>> +	  /* Same logic as above, but for for a number of temporary failures in a
>>> +	     a row.  */
> s/for for/for/
> (looking back, I also see this at least once in [1/3] )
Fixed.
>
>
>>> +	  else if (__elision_aconf.skip_lock_out_of_tbegin_retries > 0
>>> +		   && __elision_aconf.try_tbegin > 0)
>>> +	    *adapt_count = __elision_aconf.skip_lock_out_of_tbegin_retries;
>>> +	}
>>> +     }
> tab or whitespace indentation glitch here?
I will check, but I think it using tab correctly here.