On Mon 19-03-18 15:08:24, Andrew Morton wrote:
> 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);
> }
I would just put __lock_page_check_context in place. Or do you expect
more callers? But agreed that this looks neater than in line code.
> 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.
Yes but it is meant to catch locking issues in general so I think doing
this check under the same config makes sense.
> I guess we should add a new config item under PROVE_LOCKING,
I am not convinced a new config is really worth it. We have way too many
already and PROVE_LOCKING sounds like a good fit to me.
> or perhaps use CONFIG_DEBUG_VM.
Please don't. There are people running with this config and adding more
potentially performance visible changes wouldn't make them too happy.
> Also, is PF_MEMALLOC the best way of determining that we're running
> reclaim? What about using current->reclaim_state?
Yeah, reclaim_state state would rule out PF_MEMALLOC (ab)users
allocating under the page lock.
--
Michal Hocko
SUSE Labs
