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

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

On 2019/3/8 0:33, Andrey Ryabinin wrote:
> 
> 
> 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.

Thanks for the info.

> 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

OK, if that is the final decision, then I think Jiufei's patch that
moves kvfree() out of the locked region is the right thing to do for
this issue here.