From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB47BC4363D for ; Tue, 22 Sep 2020 15:58:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 84EF62084C for ; Tue, 22 Sep 2020 15:58:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="cYTOJjtv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 84EF62084C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B59B6900045; Tue, 22 Sep 2020 11:58:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AE29B90000A; Tue, 22 Sep 2020 11:58:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9820C900045; Tue, 22 Sep 2020 11:58:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0204.hostedemail.com [216.40.44.204]) by kanga.kvack.org (Postfix) with ESMTP id 7E9AF90000A for ; Tue, 22 Sep 2020 11:58:50 -0400 (EDT) Received: from smtpin17.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 3F97A1EF1 for ; Tue, 22 Sep 2020 15:58:50 +0000 (UTC) X-FDA: 77291155620.17.team41_6009eb82714f Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin17.hostedemail.com (Postfix) with ESMTP id 4198D180D0186 for ; Tue, 22 Sep 2020 15:58:49 +0000 (UTC) X-HE-Tag: team41_6009eb82714f X-Filterd-Recvd-Size: 10369 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [207.211.31.81]) by imf39.hostedemail.com (Postfix) with ESMTP for ; Tue, 22 Sep 2020 15:58:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1600790328; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=jZEZPjbCW+WDznyPzY3pnwtHOxYlin7U6kq/5+vfiZE=; b=cYTOJjtvjwY1fgV4xgzk5qrqvirJoMd9xgoFlGc2ju5f8uSeecJ25V72qGnA6x8j8kGRN8 UkkwSeKxQVPBG4Cjgu8sHp9OTIi0fkY+R+iZDAJa3TTzs8uwnxt7b8bG7iT7unh7/cW469 cfKp1ya2dQ5B/JIKYRcMmdg5SowsQ30= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-201-tuW7tUFXMkGTDXBpfUKFeg-1; Tue, 22 Sep 2020 11:58:46 -0400 X-MC-Unique: tuW7tUFXMkGTDXBpfUKFeg-1 Received: by mail-qk1-f197.google.com with SMTP id c19so14010055qkk.20 for ; Tue, 22 Sep 2020 08:58:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=jZEZPjbCW+WDznyPzY3pnwtHOxYlin7U6kq/5+vfiZE=; b=lJ7pmQugJDJtCUsl6Q8WVvip80uiGObrv5KPl7pC1YReDxcU9GAX1LddQ/tOJpS1ZP 2mQydx8saNeLrxbqxdzmAgRsETOlr1oOWFpVe080WDHMpFAaGj6Z7F4qWzI/RGvE93GJ nobrrmUZUwzI+fyi1qfTYeWbkANeqiR0ADp88K91oWKAjNXriM3xlOgm47gxP9vLfTJE cEuk6Hyb5+00X4PUzOYo4+JgJ3VCwlrYyEKYN2ctFc0lYcmvgpXNVMliIUIY2wcdC3Jr ItBWHOm4BHpRoQB7SLF2bsMeN0EkY6N4Unw0W98RzaRBkj40rJ5R1HedWzuavT2x9wnF Ftkw== X-Gm-Message-State: AOAM530VeUs7Perqcz6qhv1zF8/T9MhPyctB88btuBHnJMK4HHtyRnJJ yy/FRiW3IeveF8U+PE8oCkOvH+wuUd0b1y6rByMhvjjKuMTEQbBaysukn5I4ymTVXBWjq00rUE5 6fzF68eaHGEo= X-Received: by 2002:a37:c203:: with SMTP id i3mr5326166qkm.155.1600790325427; Tue, 22 Sep 2020 08:58:45 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy3NRg7sKvd2vdXkXVqig5l4rY6SRi9Xchw1HLlRRiR3IT2ZLli2VcqqeYnwmwJakErDEjHag== X-Received: by 2002:a37:c203:: with SMTP id i3mr5326127qkm.155.1600790325099; Tue, 22 Sep 2020 08:58:45 -0700 (PDT) Received: from xz-x1 (bras-vprn-toroon474qw-lp130-11-70-53-122-15.dsl.bell.ca. [70.53.122.15]) by smtp.gmail.com with ESMTPSA id g8sm11777550qkl.28.2020.09.22.08.58.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Sep 2020 08:58:43 -0700 (PDT) Date: Tue, 22 Sep 2020 11:58:42 -0400 From: Peter Xu To: Oleg Nesterov Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Linus Torvalds , Michal Hocko , Kirill Shutemov , Jann Horn , Kirill Tkhai , Hugh Dickins , Leon Romanovsky , Jan Kara , John Hubbard , Christoph Hellwig , Andrew Morton , Jason Gunthorpe , Andrea Arcangeli Subject: Re: [PATCH 4/5] mm: Do early cow for pinned pages during fork() for ptes Message-ID: <20200922155842.GG19098@xz-x1> References: <20200921211744.24758-1-peterx@redhat.com> <20200921212028.25184-1-peterx@redhat.com> <20200922114839.GC11679@redhat.com> <20200922124013.GD11679@redhat.com> MIME-Version: 1.0 In-Reply-To: <20200922124013.GD11679@redhat.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=peterx@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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