Re: [Xenomai-core] [Adeos-main] [COW-BUG] __alloc_pages called from atomic context

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 4836 bytes --]

Gilles Chanteperdrix wrote:
> On 8/7/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Gilles Chanteperdrix wrote:
>>> On 8/7/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> we are getting a lot of
>>>>>>
>>>>>> BUG: sleeping function called from invalid context at mm/page_alloc.c:1225
>>>>>> in_atomic():1, irqs_disabled():0
>>>>>>  [<c010305d>] show_trace_log_lvl+0x1a/0x2f
>>>>>>  [<c0103156>] show_trace+0x12/0x14
>>>>>>  [<c0103915>] dump_stack+0x16/0x18
>>>>>>  [<c010c4ab>] __might_sleep+0xcd/0xd3
>>>>>>  [<c0149488>] __alloc_pages+0x32/0x281
>>>>>>  [<c014fdd2>] copy_page_range+0x221/0x41e
>>>>>>  [<c010ec18>] copy_process+0x9e1/0xfe2
>>>>>>  [<c010f415>] do_fork+0x99/0x176
>>>>>>  [<c0100e75>] sys_clone+0x33/0x39
>>>>>>  [<c0102aaf>] syscall_call+0x7/0xb
>>>>>>  =======================
>>>>>>
>>>>>> here due to a Xenomai program issuing system() calls.
>>>>>>
>>>>>> After once again dissecting the "nice" mm code (sigh...), the reason
>>>>>> turned out to be plain simple:
>>>>>>
>>>>>> copy_pte_range(...);
>>>>>>   spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>>>>>>   copy_one_pte(...);
>>>>>>     if (is_cow_mapping(vm_flags))
>>>>>>       alloc_page_vma(GFP_HIGHUSER, ...);
>>>>>>         __alloc_pages(...)
>>>>>>        might_sleep_if(gfp_mask & __GFP_WAIT);
>>>>>>
>>>>>> And this is true due to #define GFP_HIGHUSER (__GFP_WAIT | ...
>>>>>>
>>>>>> So the bad news is that the COW code in likely all i-pipe versions is
>>>>>> broken. But the good new is that this might be easily fixable by
>>>>>> providing the right gfp_mask. GFP_ATOMIC?
>>>>> It does not look like a good solution, you are going to empty the
>>>>> GFP_ATOMIC pools. The proper solution would rather be to look at the
>>>>> real mm code (I mean not the one I wrote) and see how they cope with
>>>>> this issue.
>>>> Mmpf. What are the chances for a quick fix within the next days? We have
>>>> to consider alternatives right now here because the whole system is
>>>> meant for production purpose next week (C-ELROB '07).
>>>>
>>>> OK, I'm already finding myself inside the code :-/. What about this
>>>> approach: We try to alloc with GFP_ATOMIC. Once this fails, we break
>>>> out, drop all locks (just like it happens in case of need_resched()),
>>>> try to fill up the pool, and restart then. What would reliably make
>>>> Linux refill its atomic pool?
>>>>
>>>> Alternative approach: preallocate the required pages before entering the
>>>> loop in copy_pte_range. But that may require more code changes.
>>> I would say the real fix is to drop momentarily the spinlock(s?) for allocating.
>>>
>> Are you sure it's safe to drop locks in the (logical) middle of
>> copy_one_pte()? I can't tell yet from the few glances I took. It's just
>> my feeling that says "no" so far.
> 
> There is certainly something possible, since the vanilla kernel
> actually works without these warning.

Vanilla doesn't allocate pages from within copy_one_pte.


So, here comes proposal #1 for a solution, also addressing the broken
ENOMEM-error path of the current patch. Only compile tested yet. Might
this work?

Jan


---
 mm/memory.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Index: linux-2.6.20.15/mm/memory.c
===================================================================
--- linux-2.6.20.15.orig/mm/memory.c
+++ linux-2.6.20.15/mm/memory.c
@@ -498,7 +498,7 @@ copy_one_pte(struct mm_struct *dst_mm, s
 #ifdef CONFIG_IPIPE
 		if (((vm_flags|src_mm->def_flags) & (VM_LOCKED|VM_PINNED)) == (VM_LOCKED|VM_PINNED)) {
 			struct page *old_page = vm_normal_page(vma, addr, pte);
-			page = alloc_page_vma(GFP_HIGHUSER, vma, addr);
+			page = alloc_page_vma(GFP_ATOMIC, vma, addr);
 			if (!page)
 				return -ENOMEM;
 
@@ -546,6 +546,8 @@ static int copy_pte_range(struct mm_stru
 	spinlock_t *src_ptl, *dst_ptl;
 	int progress = 0;
 	int rss[2];
+	int nomem_retries = 10; /* Arbitrary limit */
+	int err = 0;
 
 again:
 	rss[1] = rss[0] = 0;
@@ -574,8 +576,12 @@ again:
 			continue;
 		}
 		if (copy_one_pte(dst_mm, src_mm, dst_pte,
-				 src_pte, vma, addr, rss))
-			return -ENOMEM;
+				 src_pte, vma, addr, rss)) {
+			if (--nomem_retries == 0)
+				err = -ENOMEM;
+			break;
+		}
+		nomem_retries = 10;
 		progress += 8;
 	} while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
 
@@ -585,9 +591,9 @@ again:
 	add_mm_rss(dst_mm, rss[0], rss[1]);
 	pte_unmap_unlock(dst_pte - 1, dst_ptl);
 	cond_resched();
-	if (addr != end)
+	if (addr != end && !err)
 		goto again;
-	return 0;
+	return err;
 }
 
 static inline int copy_pmd_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]