From: Kirill Tkhai <ktkhai@virtuozzo.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: mingo@redhat.com, stable@vger.kernel.org,
linux-kernel@vger.kernel.org, Waiman Long <longman@redhat.com>,
Boqun Feng <boqun.feng@gmail.com>
Subject: Re: [PATCH] locking/qrwlock: Give priority to readers with irqs disabled to prevent deadlock
Date: Thu, 5 Apr 2018 12:43:35 +0300 [thread overview]
Message-ID: <0092bc52-6536-4226-cdbf-8bdc97265c3f@virtuozzo.com> (raw)
In-Reply-To: <20180404161818.GJ4043@hirez.programming.kicks-ass.net>
On 04.04.2018 19:18, Peter Zijlstra wrote:
> On Wed, Apr 04, 2018 at 06:51:08PM +0300, Kirill Tkhai wrote:
>> On 04.04.2018 18:35, Peter Zijlstra wrote:
>>> On Wed, Apr 04, 2018 at 06:24:39PM +0300, 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
>>>
>>> That re-introduces starvation scenarios. And the above looks like a
>>> proper deadlock that should be sorted by fixing the locking order.
>>
>> We can move tasklist_lock out of send_sigio(), but I'm not sure
>> it's possible for read_lock(&fown->lock).
>
> So the scenario is:
>
> CPU0 CPU1 CPU2
>
> spin_lock_irqsave(&fa->fa_lock);
> read_lock(&tasklist_lock)
> write_lock_irq(&tasklist_lock)
> read_lock(&tasklist_lock)
> <IRQ>
> spin_lock_irqsave(&fa->fa_lock);
>
>
> Right? (where the row now signifies time)
>
> That doesn't seem to include fown->lock, you're saying it has an
> identical issue?
There is also read_lock(), which can be taken from interrupt:
CPU0 CPU1 CPU2
f_getown() kill_fasync()
read_lock(&f_own->lock) spin_lock_irqsave(&fa->fa_lock, flags)
<IRQ> send_sigio() write_lock_irq(&f_own->lock);
kill_fasync() read_lock(&fown->lock)
spin_lock_irqsave(&fa->fa_lock, flags)
To prevent deadlock, this requires all &f_own->lock be taken via read_lock_irqsave().
This may be formalized as lockdep rule: if spinlock nests into read_lock(), and they
both can be called from interrupt, the rest of read_lock() always must disable interrupts.
>> Is there another solution? Is there reliable way to iterate do_each_pid_task()
>> with rcu_read_lock()?
>
> Depends on what you call reliable :-), Yes you can use
> do_each_pid_task() with RCU, but as always you're prone to see tasks
> that are dead and miss tasks that just came in.
>
> If that is sufficient for the signal muck, dunno :/ Typically signals
> use sighand lock, not tasklist_lock.
The first thing is not a problem, while missing a newly moved task is not suitable.
So, it seems it's not reliable...
Kirill
next prev parent reply other threads:[~2018-04-05 9:43 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-04 15:24 [PATCH] locking/qrwlock: Give priority to readers with irqs disabled to prevent deadlock Kirill Tkhai
2018-04-04 15:35 ` Peter Zijlstra
2018-04-04 15:51 ` Kirill Tkhai
2018-04-04 15:55 ` Kirill Tkhai
2018-04-04 16:25 ` Waiman Long
2018-04-05 9:57 ` Kirill Tkhai
2018-04-04 16:18 ` Peter Zijlstra
2018-04-05 9:43 ` Kirill Tkhai [this message]
2018-04-04 15:43 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0092bc52-6536-4226-cdbf-8bdc97265c3f@virtuozzo.com \
--to=ktkhai@virtuozzo.com \
--cc=boqun.feng@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=longman@redhat.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.