[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <52C2385A.8020608@oracle.com>
Date: Mon, 30 Dec 2013 22:22:02 -0500
From: Sasha Levin <sasha.levin@...cle.com>
To: "Kirill A. Shutemov" <kirill@...temov.name>
CC: akpm@...ux-foundation.org, linux-mm@...ck.org,
LKML <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC 1/2] mm: additional page lock debugging
On 12/30/2013 05:48 PM, Kirill A. Shutemov wrote:
> On Mon, Dec 30, 2013 at 11:33:47AM -0500, Sasha Levin wrote:
>> On 12/30/2013 06:43 AM, Kirill A. Shutemov wrote:
>>> On Sat, Dec 28, 2013 at 08:45:03PM -0500, Sasha Levin wrote:
>>>> We've recently stumbled on several issues with the page lock which
>>>> triggered BUG_ON()s.
>>>>
>>>> While working on them, it was clear that due to the complexity of
>>>> locking its pretty hard to figure out if something is supposed
>>>> to be locked or not, and if we encountered a race it was quite a
>>>> pain narrowing it down.
>>>>
>>>> This is an attempt at solving this situation. This patch adds simple
>>>> asserts to catch cases where someone is trying to lock the page lock
>>>> while it's already locked, and cases to catch someone unlocking the
>>>> lock without it being held.
>>>>
>>>> My initial patch attempted to use lockdep to get further coverege,
>>>> but that attempt uncovered the amount of issues triggered and made
>>>> it impossible to debug the lockdep integration without clearing out
>>>> a large portion of existing bugs.
>>>>
>>>> This patch adds a new option since it will horribly break any system
>>>> booting with it due to the amount of issues that it uncovers. This is
>>>> more of a "call for help" to other mm/ hackers to help clean it up.
>>>>
>>>> Signed-off-by: Sasha Levin <sasha.levin@...cle.com>
>>>> ---
>>>> include/linux/pagemap.h | 11 +++++++++++
>>>> lib/Kconfig.debug | 9 +++++++++
>>>> mm/filemap.c | 4 +++-
>>>> 3 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
>>>> index 1710d1b..da24939 100644
>>>> --- a/include/linux/pagemap.h
>>>> +++ b/include/linux/pagemap.h
>>>> @@ -321,6 +321,14 @@ static inline pgoff_t linear_page_index(struct vm_area_struct *vma,
>>>> return pgoff >> (PAGE_CACHE_SHIFT - PAGE_SHIFT);
>>>> }
>>>>
>>>> +#ifdef CONFIG_DEBUG_VM_PAGE_LOCKS
>>>> +#define VM_ASSERT_LOCKED(page) VM_BUG_ON_PAGE(!PageLocked(page), (page))
>>>> +#define VM_ASSERT_UNLOCKED(page) VM_BUG_ON_PAGE(PageLocked(page), (page))
>>>> +#else
>>>> +#define VM_ASSERT_LOCKED(page) do { } while (0)
>>>> +#define VM_ASSERT_UNLOCKED(page) do { } while (0)
>>>> +#endif
>>>> +
>>>> extern void __lock_page(struct page *page);
>>>> extern int __lock_page_killable(struct page *page);
>>>> extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
>>>> @@ -329,16 +337,19 @@ extern void unlock_page(struct page *page);
>>>>
>>>> static inline void __set_page_locked(struct page *page)
>>>> {
>>>> + VM_ASSERT_UNLOCKED(page);
>>>> __set_bit(PG_locked, &page->flags);
>>>> }
>>>>
>>>> static inline void __clear_page_locked(struct page *page)
>>>> {
>>>> + VM_ASSERT_LOCKED(page);
>>>> __clear_bit(PG_locked, &page->flags);
>>>> }
>>>>
>>>> static inline int trylock_page(struct page *page)
>>>> {
>>>> + VM_ASSERT_UNLOCKED(page);
>>>
>>> This is not correct. It's perfectly fine if the page is locked here: it's
>>> why trylock needed.
>>>
>>> IIUC, what we want to catch is the case when the page has already locked
>>> by the task.
>>
>> Frankly, we shouldn't have trylock_page() at all.
>
> It has valid use cases: if you don't want to sleep on lock, just give up
> right away. Like grab_cache_page_nowait().
Agreed. We should be checking if the same process is the one holding the lock in
general. I'll address that in the next version.
[snip]
>> Many places lock a long list of pages in bulk - we could allow that with
>> nesting, but then you lose your ability to detect trivial deadlocks.
>
> I see. But we need something better then plain VM_BUG() to be able to
> analyze what happened.
I really want to use lockdep here, but I'm not really sure how to handle locks which live
for a rather long while instead of being locked and unlocked in the same function like
most of the rest of the kernel. (Cc Ingo, PeterZ).
Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists