Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1412094 > unrolled thread

[PATCH] locking/qrwlock: fix write unlock issue in big endian

Back to article view | Back to linux.kernel

Contents

  [PATCH] locking/qrwlock: fix write unlock issue in big endian Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-02 12:10 +0200
    Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian Arnd Bergmann <arnd@arndb.de> - 2016-06-02 12:50 +0200
      Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-02 13:10 +0200
        Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian Peter Zijlstra <peterz@infradead.org> - 2016-06-02 13:20 +0200
          Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-03 09:30 +0200
      Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian Peter Zijlstra <peterz@infradead.org> - 2016-06-02 13:10 +0200
        Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-03 09:20 +0200
        Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-06 05:20 +0200
    Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian Will Deacon <will.deacon@arm.com> - 2016-06-08 11:30 +0200

#1412094 — [PATCH] locking/qrwlock: fix write unlock issue in big endian

strcut __qrwlock has different layout in big endian machine. we need set
the __qrwlock->wmode to NULL, and the address is not &lock->cnts in big
endian machine.

Do as what read unlock does. we are lucky that the __qrwlock->wmode's
val is _QW_LOCKED.

Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
---
 include/asm-generic/qrwlock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 54a8e65..eadd7a3 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
  */
 static inline void queued_write_unlock(struct qrwlock *lock)
 {
-	smp_store_release((u8 *)&lock->cnts, 0);
+	(void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
 }
 
 /*
-- 
1.9.1

[toc] | [next] | [standalone]

#1412113

[Multipart message — attachments visible in raw view] — view raw

On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> index 54a8e65..eadd7a3 100644
> --- a/include/asm-generic/qrwlock.h
> +++ b/include/asm-generic/qrwlock.h
> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
>   */
>  static inline void queued_write_unlock(struct qrwlock *lock)
>  {
> -       smp_store_release((u8 *)&lock->cnts, 0);
> +       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
>  }

Isn't this more expensive than the existing version?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1412126

On 2016年06月02日 18:44, Arnd Bergmann wrote:
> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
>> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
>> index 54a8e65..eadd7a3 100644
>> --- a/include/asm-generic/qrwlock.h
>> +++ b/include/asm-generic/qrwlock.h
>> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
>>    */
>>   static inline void queued_write_unlock(struct qrwlock *lock)
>>   {
>> -       smp_store_release((u8 *)&lock->cnts, 0);
>> +       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
>>   }
>
> Isn't this more expensive than the existing version?
>
yes, a little more expensive than the existing version
But does this is generic code, I am not sure how it will impact the performance on other archs.

If you like
we calculate the correct address to set to NULL
say,
static inline void queued_write_unlock(struct qrwlock *lock)
{
u8 *wl = lock;

#ifdef __BIG_ENDIAN
wl += 3;
#endif
smp_store_release(wl, 0);

}



> 	Arnd
>

[toc] | [prev] | [next] | [standalone]

#1412136

On Thu, Jun 02, 2016 at 07:01:17PM +0800, xinhui wrote:
> 
> On 2016年06月02日 18:44, Arnd Bergmann wrote:
> >On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
> >>diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> >>index 54a8e65..eadd7a3 100644
> >>--- a/include/asm-generic/qrwlock.h
> >>+++ b/include/asm-generic/qrwlock.h
> >>@@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
> >>   */
> >>  static inline void queued_write_unlock(struct qrwlock *lock)
> >>  {
> >>-       smp_store_release((u8 *)&lock->cnts, 0);
> >>+       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
> >>  }
> >
> >Isn't this more expensive than the existing version?
> >
> yes, a little more expensive than the existing version

Think 20+ cycles worse.

> But does this is generic code, I am not sure how it will impact the performance on other archs.

As always, you get to audit users of stuff you change. And here you're
lucky, there's only 1.

> If you like
> we calculate the correct address to set to NULL
> say,
> static inline void queued_write_unlock(struct qrwlock *lock)
> {
> u8 *wl = lock;
> 
> #ifdef __BIG_ENDIAN
> wl += 3;
> #endif
> smp_store_release(wl, 0);
> 
> }

No, that's horrible. Either lift __qrwlock into qrwlock_types.h or do
what qspinlock does. And looking at that, we could make
queued_spin_unlock() use the atomic_sub_return_relaxed() thing too I
suppose, that generates slightly better code.

[toc] | [prev] | [next] | [standalone]

#1412863

On 2016年06月02日 19:15, Peter Zijlstra wrote:
> On Thu, Jun 02, 2016 at 07:01:17PM +0800, xinhui wrote:
>>
>> On 2016年06月02日 18:44, Arnd Bergmann wrote:
>>> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
>>>> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
>>>> index 54a8e65..eadd7a3 100644
>>>> --- a/include/asm-generic/qrwlock.h
>>>> +++ b/include/asm-generic/qrwlock.h
>>>> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
>>>>    */
>>>>   static inline void queued_write_unlock(struct qrwlock *lock)
>>>>   {
>>>> -       smp_store_release((u8 *)&lock->cnts, 0);
>>>> +       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
>>>>   }
>>>
>>> Isn't this more expensive than the existing version?
>>>
>> yes, a little more expensive than the existing version
>
> Think 20+ cycles worse.
>
>> But does this is generic code, I am not sure how it will impact the performance on other archs.
>
> As always, you get to audit users of stuff you change. And here you're
> lucky, there's only 1.
>
yes, and hope there will be 2 :)

>> If you like
>> we calculate the correct address to set to NULL
>> say,
>> static inline void queued_write_unlock(struct qrwlock *lock)
>> {
>> u8 *wl = lock;
>>
>> #ifdef __BIG_ENDIAN
>> wl += 3;
>> #endif
>> smp_store_release(wl, 0);
>>
>> }
>
> No, that's horrible. Either lift __qrwlock into qrwlock_types.h or do
> what qspinlock does. And looking at that, we could make
agree.

> queued_spin_unlock() use the atomic_sub_return_relaxed() thing too I
> suppose, that generates slightly better code.
>
thanks for your suggestion.
I can have a try in queued_spin_unlock().

[toc] | [prev] | [next] | [standalone]

#1412128

On Thu, Jun 02, 2016 at 12:44:51PM +0200, Arnd Bergmann wrote:
> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
> > diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> > index 54a8e65..eadd7a3 100644
> > --- a/include/asm-generic/qrwlock.h
> > +++ b/include/asm-generic/qrwlock.h
> > @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
> >   */
> >  static inline void queued_write_unlock(struct qrwlock *lock)
> >  {
> > -       smp_store_release((u8 *)&lock->cnts, 0);
> > +       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
> >  }
> 
> Isn't this more expensive than the existing version?

Yes, loads. And while this might be a suitable fix for asm-generic, it
will introduce a fairly large regression on x86 (which is currently the
only user of this).

[toc] | [prev] | [next] | [standalone]

#1412856

On 2016年06月02日 19:02, Peter Zijlstra wrote:
> On Thu, Jun 02, 2016 at 12:44:51PM +0200, Arnd Bergmann wrote:
>> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
>>> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
>>> index 54a8e65..eadd7a3 100644
>>> --- a/include/asm-generic/qrwlock.h
>>> +++ b/include/asm-generic/qrwlock.h
>>> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
>>>    */
>>>   static inline void queued_write_unlock(struct qrwlock *lock)
>>>   {
>>> -       smp_store_release((u8 *)&lock->cnts, 0);
>>> +       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
>>>   }
>>
>> Isn't this more expensive than the existing version?
>
> Yes, loads. And while this might be a suitable fix for asm-generic, it
> will introduce a fairly large regression on x86 (which is currently the
> only user of this).
>
well, to show respect to struct __qrwlock private field.
We can keep smp_store_release((u8 *)&lock->cnts, 0) in little_endian machine.
as this should be quick and no performance issue to all other archs(although there is only 1 now)

BUT, We need use (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts) in big_endian machine.
because it's bad to export struct __qrwlock and set its private field to NULL.

How about code like below.

static inline void queued_write_unlock(struct qrwlock *lock)
{
#ifdef __BIG_ENDIAN
         (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
#else
	smp_store_release((u8 *)&lock->cnts, 0);
#endif
}

BUT I think that would make thing a little complex to understand. :(
So at last, in my opinion, I suggest my patch :)
any thoughts?

thanks
xinhui



  

[toc] | [prev] | [next] | [standalone]

#1414574

On 2016年06月04日 04:57, Waiman Long wrote:
> On 06/03/2016 03:17 AM, xinhui wrote:
>>
>> On 2016年06月02日 19:02, Peter Zijlstra wrote:
>>> On Thu, Jun 02, 2016 at 12:44:51PM +0200, Arnd Bergmann wrote:
>>>> On Thursday, June 2, 2016 6:09:08 PM CEST Pan Xinhui wrote:
>>>>> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
>>>>> index 54a8e65..eadd7a3 100644
>>>>> --- a/include/asm-generic/qrwlock.h
>>>>> +++ b/include/asm-generic/qrwlock.h
>>>>> @@ -139,7 +139,7 @@ static inline void queued_read_unlock(struct qrwlock *lock)
>>>>>    */
>>>>>   static inline void queued_write_unlock(struct qrwlock *lock)
>>>>>   {
>>>>> -       smp_store_release((u8 *)&lock->cnts, 0);
>>>>> +       (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
>>>>>   }
>>>>
>>>> Isn't this more expensive than the existing version?
>>>
>>> Yes, loads. And while this might be a suitable fix for asm-generic, it
>>> will introduce a fairly large regression on x86 (which is currently the
>>> only user of this).
>>>
>> well, to show respect to struct __qrwlock private field.
>> We can keep smp_store_release((u8 *)&lock->cnts, 0) in little_endian machine.
>> as this should be quick and no performance issue to all other archs(although there is only 1 now)
>>
>> BUT, We need use (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts) in big_endian machine.
>> because it's bad to export struct __qrwlock and set its private field to NULL.
>>
>> How about code like below.
>>
>> static inline void queued_write_unlock(struct qrwlock *lock)
>> {
>> #ifdef __BIG_ENDIAN
>>         (void)atomic_sub_return_release(_QW_LOCKED, &lock->cnts);
>> #else
>>     smp_store_release((u8 *)&lock->cnts, 0);
>> #endif
>> }
>>
>> BUT I think that would make thing a little complex to understand. :(
>> So at last, in my opinion, I suggest my patch :)
>> any thoughts?
>
> Another alternative is to make queued_write_unlock() overrideable from asm/qrwlock.h, just like what we did with queued_spin_unlock().
>
fair enough :)
And archs can write better code for themself.
I will send patch v2 with suggested-by of you. :)

thanks
xinhui

> Cheers,
> Longman
>

[toc] | [prev] | [next] | [standalone]

#1417144

On Thu, Jun 02, 2016 at 06:09:08PM +0800, Pan Xinhui wrote:
> strcut __qrwlock has different layout in big endian machine. we need set
> the __qrwlock->wmode to NULL, and the address is not &lock->cnts in big
> endian machine.
> 
> Do as what read unlock does. we are lucky that the __qrwlock->wmode's
> val is _QW_LOCKED.

Doesn't this have wider implications for the qrwlocks, for example:

  while ((cnts & _QW_WMASK) == _QW_LOCKED) { ... }

would actually end up looking at the wrong field of the lock?

Shouldn't we just remove the #ifdef __LITTLE_ENDIAN stuff from __qrwlock,
given that all the struct members are u8?

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web