Re: [PATCH v2] mm: Warn on lock_page() from reclaim context.

[Michal Hocko <mhocko@suse.com>]

On Mon 19-03-18 15:08:24, Andrew Morton wrote:
> On Sun, 18 Mar 2018 10:22:49 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> > >From f43b8ca61b76f9a19c13f6bf42b27fad9554afc0 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > 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