Re: [PATCH] mm: fix sleeping function warning in alloc_swap_info

[Aaron Lu <aaron.lu@linux.alibaba.com>]

On Thu, Mar 07, 2019 at 05:47:13PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 3/7/19 5:43 PM, Aaron Lu wrote:
> > On Tue, Jan 29, 2019 at 05:01:50PM -0800, Andrew Morton wrote:
> >> On Wed, 30 Jan 2019 09:42:06 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>
> >>>>>
> >>>>> If we want to allow vfree() to sleep, at least we need to test with
> >>>>> kvmalloc() == vmalloc() (i.e. force kvmalloc()/kvfree() users to use
> >>>>> vmalloc()/vfree() path). For now, reverting the
> >>>>> "Context: Either preemptible task context or not-NMI interrupt." change
> >>>>> will be needed for stable kernels.
> >>>>
> >>>> So, the comment for vfree "May sleep if called *not* from interrupt
> >>>> context." is wrong?
> >>>
> >>> Commit bf22e37a641327e3 ("mm: add vfree_atomic()") says
> >>>
> >>>     We are going to use sleeping lock for freeing vmap.  However some
> >>>     vfree() users want to free memory from atomic (but not from interrupt)
> >>>     context.  For this we add vfree_atomic() - deferred variation of vfree()
> >>>     which can be used in any atomic context (except NMIs).
> >>>
> >>> and commit 52414d3302577bb6 ("kvfree(): fix misleading comment") made
> >>>
> >>>     - * Context: Any context except NMI.
> >>>     + * Context: Either preemptible task context or not-NMI interrupt.
> >>>
> >>> change. But I think that we converted kmalloc() to kvmalloc() without checking
> >>> context of kvfree() callers. Therefore, I think that kvfree() needs to use
> >>> vfree_atomic() rather than just saying "vfree() might sleep if called not in
> >>> interrupt context."...
> >>
> >> Whereabouts in the vfree() path can the kernel sleep?
> > 
> > (Sorry for the late reply.)
> > 
> > Adding Andrey Ryabinin, author of commit 52414d3302577bb6
> > ("kvfree(): fix misleading comment"), maybe Andrey remembers
> > where vfree() can sleep.
> > 
> > In the meantime, does "cond_resched_lock(&vmap_area_lock);" in
> > __purge_vmap_area_lazy() count as a sleep point?
> 
> Yes, this is the place (the only one) where vfree() can sleep.

OK, thanks for the quick confirm.

So what about this: use __vfree_deferred() when:
 - in_interrupt(), because we can't use mutex_trylock() as pointed out
   by Tetsuo;
 - in_atomic(), because cond_resched_lock();
 - irqs_disabled(), as smp_call_function_many() will deadlock.

An untested diff to show the idea(not sure if warn is needed):

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index e86ba6e74b50..28d200f054b0 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1578,7 +1578,7 @@ void vfree_atomic(const void *addr)
 
 static void __vfree(const void *addr)
 {
-	if (unlikely(in_interrupt()))
+	if (unlikely(in_interrupt() || in_atomic() || irqs_disabled()))
 		__vfree_deferred(addr);
 	else
 		__vunmap(addr, 1);
@@ -1606,8 +1606,6 @@ void vfree(const void *addr)
 
 	kmemleak_free(addr);
 
-	might_sleep_if(!in_interrupt());
-
 	if (!addr)
 		return;