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@xxxxxxxxxxxxxxxxxxx> 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;
