Re: [PATCH 1/2] mm/mmu_notifier: init notifier if necessary

[Gavin Shan <shangw@linux.vnet.ibm.com>]

On Thu, Aug 30, 2012 at 12:13:02PM -0700, Andrew Morton wrote:
>On Sat, 25 Aug 2012 17:47:50 +0800
>Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
>
>> >> --- a/mm/mmu_notifier.c
>> >> +++ b/mm/mmu_notifier.c
>> >> @@ -192,22 +192,23 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
>> >>  
>> >>  	BUG_ON(atomic_read(&mm->mm_users) <= 0);
>> >>  
>> >> -	ret = -ENOMEM;
>> >> -	mmu_notifier_mm = kmalloc(sizeof(struct mmu_notifier_mm), GFP_KERNEL);
>> >> -	if (unlikely(!mmu_notifier_mm))
>> >> -		goto out;
>> >> -
>> >>  	if (take_mmap_sem)
>> >>  		down_write(&mm->mmap_sem);
>> >>  	ret = mm_take_all_locks(mm);
>> >>  	if (unlikely(ret))
>> >> -		goto out_cleanup;
>> >> +		goto out;
>> >>  
>> >>  	if (!mm_has_notifiers(mm)) {
>> >> +		mmu_notifier_mm = kmalloc(sizeof(struct mmu_notifier_mm),
>> >> +					GFP_ATOMIC);
>> >
>> >Why was the code switched to the far weaker GFP_ATOMIC?  We can still
>> >perform sleeping allocations inside mmap_sem.
>> >
>> 
>> Yes, we can perform sleeping while allocating memory, but we're holding
>> the "mmap_sem". GFP_KERNEL possiblly block somebody else who also waits
>> on mmap_sem for long time even though the case should be rare :-)
>
>GFP_ATOMIC allocations are unreliable.  If the allocation attempt fails
>here, an entire kernel subsystem will have failed, quite probably
>requiring a reboot.  It's a bad tradeoff.
>

Yep. Thanks, Andrew :-)

>Please fix this and retest.  With lockdep enabled, of course.
>
>And please do not attempt to sneak changes like this into the kernel
>without even mentioning them in the changelog.  If I hadn't have
>happened to notice this, we'd have ended up with a less reliable
>kernel.
>

I'll fix and rerest it.

Thanks,
Gavin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>