On 09/22, Oleg Nesterov wrote:
>
> On 09/21, Peter Xu wrote:
> >
> > @@ -859,6 +989,25 @@ static int copy_pte_range(struct mm_struct *dst_mm,
> > struct mm_struct *src_mm,
> > spin_needbreak(src_ptl) || spin_needbreak(dst_ptl))
> > break;
> > }
> > +
> > + if (unlikely(data.cow_new_page)) {
> > + /*
> > + * If cow_new_page set, we must be at the 2nd round of
> > + * a previous COPY_MM_BREAK_COW. Try to arm the new
> > + * page now. Note that in all cases page_break_cow()
> > + * will properly release the objects in copy_mm_data.
> > + */
> > + WARN_ON_ONCE(copy_ret != COPY_MM_BREAK_COW);
> > + if (pte_install_copied_page(dst_mm, new, src_pte,
> > + dst_pte, addr, rss,
> > + &data)) {
> > + /* We installed the pte successfully; move on */
> > + progress++;
> > + continue;
>
> I'm afraid I misread this patch too ;)
>
> But it seems to me in this case the main loop can really "leak"
> COPY_MM_BREAK_COW. Suppose the the next 31 pte's are pte_none() and
> need_resched() is true.
>
> No?
If yes, perhaps we can simplify the copy_ret/cow_new_page logic and make
it more explicit?
Something like below, on top of this patch...
Oleg.
--- x/mm/memory.c
+++ x/mm/memory.c
@@ -704,17 +704,6 @@
};
};
-static inline void page_release_cow(struct copy_mm_data *data)
-{
- /* The old page should only be released in page_duplicate() */
- WARN_ON_ONCE(data->cow_old_page);
-
- if (data->cow_new_page) {
- put_page(data->cow_new_page);
- data->cow_new_page = NULL;
- }
-}
-
/*
* Duplicate the page for this PTE. Returns zero if page copied (so we need to
* retry on the same PTE again to arm the copied page very soon), or negative
@@ -925,7 +914,7 @@
if (!pte_same(*src_pte, data->cow_oldpte)) {
/* PTE has changed under us. Release the page and retry */
- page_release_cow(data);
+ put_page(data->cow_new_page);
return false;
}
@@ -936,12 +925,6 @@
set_pte_at(dst_mm, addr, dst_pte, entry);
rss[mm_counter(new_page)]++;
- /*
- * Manually clear the new page pointer since we've moved ownership to
- * the newly armed PTE.
- */
- data->cow_new_page = NULL;
-
return true;
}
@@ -958,16 +941,12 @@
struct copy_mm_data data;
again:
- /* We don't reset this for COPY_MM_BREAK_COW */
- memset(&data, 0, sizeof(data));
-
-again_break_cow:
init_rss_vec(rss);
dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
if (!dst_pte) {
- /* Guarantee that the new page is released if there is */
- page_release_cow(&data);
+ if (unlikely(copy_ret == COPY_MM_BREAK_COW))
+ put_page(data.cow_new_page);
return -ENOMEM;
}
src_pte = pte_offset_map(src_pmd, addr);
@@ -978,6 +957,22 @@
arch_enter_lazy_mmu_mode();
progress = 0;
+ if (unlikely(copy_ret == COPY_MM_BREAK_COW)) {
+ /*
+ * Note that in all cases pte_install_copied_page()
+ * will properly release the objects in copy_mm_data.
+ */
+ copy_ret = COPY_MM_DONE;
+ if (pte_install_copied_page(dst_mm, new, src_pte,
+ dst_pte, addr, rss,
+ &data)) {
+ /* We installed the pte successfully; move on */
+ progress++;
+ goto next;
+ }
+ /* PTE changed. Retry this pte (falls through) */
+ }
+
do {
/*
* We are holding two locks at this point - either of them
@@ -990,24 +985,6 @@
break;
}
- if (unlikely(data.cow_new_page)) {
- /*
- * If cow_new_page set, we must be at the 2nd round of
- * a previous COPY_MM_BREAK_COW. Try to arm the new
- * page now. Note that in all cases page_break_cow()
- * will properly release the objects in copy_mm_data.
- */
- WARN_ON_ONCE(copy_ret != COPY_MM_BREAK_COW);
- if (pte_install_copied_page(dst_mm, new, src_pte,
- dst_pte, addr, rss,
- &data)) {
- /* We installed the pte successfully; move on */
- progress++;
- continue;
- }
- /* PTE changed. Retry this pte (falls through) */
- }
-
if (pte_none(*src_pte)) {
progress++;
continue;
@@ -1017,6 +994,7 @@
if (copy_ret != COPY_MM_DONE)
break;
progress += 8;
+next:
} while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
arch_leave_lazy_mmu_mode();
@@ -1030,13 +1008,14 @@
case COPY_MM_SWAP_CONT:
if (add_swap_count_continuation(data.entry, GFP_KERNEL) < 0)
return -ENOMEM;
- break;
+ copy_ret = COPY_MM_DONE;
+ goto again;
case COPY_MM_BREAK_COW:
/* Do accounting onto parent mm directly */
ret = page_duplicate(src_mm, vma, addr, &data);
if (ret)
return ret;
- goto again_break_cow;
+ goto again;
case COPY_MM_DONE:
/* This means we're all good. */
break;
