On Tue, Sep 22, 2020 at 02:40:14PM +0200, Oleg Nesterov wrote:
> 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?
I still think it's a no...
Note that now we'll reset "progress" every time before the do loop, so we'll
never reach need_resched() (since progress<32) before pte_install_copied_page()
when needed.
I explicitly put the pte_install_copied_page() into the loop just...
>
> 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;
> - }
> -}
(I'm not very sure on whether I should drop this helper. I wanted to have more
spots for checking everything is right and raise if something got wrong, and I
also wanted to have the cow_new_page to never contain invalid page pointer too
since after the put_page() it's invalid (otherwise we'll need to set NULL when
we do put_page every time explicitly). I'll still tend to keep this if no
strong opinion.. or I can also drop it if there's another vote.)
> -
> /*
> * 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;
... to avoid jumps like this because I think it's really tricky. :)
> + }
> + /* 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;
Kind of a continuation of the discussion from previous patch - I think we'd
better reset copy_ret not only for this case, but move it after the switch
(just in case there'll be new ones). The new BREAK_COW uses goto so it's quite
special.
> + goto again;
I feel like this could go wrong without the "addr != end" check later, when
this is the last pte to check.
Thanks,
> 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;
>
--
Peter Xu
