* [PATCH] locking/qrwlock: fix write unlock issue in big endian @ 2016-06-02 10:09 Pan Xinhui 2016-06-02 10:44 ` Arnd Bergmann 2016-06-08 9:22 ` Will Deacon 0 siblings, 2 replies; 13+ messages in thread From: Pan Xinhui @ 2016-06-02 10:09 UTC (permalink / raw) To: linux-arch, linux-kernel; +Cc: arnd, waiman.long, peterz, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 10:09 [PATCH] locking/qrwlock: fix write unlock issue in big endian Pan Xinhui @ 2016-06-02 10:44 ` Arnd Bergmann 2016-06-02 11:01 ` xinhui 2016-06-02 11:02 ` Peter Zijlstra 2016-06-08 9:22 ` Will Deacon 1 sibling, 2 replies; 13+ messages in thread From: Arnd Bergmann @ 2016-06-02 10:44 UTC (permalink / raw) To: Pan Xinhui; +Cc: linux-arch, linux-kernel, waiman.long, peterz [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 10:44 ` Arnd Bergmann @ 2016-06-02 11:01 ` xinhui 2016-06-02 11:15 ` Peter Zijlstra 2016-06-02 11:02 ` Peter Zijlstra 1 sibling, 1 reply; 13+ messages in thread From: xinhui @ 2016-06-02 11:01 UTC (permalink / raw) To: Arnd Bergmann; +Cc: linux-arch, linux-kernel, waiman.long, peterz 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 11:01 ` xinhui @ 2016-06-02 11:15 ` Peter Zijlstra 2016-06-03 7:20 ` xinhui 0 siblings, 1 reply; 13+ messages in thread From: Peter Zijlstra @ 2016-06-02 11:15 UTC (permalink / raw) To: xinhui; +Cc: Arnd Bergmann, linux-arch, linux-kernel, waiman.long 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 11:15 ` Peter Zijlstra @ 2016-06-03 7:20 ` xinhui 0 siblings, 0 replies; 13+ messages in thread From: xinhui @ 2016-06-03 7:20 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Arnd Bergmann, linux-arch, linux-kernel, waiman.long 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(). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 10:44 ` Arnd Bergmann 2016-06-02 11:01 ` xinhui @ 2016-06-02 11:02 ` Peter Zijlstra 2016-06-03 7:17 ` xinhui [not found] ` <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> 1 sibling, 2 replies; 13+ messages in thread From: Peter Zijlstra @ 2016-06-02 11:02 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Pan Xinhui, linux-arch, linux-kernel, waiman.long 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). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 11:02 ` Peter Zijlstra @ 2016-06-03 7:17 ` xinhui [not found] ` <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> 1 sibling, 0 replies; 13+ messages in thread From: xinhui @ 2016-06-03 7:17 UTC (permalink / raw) To: Peter Zijlstra, Arnd Bergmann; +Cc: linux-arch, linux-kernel, waiman.long 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com>]
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian [not found] ` <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> @ 2016-06-03 20:57 ` Waiman Long 2016-06-06 3:15 ` xinhui 0 siblings, 1 reply; 13+ messages in thread From: Waiman Long @ 2016-06-03 20:57 UTC (permalink / raw) To: xinhui Cc: Peter Zijlstra, Arnd Bergmann, linux-arch, linux-kernel, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-03 20:57 ` Waiman Long @ 2016-06-06 3:15 ` xinhui 0 siblings, 0 replies; 13+ messages in thread From: xinhui @ 2016-06-06 3:15 UTC (permalink / raw) To: Waiman Long Cc: Peter Zijlstra, Arnd Bergmann, linux-arch, linux-kernel, waiman.long 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-02 10:09 [PATCH] locking/qrwlock: fix write unlock issue in big endian Pan Xinhui 2016-06-02 10:44 ` Arnd Bergmann @ 2016-06-08 9:22 ` Will Deacon 2016-06-14 6:11 ` xinhui 1 sibling, 1 reply; 13+ messages in thread From: Will Deacon @ 2016-06-08 9:22 UTC (permalink / raw) To: Pan Xinhui; +Cc: linux-arch, linux-kernel, arnd, waiman.long, peterz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-08 9:22 ` Will Deacon @ 2016-06-14 6:11 ` xinhui 2016-06-14 10:40 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: xinhui @ 2016-06-14 6:11 UTC (permalink / raw) To: Will Deacon; +Cc: linux-arch, linux-kernel, arnd, waiman.long, peterz 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-14 6:11 ` xinhui @ 2016-06-14 10:40 ` Will Deacon 2016-06-15 3:47 ` xinhui 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2016-06-14 10:40 UTC (permalink / raw) To: xinhui; +Cc: linux-arch, linux-kernel, arnd, waiman.long, peterz 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] locking/qrwlock: fix write unlock issue in big endian 2016-06-14 10:40 ` Will Deacon @ 2016-06-15 3:47 ` xinhui 0 siblings, 0 replies; 13+ messages in thread From: xinhui @ 2016-06-15 3:47 UTC (permalink / raw) To: Will Deacon; +Cc: linux-arch, linux-kernel, arnd, waiman.long, peterz 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-06-15 3:47 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-02 10:09 [PATCH] locking/qrwlock: fix write unlock issue in big endian Pan Xinhui 2016-06-02 10:44 ` Arnd Bergmann 2016-06-02 11:01 ` xinhui 2016-06-02 11:15 ` Peter Zijlstra 2016-06-03 7:20 ` xinhui 2016-06-02 11:02 ` Peter Zijlstra 2016-06-03 7:17 ` xinhui [not found] ` <201606030718.u537FQg0009963@mx0a-001b2d01.pphosted.com> 2016-06-03 20:57 ` Waiman Long 2016-06-06 3:15 ` xinhui 2016-06-08 9:22 ` Will Deacon 2016-06-14 6:11 ` xinhui 2016-06-14 10:40 ` Will Deacon 2016-06-15 3:47 ` xinhui
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).