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

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

[Pan Xinhui]

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

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

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

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

[-- Attachment #2: org.kde.konsole.desktop --]
[-- Type: application/x-desktop, Size: 6368 bytes --]

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

[xinhui]

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
>

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

[Peter Zijlstra]

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).

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

[Peter Zijlstra]

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.

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

[xinhui]

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



  

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

[xinhui]

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().

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

[Waiman Long]

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().

Cheers,
Longman

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

[xinhui]

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
>

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

[Will Deacon]

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

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

[xinhui]

On 2016年06月08日 17:22, Will Deacon wrote:
> 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?
>
I does not clearly understand your idea. :(
the condition in the while() is always true from the view of current code.
BUT if __qrwlock has same layout on the two endian machine, the while() will end up. :)


> Shouldn't we just remove the #ifdef __LITTLE_ENDIAN stuff from __qrwlock,
> given that all the struct members are u8?
>
No. that makes codes complex. for example

struct __qrwlock lock;

WRITE_ONCE(lock->wmode, _QW_WAITING);
if (atomic_(&lock->cnts) == _QW_WAITING) {
	do_something();
}

IF you remove the  #ifdef __LITTLE_ENDIAN stuff from __qrwlock.
codes above obviously will break. And we already have such code.

thanks
xinhui

> Will
>

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

[Will Deacon]

On Tue, Jun 14, 2016 at 02:11:48PM +0800, xinhui wrote:
> 
> On 2016年06月08日 17:22, Will Deacon wrote:
> >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?
> >
> I does not clearly understand your idea. :(

That's because I'm talking rubbish :) Sorry, I completely confused myself.
Locking is bad enough on its own, but add big-endian to the mix and I'm
all done.

> >Shouldn't we just remove the #ifdef __LITTLE_ENDIAN stuff from __qrwlock,
> >given that all the struct members are u8?
> >
> No. that makes codes complex. for example
> 
> struct __qrwlock lock;
> 
> WRITE_ONCE(lock->wmode, _QW_WAITING);
> if (atomic_(&lock->cnts) == _QW_WAITING) {
> 	do_something();
> }
> 
> IF you remove the  #ifdef __LITTLE_ENDIAN stuff from __qrwlock.
> codes above obviously will break. And we already have such code.

I was wondering more along the lines of having one definition of the data
structure, but then defining _QW_* differently depending on endianness
(i.e. add a << 24 when big-endian). That way queued_write_unlock can
stay like it is (having an arch override to handle the big-endian case
is incredibly ugly).

Will

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

[xinhui]

On 2016年06月14日 18:40, Will Deacon wrote:
> On Tue, Jun 14, 2016 at 02:11:48PM +0800, xinhui wrote:
>>
>> On 2016年06月08日 17:22, Will Deacon wrote:
>>> 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?
>>>
>> I does not clearly understand your idea. :(
>
> That's because I'm talking rubbish :) Sorry, I completely confused myself.
> Locking is bad enough on its own, but add big-endian to the mix and I'm
> all done.
>
>>> Shouldn't we just remove the #ifdef __LITTLE_ENDIAN stuff from __qrwlock,
>>> given that all the struct members are u8?
>>>
>> No. that makes codes complex. for example
>>
>> struct __qrwlock lock;
>>
>> WRITE_ONCE(lock->wmode, _QW_WAITING);
>> if (atomic_(&lock->cnts) == _QW_WAITING) {
>> 	do_something();
>> }
>>
>> IF you remove the  #ifdef __LITTLE_ENDIAN stuff from __qrwlock.
>> codes above obviously will break. And we already have such code.
>
> I was wondering more along the lines of having one definition of the data
> structure, but then defining _QW_* differently depending on endianness
> (i.e. add a << 24 when big-endian). That way queued_write_unlock can
make sense. And I review all the code, there is not much code to be changed.
I will work out one patch based on your idea :)

> stay like it is (having an arch override to handle the big-endian case
> is incredibly ugly).
>
I admit that. HOWEVER from the view of performance, having an arch override is acceptable.

thanks
xinhui
> Will
>