On Sun, 18 Mar 2018 10:22:49 +0900 Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
> >From f43b8ca61b76f9a19c13f6bf42b27fad9554afc0 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Date: Sun, 18 Mar 2018 10:18:01 +0900
> Subject: [PATCH v2] mm: Warn on lock_page() from reclaim context.
>
> Kirill A. Shutemov noticed that calling lock_page[_killable]() from
> reclaim context might cause deadlock. In order to help finding such
> lock_page[_killable]() users (including out of tree users), this patch
> emits warning messages when CONFIG_PROVE_LOCKING is enabled.
>
> ...
>
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -466,6 +466,7 @@ static inline pgoff_t linear_page_index(struct vm_area_struct *vma,
> extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
> unsigned int flags);
> extern void unlock_page(struct page *page);
> +extern void __warn_lock_page_from_reclaim_context(void);
>
> static inline int trylock_page(struct page *page)
> {
> @@ -479,6 +480,9 @@ static inline int trylock_page(struct page *page)
> static inline void lock_page(struct page *page)
> {
> might_sleep();
> + if (IS_ENABLED(CONFIG_PROVE_LOCKING) &&
> + unlikely(current->flags & PF_MEMALLOC))
> + __warn_lock_page_from_reclaim_context();
> if (!trylock_page(page))
> __lock_page(page);
> }
I think it would be neater to do something like
#ifdef CONFIG_PROVE_LOCKING
static inline void lock_page_check_context(struct page *page)
{
if (unlikely(current->flags & PF_MEMALLOC))
__lock_page_check_context(page);
}
#else
static inline void lock_page_check_context(struct page *page)
{
}
#endif
and
void __lock_page_check_context(struct page *page)
{
WARN_ONCE(...);
dump_page(page);
}
And I wonder if overloading CONFIG_PROVE_LOCKING is appropriate here.
CONFIG_PROVE_LOCKING is a high-level thing under which a whole bunch of
different debugging options may exist. I guess we should add a new
config item under PROVE_LOCKING, or perhaps use CONFIG_DEBUG_VM.
Also, is PF_MEMALLOC the best way of determining that we're running
reclaim? What about using current->reclaim_state?
