diff mbox series

[v2,06/28] locking/rwlocks: Add contention detection for rwlocks

Message ID 20210202185734.1680553-7-bgardon@google.com (mailing list archive)
State New, archived
Headers show
Series Allow parallel MMU operations with TDP MMU | expand

Commit Message

Ben Gardon Feb. 2, 2021, 6:57 p.m. UTC 
rwlocks do not currently have any facility to detect contention
like spinlocks do. In order to allow users of rwlocks to better manage
latency, add contention detection for queued rwlocks.

CC: Ingo Molnar <mingo@redhat.com>
CC: Will Deacon <will@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Davidlohr Bueso <dbueso@suse.de>
Acked-by: Waiman Long <longman@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 include/asm-generic/qrwlock.h | 24 ++++++++++++++++++------
 include/linux/rwlock.h        |  7 +++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

Comments

Guenter Roeck Feb. 9, 2021, 8:39 p.m. UTC | #1 
On Tue, Feb 02, 2021 at 10:57:12AM -0800, Ben Gardon wrote:
> rwlocks do not currently have any facility to detect contention
> like spinlocks do. In order to allow users of rwlocks to better manage
> latency, add contention detection for queued rwlocks.
> 
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Will Deacon <will@kernel.org>
> Acked-by: Peter Zijlstra <peterz@infradead.org>
> Acked-by: Davidlohr Bueso <dbueso@suse.de>
> Acked-by: Waiman Long <longman@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ben Gardon <bgardon@google.com>

When building mips:defconfig, this patch results in:

Error log:
In file included from include/linux/spinlock.h:90,
                 from include/linux/ipc.h:5,
                 from include/uapi/linux/sem.h:5,
                 from include/linux/sem.h:5,
                 from include/linux/compat.h:14,
                 from arch/mips/kernel/asm-offsets.c:12:
arch/mips/include/asm/spinlock.h:17:28: error: redefinition of 'queued_spin_unlock'
   17 | #define queued_spin_unlock queued_spin_unlock
      |                            ^~~~~~~~~~~~~~~~~~
arch/mips/include/asm/spinlock.h:22:20: note: in expansion of macro 'queued_spin_unlock'
   22 | static inline void queued_spin_unlock(struct qspinlock *lock)
      |                    ^~~~~~~~~~~~~~~~~~
In file included from include/asm-generic/qrwlock.h:17,
                 from ./arch/mips/include/generated/asm/qrwlock.h:1,
                 from arch/mips/include/asm/spinlock.h:13,
                 from include/linux/spinlock.h:90,
                 from include/linux/ipc.h:5,
                 from include/uapi/linux/sem.h:5,
                 from include/linux/sem.h:5,
                 from include/linux/compat.h:14,
                 from arch/mips/kernel/asm-offsets.c:12:
include/asm-generic/qspinlock.h:94:29: note: previous definition of 'queued_spin_unlock' was here
   94 | static __always_inline void queued_spin_unlock(struct qspinlock *lock)
      |                             ^~~~~~~~~~~~~~~~~~

Bisect log attached.

Guenter

---
# bad: [a4bfd8d46ac357c12529e4eebb6c89502b03ecc9] Add linux-next specific files for 20210209
# good: [92bf22614b21a2706f4993b278017e437f7785b3] Linux 5.11-rc7
git bisect start 'HEAD' 'v5.11-rc7'
# good: [a8eb921ba7e8e77d994a1c6c69c8ef08456ecf53] Merge remote-tracking branch 'crypto/master'
git bisect good a8eb921ba7e8e77d994a1c6c69c8ef08456ecf53
# good: [21d507c41bdf83f6afc0e02976e43c10badfc6cd] Merge remote-tracking branch 'spi/for-next'
git bisect good 21d507c41bdf83f6afc0e02976e43c10badfc6cd
# bad: [30cd4c688a3bcf324f011d7716044b1a4681efc1] Merge remote-tracking branch 'soundwire/next'
git bisect bad 30cd4c688a3bcf324f011d7716044b1a4681efc1
# bad: [c43d2173d3eb4047bb62a7a393a298a1032cce18] Merge remote-tracking branch 'drivers-x86/for-next'
git bisect bad c43d2173d3eb4047bb62a7a393a298a1032cce18
# good: [973e9d8622a6fecc52f639680cbbde1519e1fcf8] Merge remote-tracking branch 'rcu/rcu/next'
git bisect good 973e9d8622a6fecc52f639680cbbde1519e1fcf8
# bad: [7b2aaf51d499e0372cbecafad04582c71ad03c73] Merge remote-tracking branch 'kvm/next'
git bisect bad 7b2aaf51d499e0372cbecafad04582c71ad03c73
# good: [04548ed0206ca895c8edd6a078c20a218423890b] KVM: SVM: Replace hard-coded value with #define
git bisect good 04548ed0206ca895c8edd6a078c20a218423890b
# bad: [92f4d400a407235783afd4399fa26c4c665024b5] KVM: x86/xen: Fix __user pointer handling for hypercall page installation
git bisect bad 92f4d400a407235783afd4399fa26c4c665024b5
# good: [ed5e484b79e8a9b8be714bd85b6fc70bd6dc99a7] KVM: x86/mmu: Ensure forward progress when yielding in TDP MMU iter
git bisect good ed5e484b79e8a9b8be714bd85b6fc70bd6dc99a7
# bad: [f3d4b4b1dc1c5fb9ea17cac14133463bfe72f170] sched: Add cond_resched_rwlock
git bisect bad f3d4b4b1dc1c5fb9ea17cac14133463bfe72f170
# good: [f1b3b06a058bb5c636ffad0afae138fe30775881] KVM: x86/mmu: Clear dirtied pages mask bit before early break
git bisect good f1b3b06a058bb5c636ffad0afae138fe30775881
# bad: [26128cb6c7e6731fe644c687af97733adfdb5ee9] locking/rwlocks: Add contention detection for rwlocks
git bisect bad 26128cb6c7e6731fe644c687af97733adfdb5ee9
# good: [7cca2d0b7e7d9f3cd740d41afdc00051c9b508a0] KVM: x86/mmu: Protect TDP MMU page table memory with RCU
git bisect good 7cca2d0b7e7d9f3cd740d41afdc00051c9b508a0
# first bad commit: [26128cb6c7e6731fe644c687af97733adfdb5ee9] locking/rwlocks: Add contention detection for rwlocks
Waiman Long Feb. 9, 2021, 9:46 p.m. UTC | #2 
On 2/9/21 3:39 PM, Guenter Roeck wrote:
> On Tue, Feb 02, 2021 at 10:57:12AM -0800, Ben Gardon wrote:
>> rwlocks do not currently have any facility to detect contention
>> like spinlocks do. In order to allow users of rwlocks to better manage
>> latency, add contention detection for queued rwlocks.
>>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Will Deacon <will@kernel.org>
>> Acked-by: Peter Zijlstra <peterz@infradead.org>
>> Acked-by: Davidlohr Bueso <dbueso@suse.de>
>> Acked-by: Waiman Long <longman@redhat.com>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Ben Gardon <bgardon@google.com>
> When building mips:defconfig, this patch results in:
>
> Error log:
> In file included from include/linux/spinlock.h:90,
>                   from include/linux/ipc.h:5,
>                   from include/uapi/linux/sem.h:5,
>                   from include/linux/sem.h:5,
>                   from include/linux/compat.h:14,
>                   from arch/mips/kernel/asm-offsets.c:12:
> arch/mips/include/asm/spinlock.h:17:28: error: redefinition of 'queued_spin_unlock'
>     17 | #define queued_spin_unlock queued_spin_unlock
>        |                            ^~~~~~~~~~~~~~~~~~
> arch/mips/include/asm/spinlock.h:22:20: note: in expansion of macro 'queued_spin_unlock'
>     22 | static inline void queued_spin_unlock(struct qspinlock *lock)
>        |                    ^~~~~~~~~~~~~~~~~~
> In file included from include/asm-generic/qrwlock.h:17,
>                   from ./arch/mips/include/generated/asm/qrwlock.h:1,
>                   from arch/mips/include/asm/spinlock.h:13,
>                   from include/linux/spinlock.h:90,
>                   from include/linux/ipc.h:5,
>                   from include/uapi/linux/sem.h:5,
>                   from include/linux/sem.h:5,
>                   from include/linux/compat.h:14,
>                   from arch/mips/kernel/asm-offsets.c:12:
> include/asm-generic/qspinlock.h:94:29: note: previous definition of 'queued_spin_unlock' was here
>     94 | static __always_inline void queued_spin_unlock(struct qspinlock *lock)
>        |                             ^~~~~~~~~~~~~~~~~~

I think the compile error is caused by the improper header file 
inclusion ordering. Can you try the following change to see if it can 
fix the compile error?

Cheers,
Longman

diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 0020d3b820a7..d7178a9439b5 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -10,11 +10,11 @@
  #define __ASM_GENERIC_QRWLOCK_H

  #include <linux/atomic.h>
+#include <linux/spinlock.h>
  #include <asm/barrier.h>
  #include <asm/processor.h>

  #include <asm-generic/qrwlock_types.h>
-#include <asm-generic/qspinlock.h>

  /*
   * Writer states & reader shift and bias.
Guenter Roeck Feb. 9, 2021, 10:25 p.m. UTC | #3 
On Tue, Feb 09, 2021 at 04:46:02PM -0500, Waiman Long wrote:
> On 2/9/21 3:39 PM, Guenter Roeck wrote:
> > On Tue, Feb 02, 2021 at 10:57:12AM -0800, Ben Gardon wrote:
> > > rwlocks do not currently have any facility to detect contention
> > > like spinlocks do. In order to allow users of rwlocks to better manage
> > > latency, add contention detection for queued rwlocks.
> > > 
> > > CC: Ingo Molnar <mingo@redhat.com>
> > > CC: Will Deacon <will@kernel.org>
> > > Acked-by: Peter Zijlstra <peterz@infradead.org>
> > > Acked-by: Davidlohr Bueso <dbueso@suse.de>
> > > Acked-by: Waiman Long <longman@redhat.com>
> > > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Ben Gardon <bgardon@google.com>
> > When building mips:defconfig, this patch results in:
> > 
> > Error log:
> > In file included from include/linux/spinlock.h:90,
> >                   from include/linux/ipc.h:5,
> >                   from include/uapi/linux/sem.h:5,
> >                   from include/linux/sem.h:5,
> >                   from include/linux/compat.h:14,
> >                   from arch/mips/kernel/asm-offsets.c:12:
> > arch/mips/include/asm/spinlock.h:17:28: error: redefinition of 'queued_spin_unlock'
> >     17 | #define queued_spin_unlock queued_spin_unlock
> >        |                            ^~~~~~~~~~~~~~~~~~
> > arch/mips/include/asm/spinlock.h:22:20: note: in expansion of macro 'queued_spin_unlock'
> >     22 | static inline void queued_spin_unlock(struct qspinlock *lock)
> >        |                    ^~~~~~~~~~~~~~~~~~
> > In file included from include/asm-generic/qrwlock.h:17,
> >                   from ./arch/mips/include/generated/asm/qrwlock.h:1,
> >                   from arch/mips/include/asm/spinlock.h:13,
> >                   from include/linux/spinlock.h:90,
> >                   from include/linux/ipc.h:5,
> >                   from include/uapi/linux/sem.h:5,
> >                   from include/linux/sem.h:5,
> >                   from include/linux/compat.h:14,
> >                   from arch/mips/kernel/asm-offsets.c:12:
> > include/asm-generic/qspinlock.h:94:29: note: previous definition of 'queued_spin_unlock' was here
> >     94 | static __always_inline void queued_spin_unlock(struct qspinlock *lock)
> >        |                             ^~~~~~~~~~~~~~~~~~
> 
> I think the compile error is caused by the improper header file inclusion
> ordering. Can you try the following change to see if it can fix the compile
> error?
> 

That results in:

In file included from ./arch/mips/include/generated/asm/qrwlock.h:1,
                 from ./arch/mips/include/asm/spinlock.h:13,
                 from ./include/linux/spinlock.h:90,
                 from ./include/linux/ipc.h:5,
                 from ./include/uapi/linux/sem.h:5,
                 from ./include/linux/sem.h:5,
                 from ./include/linux/compat.h:14,
                 from arch/mips/kernel/asm-offsets.c:12:
./include/asm-generic/qrwlock.h: In function 'queued_rwlock_is_contended':
./include/asm-generic/qrwlock.h:127:9: error: implicit declaration of function 'arch_spin_is_locked'

Guenter

> Cheers,
> Longman
> 
> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> index 0020d3b820a7..d7178a9439b5 100644
> --- a/include/asm-generic/qrwlock.h
> +++ b/include/asm-generic/qrwlock.h
> @@ -10,11 +10,11 @@
>  #define __ASM_GENERIC_QRWLOCK_H
> 
>  #include <linux/atomic.h>
> +#include <linux/spinlock.h>
>  #include <asm/barrier.h>
>  #include <asm/processor.h>
> 
>  #include <asm-generic/qrwlock_types.h>
> -#include <asm-generic/qspinlock.h>
> 
>  /*
>   * Writer states & reader shift and bias.
> 
> 
>
Waiman Long Feb. 10, 2021, 12:27 a.m. UTC | #4 
On 2/9/21 5:25 PM, Guenter Roeck wrote:
> On Tue, Feb 09, 2021 at 04:46:02PM -0500, Waiman Long wrote:
>> On 2/9/21 3:39 PM, Guenter Roeck wrote:
>>> On Tue, Feb 02, 2021 at 10:57:12AM -0800, Ben Gardon wrote:
>>>> rwlocks do not currently have any facility to detect contention
>>>> like spinlocks do. In order to allow users of rwlocks to better manage
>>>> latency, add contention detection for queued rwlocks.
>>>>
>>>> CC: Ingo Molnar <mingo@redhat.com>
>>>> CC: Will Deacon <will@kernel.org>
>>>> Acked-by: Peter Zijlstra <peterz@infradead.org>
>>>> Acked-by: Davidlohr Bueso <dbueso@suse.de>
>>>> Acked-by: Waiman Long <longman@redhat.com>
>>>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Ben Gardon <bgardon@google.com>
>>> When building mips:defconfig, this patch results in:
>>>
>>> Error log:
>>> In file included from include/linux/spinlock.h:90,
>>>                    from include/linux/ipc.h:5,
>>>                    from include/uapi/linux/sem.h:5,
>>>                    from include/linux/sem.h:5,
>>>                    from include/linux/compat.h:14,
>>>                    from arch/mips/kernel/asm-offsets.c:12:
>>> arch/mips/include/asm/spinlock.h:17:28: error: redefinition of 'queued_spin_unlock'
>>>      17 | #define queued_spin_unlock queued_spin_unlock
>>>         |                            ^~~~~~~~~~~~~~~~~~
>>> arch/mips/include/asm/spinlock.h:22:20: note: in expansion of macro 'queued_spin_unlock'
>>>      22 | static inline void queued_spin_unlock(struct qspinlock *lock)
>>>         |                    ^~~~~~~~~~~~~~~~~~
>>> In file included from include/asm-generic/qrwlock.h:17,
>>>                    from ./arch/mips/include/generated/asm/qrwlock.h:1,
>>>                    from arch/mips/include/asm/spinlock.h:13,
>>>                    from include/linux/spinlock.h:90,
>>>                    from include/linux/ipc.h:5,
>>>                    from include/uapi/linux/sem.h:5,
>>>                    from include/linux/sem.h:5,
>>>                    from include/linux/compat.h:14,
>>>                    from arch/mips/kernel/asm-offsets.c:12:
>>> include/asm-generic/qspinlock.h:94:29: note: previous definition of 'queued_spin_unlock' was here
>>>      94 | static __always_inline void queued_spin_unlock(struct qspinlock *lock)
>>>         |                             ^~~~~~~~~~~~~~~~~~
>> I think the compile error is caused by the improper header file inclusion
>> ordering. Can you try the following change to see if it can fix the compile
>> error?
>>
> That results in:
>
> In file included from ./arch/mips/include/generated/asm/qrwlock.h:1,
>                   from ./arch/mips/include/asm/spinlock.h:13,
>                   from ./include/linux/spinlock.h:90,
>                   from ./include/linux/ipc.h:5,
>                   from ./include/uapi/linux/sem.h:5,
>                   from ./include/linux/sem.h:5,
>                   from ./include/linux/compat.h:14,
>                   from arch/mips/kernel/asm-offsets.c:12:
> ./include/asm-generic/qrwlock.h: In function 'queued_rwlock_is_contended':
> ./include/asm-generic/qrwlock.h:127:9: error: implicit declaration of function 'arch_spin_is_locked'
>
> Guenter

It is because in arch/mips/include/asm/spinlock.h, asm/qrwlock.h is 
included before asm/qspinlock.h. The compilation error should be gone if 
the asm/qrwlock.h is removed or moved after asm/qspinlock.h.

I did a x86 build and there was no compilation issue.

Cheers,
Longman
Waiman Long Feb. 10, 2021, 12:41 a.m. UTC | #5 
On 2/9/21 7:27 PM, Waiman Long wrote:
> On 2/9/21 5:25 PM, Guenter Roeck wrote:
>> On Tue, Feb 09, 2021 at 04:46:02PM -0500, Waiman Long wrote:
>>> On 2/9/21 3:39 PM, Guenter Roeck wrote:
>>>> On Tue, Feb 02, 2021 at 10:57:12AM -0800, Ben Gardon wrote:
>>>>> rwlocks do not currently have any facility to detect contention
>>>>> like spinlocks do. In order to allow users of rwlocks to better 
>>>>> manage
>>>>> latency, add contention detection for queued rwlocks.
>>>>>
>>>>> CC: Ingo Molnar <mingo@redhat.com>
>>>>> CC: Will Deacon <will@kernel.org>
>>>>> Acked-by: Peter Zijlstra <peterz@infradead.org>
>>>>> Acked-by: Davidlohr Bueso <dbueso@suse.de>
>>>>> Acked-by: Waiman Long <longman@redhat.com>
>>>>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Signed-off-by: Ben Gardon <bgardon@google.com>
>>>> When building mips:defconfig, this patch results in:
>>>>
>>>> Error log:
>>>> In file included from include/linux/spinlock.h:90,
>>>>                    from include/linux/ipc.h:5,
>>>>                    from include/uapi/linux/sem.h:5,
>>>>                    from include/linux/sem.h:5,
>>>>                    from include/linux/compat.h:14,
>>>>                    from arch/mips/kernel/asm-offsets.c:12:
>>>> arch/mips/include/asm/spinlock.h:17:28: error: redefinition of 
>>>> 'queued_spin_unlock'
>>>>      17 | #define queued_spin_unlock queued_spin_unlock
>>>>         |                            ^~~~~~~~~~~~~~~~~~
>>>> arch/mips/include/asm/spinlock.h:22:20: note: in expansion of macro 
>>>> 'queued_spin_unlock'
>>>>      22 | static inline void queued_spin_unlock(struct qspinlock 
>>>> *lock)
>>>>         |                    ^~~~~~~~~~~~~~~~~~
>>>> In file included from include/asm-generic/qrwlock.h:17,
>>>>                    from ./arch/mips/include/generated/asm/qrwlock.h:1,
>>>>                    from arch/mips/include/asm/spinlock.h:13,
>>>>                    from include/linux/spinlock.h:90,
>>>>                    from include/linux/ipc.h:5,
>>>>                    from include/uapi/linux/sem.h:5,
>>>>                    from include/linux/sem.h:5,
>>>>                    from include/linux/compat.h:14,
>>>>                    from arch/mips/kernel/asm-offsets.c:12:
>>>> include/asm-generic/qspinlock.h:94:29: note: previous definition of 
>>>> 'queued_spin_unlock' was here
>>>>      94 | static __always_inline void queued_spin_unlock(struct 
>>>> qspinlock *lock)
>>>>         |                             ^~~~~~~~~~~~~~~~~~
>>> I think the compile error is caused by the improper header file 
>>> inclusion
>>> ordering. Can you try the following change to see if it can fix the 
>>> compile
>>> error?
>>>
>> That results in:
>>
>> In file included from ./arch/mips/include/generated/asm/qrwlock.h:1,
>>                   from ./arch/mips/include/asm/spinlock.h:13,
>>                   from ./include/linux/spinlock.h:90,
>>                   from ./include/linux/ipc.h:5,
>>                   from ./include/uapi/linux/sem.h:5,
>>                   from ./include/linux/sem.h:5,
>>                   from ./include/linux/compat.h:14,
>>                   from arch/mips/kernel/asm-offsets.c:12:
>> ./include/asm-generic/qrwlock.h: In function 
>> 'queued_rwlock_is_contended':
>> ./include/asm-generic/qrwlock.h:127:9: error: implicit declaration of 
>> function 'arch_spin_is_locked'
>>
>> Guenter
>
> It is because in arch/mips/include/asm/spinlock.h, asm/qrwlock.h is 
> included before asm/qspinlock.h. The compilation error should be gone 
> if the asm/qrwlock.h is removed or moved after asm/qspinlock.h. 

After thinking a bit more, I think we should remove asm/qrwlock.h in 
arch/mips/include/asm/spinlock.h. qrwlock and qspinlocks are 
independent. An architecture can include one but not the other. Also 
there is no point in including qrwlock.h in a asm/spinlock.h.

Regards,
Longman
Waiman Long Feb. 10, 2021, 3:32 a.m. UTC | #6 
On 2/2/21 1:57 PM, Ben Gardon wrote:
> rwlocks do not currently have any facility to detect contention
> like spinlocks do. In order to allow users of rwlocks to better manage
> latency, add contention detection for queued rwlocks.
>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Will Deacon <will@kernel.org>
> Acked-by: Peter Zijlstra <peterz@infradead.org>
> Acked-by: Davidlohr Bueso <dbueso@suse.de>
> Acked-by: Waiman Long <longman@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>   include/asm-generic/qrwlock.h | 24 ++++++++++++++++++------
>   include/linux/rwlock.h        |  7 +++++++
>   2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
> index 84ce841ce735..0020d3b820a7 100644
> --- a/include/asm-generic/qrwlock.h
> +++ b/include/asm-generic/qrwlock.h
> @@ -14,6 +14,7 @@
>   #include <asm/processor.h>
>   
>   #include <asm-generic/qrwlock_types.h>
> +#include <asm-generic/qspinlock.h>

As said in another thread, qspinlock and qrwlock can be independently 
enabled for an architecture. So we shouldn't include qspinlock.h here. 
Instead, just include the regular linux/spinlock.h file to make sure 
that arch_spin_is_locked() is available.


>   
>   /*
>    * Writer states & reader shift and bias.
> @@ -116,15 +117,26 @@ static inline void queued_write_unlock(struct qrwlock *lock)
>   	smp_store_release(&lock->wlocked, 0);
>   }
>   
> +/**
> + * queued_rwlock_is_contended - check if the lock is contended
> + * @lock : Pointer to queue rwlock structure
> + * Return: 1 if lock contended, 0 otherwise
> + */
> +static inline int queued_rwlock_is_contended(struct qrwlock *lock)
> +{
> +	return arch_spin_is_locked(&lock->wait_lock);
> +}
> +
>   /*
>    * Remapping rwlock architecture specific functions to the corresponding
>    * queue rwlock functions.
>    */
> -#define arch_read_lock(l)	queued_read_lock(l)
> -#define arch_write_lock(l)	queued_write_lock(l)
> -#define arch_read_trylock(l)	queued_read_trylock(l)
> -#define arch_write_trylock(l)	queued_write_trylock(l)
> -#define arch_read_unlock(l)	queued_read_unlock(l)
> -#define arch_write_unlock(l)	queued_write_unlock(l)
> +#define arch_read_lock(l)		queued_read_lock(l)
> +#define arch_write_lock(l)		queued_write_lock(l)
> +#define arch_read_trylock(l)		queued_read_trylock(l)
> +#define arch_write_trylock(l)		queued_write_trylock(l)
> +#define arch_read_unlock(l)		queued_read_unlock(l)
> +#define arch_write_unlock(l)		queued_write_unlock(l)
> +#define arch_rwlock_is_contended(l)	queued_rwlock_is_contended(l)
>   
>   #endif /* __ASM_GENERIC_QRWLOCK_H */
> diff --git a/include/linux/rwlock.h b/include/linux/rwlock.h
> index 3dcd617e65ae..7ce9a51ae5c0 100644
> --- a/include/linux/rwlock.h
> +++ b/include/linux/rwlock.h
> @@ -128,4 +128,11 @@ do {								\
>   	1 : ({ local_irq_restore(flags); 0; }); \
>   })
>   
> +#ifdef arch_rwlock_is_contended
> +#define rwlock_is_contended(lock) \
> +	 arch_rwlock_is_contended(&(lock)->raw_lock)
> +#else
> +#define rwlock_is_contended(lock)	((void)(lock), 0)
> +#endif /* arch_rwlock_is_contended */
> +
>   #endif /* __LINUX_RWLOCK_H */

Cheers,
Longman
Guenter Roeck Feb. 10, 2021, 6:04 a.m. UTC | #7 
On 2/9/21 4:27 PM, Waiman Long wrote:
[ ... ]

> 
> It is because in arch/mips/include/asm/spinlock.h, asm/qrwlock.h is included before asm/qspinlock.h. The compilation error should be gone if the asm/qrwlock.h is removed or moved after asm/qspinlock.h.
> 
> I did a x86 build and there was no compilation issue.
> 
I can not really comment on what exactly is wrong - I don't know the code well
enough to do that - but I don't think this is a valid argument.

Anyway, it seems like mips is the only architecture affected by the problem.
I am not entirely sure, though - linux-next is too broken for that.

Thanks,
Guenter
Waiman Long Feb. 10, 2021, 2:57 p.m. UTC | #8 
On 2/10/21 1:04 AM, Guenter Roeck wrote:
> On 2/9/21 4:27 PM, Waiman Long wrote:
> [ ... ]
>
>> It is because in arch/mips/include/asm/spinlock.h, asm/qrwlock.h is included before asm/qspinlock.h. The compilation error should be gone if the asm/qrwlock.h is removed or moved after asm/qspinlock.h.
>>
>> I did a x86 build and there was no compilation issue.
>>
> I can not really comment on what exactly is wrong - I don't know the code well
> enough to do that - but I don't think this is a valid argument.
>
> Anyway, it seems like mips is the only architecture affected by the problem.
> I am not entirely sure, though - linux-next is too broken for that.

It does look like a rather common practice to include both qrwlock.h and 
qspinlock.h in asm/spinlock.h file. I have just a patch to make sure 
that qrwlock is always included after qspinlock.h if present. Hopefully 
that can fix the compilation problem.

Cheers,
Longman
Waiman Long Feb. 10, 2021, 3:15 p.m. UTC | #9 
On 2/9/21 10:32 PM, Waiman Long wrote:
> On 2/2/21 1:57 PM, Ben Gardon wrote:
>> rwlocks do not currently have any facility to detect contention
>> like spinlocks do. In order to allow users of rwlocks to better manage
>> latency, add contention detection for queued rwlocks.
>>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: Will Deacon <will@kernel.org>
>> Acked-by: Peter Zijlstra <peterz@infradead.org>
>> Acked-by: Davidlohr Bueso <dbueso@suse.de>
>> Acked-by: Waiman Long <longman@redhat.com>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Ben Gardon <bgardon@google.com>
>> ---
>>   include/asm-generic/qrwlock.h | 24 ++++++++++++++++++------
>>   include/linux/rwlock.h        |  7 +++++++
>>   2 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/asm-generic/qrwlock.h 
>> b/include/asm-generic/qrwlock.h
>> index 84ce841ce735..0020d3b820a7 100644
>> --- a/include/asm-generic/qrwlock.h
>> +++ b/include/asm-generic/qrwlock.h
>> @@ -14,6 +14,7 @@
>>   #include <asm/processor.h>
>>     #include <asm-generic/qrwlock_types.h>
>> +#include <asm-generic/qspinlock.h>
>
> As said in another thread, qspinlock and qrwlock can be independently 
> enabled for an architecture. So we shouldn't include qspinlock.h here. 
> Instead, just include the regular linux/spinlock.h file to make sure 
> that arch_spin_is_locked() is available.

The csky architecture uses qrwlock but not qspinlock. So this patch can 
be problematic when compiling for csky.

Cheers,
Longman
diff mbox series

Patch

diff --git a/include/asm-generic/qrwlock.h b/include/asm-generic/qrwlock.h
index 84ce841ce735..0020d3b820a7 100644
--- a/include/asm-generic/qrwlock.h
+++ b/include/asm-generic/qrwlock.h
@@ -14,6 +14,7 @@ 
 #include <asm/processor.h>
 
 #include <asm-generic/qrwlock_types.h>
+#include <asm-generic/qspinlock.h>
 
 /*
  * Writer states & reader shift and bias.
@@ -116,15 +117,26 @@  static inline void queued_write_unlock(struct qrwlock *lock)
 	smp_store_release(&lock->wlocked, 0);
 }
 
+/**
+ * queued_rwlock_is_contended - check if the lock is contended
+ * @lock : Pointer to queue rwlock structure
+ * Return: 1 if lock contended, 0 otherwise
+ */
+static inline int queued_rwlock_is_contended(struct qrwlock *lock)
+{
+	return arch_spin_is_locked(&lock->wait_lock);
+}
+
 /*
  * Remapping rwlock architecture specific functions to the corresponding
  * queue rwlock functions.
  */
-#define arch_read_lock(l)	queued_read_lock(l)
-#define arch_write_lock(l)	queued_write_lock(l)
-#define arch_read_trylock(l)	queued_read_trylock(l)
-#define arch_write_trylock(l)	queued_write_trylock(l)
-#define arch_read_unlock(l)	queued_read_unlock(l)
-#define arch_write_unlock(l)	queued_write_unlock(l)
+#define arch_read_lock(l)		queued_read_lock(l)
+#define arch_write_lock(l)		queued_write_lock(l)
+#define arch_read_trylock(l)		queued_read_trylock(l)
+#define arch_write_trylock(l)		queued_write_trylock(l)
+#define arch_read_unlock(l)		queued_read_unlock(l)
+#define arch_write_unlock(l)		queued_write_unlock(l)
+#define arch_rwlock_is_contended(l)	queued_rwlock_is_contended(l)
 
 #endif /* __ASM_GENERIC_QRWLOCK_H */
diff --git a/include/linux/rwlock.h b/include/linux/rwlock.h
index 3dcd617e65ae..7ce9a51ae5c0 100644
--- a/include/linux/rwlock.h
+++ b/include/linux/rwlock.h
@@ -128,4 +128,11 @@  do {								\
 	1 : ({ local_irq_restore(flags); 0; }); \
 })
 
+#ifdef arch_rwlock_is_contended
+#define rwlock_is_contended(lock) \
+	 arch_rwlock_is_contended(&(lock)->raw_lock)
+#else
+#define rwlock_is_contended(lock)	((void)(lock), 0)
+#endif /* arch_rwlock_is_contended */
+
 #endif /* __LINUX_RWLOCK_H */