lkml.org 
[lkml]   [2020]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/5] mm: Do early cow for pinned pages during fork() for ptes
    On Tue, Sep 22, 2020 at 06:52:17PM +0200, Oleg Nesterov wrote:
    > On 09/22, Peter Xu wrote:
    > >
    > > 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.
    >
    > Yes. But copy_ret is still COPY_MM_BREAK_COW after pte_install_copied_page().
    > Now suppose that the next 31 pte's are pte_none(), progress will be incremented
    > every time.

    Yes, I think you're right - I'll need to reset that.

    >
    > > I explicitly put the pte_install_copied_page() into the loop just...
    > ...
    > > > 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. :)
    >
    > To me it looks better before the main loop because we know that
    > data.cow_new_page != NULL is only possible at the 1st iterattion after
    > restart ;)
    >
    > But I agree, this is subjective, please ignore.

    Thanks. For simplicity, I'll keep the code majorly as is. But I'm still open
    to change if e.g. someone else still perfers the other way.

    > However, I still think
    > it is better to rely on the copy_ret == COPY_MM_BREAK_COW check rather
    > than data.cow_new_page != NULL.

    Yes. Logically we should check both, but now as I'm written it as:

    if (unlikely(data.cow_new_page)) {
    WARN_ON_ONCE(copy_ret != COPY_MM_BREAK_COW);
    ...
    }

    I think it's even safer because it's actually checking both, but also warn if
    only cow_new_page is set, which should never happen anyways.

    Or I can also do it in inverted order if you think better:

    if (unlikely(copy_ret == COPY_MM_BREAK_COW)) {
    WARN_ON_ONCE(!data.cow_new_page);
    ...
    }

    >
    > > > 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.
    >
    > How? We know that copy_one_pte() failed and returned COPY_MM_SWAP_CONT
    > before addr = end.

    I think you're right, again. :)

    Thanks,

    --
    Peter Xu

    \
     
     \ /
      Last update: 2020-09-22 20:34    [W:3.180 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site