locking/qrwlock: Give priority to readers with disabled interrupts

Submitted by Kirill Tkhai on April 4, 2018, 3:22 p.m.

Details

Message ID 152285531399.3930.12169124684048820819.stgit@localhost.localdomain
State New
Series "locking/qrwlock: Give priority to readers with disabled interrupts"
Headers show

Commit Message

Kirill Tkhai April 4, 2018, 3:22 p.m.
The following situation leads to deadlock:

[task 1]                          [task 2]                         [task 3]
kill_fasync()                     mm_update_next_owner()           copy_process()
 spin_lock_irqsave(&fa->fa_lock)   read_lock(&tasklist_lock)        write_lock_irq(&tasklist_lock)
  send_sigio()                    <IRQ>                             ...
   read_lock(&fown->lock)         kill_fasync()                     ...
    read_lock(&tasklist_lock)      spin_lock_irqsave(&fa->fa_lock)  ...

Task 1 can't acquire read locked tasklist_lock, since there is
already task 3 expressed its wish to take the lock exclusive.
Task 2 holds the read locked lock, but it can't take the spin lock.

The patch makes queued_read_lock_slowpath() to give task 1 the same
priority as it was an interrupt handler, and to take the lock
dispite of task 3 is waiting it, and this prevents the deadlock.
It seems there is no better way to detect such the situations,
also in general it's not good to wait so long for readers with
interrupts disabled, since read_lock may nest with another locks
and delay the system.

This also should go to mainstream.

https://jira.sw.ru/browse/PSBM-83102

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 kernel/qrwlock.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/kernel/qrwlock.c b/kernel/qrwlock.c
index a79625ae5444..fb933f7a4631 100644
--- a/kernel/qrwlock.c
+++ b/kernel/qrwlock.c
@@ -70,7 +70,7 @@  void queued_read_lock_slowpath(struct qrwlock *lock, u32 cnts)
 	/*
 	 * Readers come here when they cannot get the lock without waiting
 	 */
-	if (unlikely(in_interrupt())) {
+	if (unlikely(irqs_disabled())) {
 		/*
 		 * Readers in interrupt context will get the lock immediately
 		 * if the writer is just waiting (not holding the lock yet).

Comments

Kirill Tkhai April 5, 2018, 1:11 p.m.
We will fix in another way. Firstly in ms.

On 04.04.2018 18:22, Kirill Tkhai wrote:
> The following situation leads to deadlock:
> 
> [task 1]                          [task 2]                         [task 3]
> kill_fasync()                     mm_update_next_owner()           copy_process()
>  spin_lock_irqsave(&fa->fa_lock)   read_lock(&tasklist_lock)        write_lock_irq(&tasklist_lock)
>   send_sigio()                    <IRQ>                             ...
>    read_lock(&fown->lock)         kill_fasync()                     ...
>     read_lock(&tasklist_lock)      spin_lock_irqsave(&fa->fa_lock)  ...
> 
> Task 1 can't acquire read locked tasklist_lock, since there is
> already task 3 expressed its wish to take the lock exclusive.
> Task 2 holds the read locked lock, but it can't take the spin lock.
> 
> The patch makes queued_read_lock_slowpath() to give task 1 the same
> priority as it was an interrupt handler, and to take the lock
> dispite of task 3 is waiting it, and this prevents the deadlock.
> It seems there is no better way to detect such the situations,
> also in general it's not good to wait so long for readers with
> interrupts disabled, since read_lock may nest with another locks
> and delay the system.
> 
> This also should go to mainstream.
> 
> https://jira.sw.ru/browse/PSBM-83102
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  kernel/qrwlock.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/qrwlock.c b/kernel/qrwlock.c
> index a79625ae5444..fb933f7a4631 100644
> --- a/kernel/qrwlock.c
> +++ b/kernel/qrwlock.c
> @@ -70,7 +70,7 @@ void queued_read_lock_slowpath(struct qrwlock *lock, u32 cnts)
>  	/*
>  	 * Readers come here when they cannot get the lock without waiting
>  	 */
> -	if (unlikely(in_interrupt())) {
> +	if (unlikely(irqs_disabled())) {
>  		/*
>  		 * Readers in interrupt context will get the lock immediately
>  		 * if the writer is just waiting (not holding the lock yet).
>