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

[Andrey Ryabinin <aryabinin@virtuozzo.com>]

On 3/7/19 6:24 PM, Aaron Lu wrote:
> 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):
> 

It was discussed before. You're not the first one suggesting something like this.
There is the comment near in_atomic() explaining  well why and when your patch won't work.

The easiest way of making vfree() to be safe in atomic contexts is this patch:
http://lkml.kernel.org/r/20170330102719.13119-1-aryabinin@virtuozzo.com

But the final decision at that time was to fix caller so the call vfree from sleepable context instead:
 http://lkml.kernel.org/r/20170330152229.f2108e718114ed77acae7405@linux-foundation.org

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