Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210203210832.113685-1-peterx@redhat.com> <20210203210832.113685-5-peterx@redhat.com>
In-Reply-To: <20210203210832.113685-5-peterx@redhat.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 3 Feb 2021 13:15:03 -0800
Message-ID: <CAHk-=whGvKx2aDhrW4K-PZix+Yt2L=dcv1meQWBiDFrW7ZJ63A@mail.gmail.com>
Subject: Re: [PATCH 4/4] hugetlb: Do early cow when page pinned on src mm
To:     Peter Xu <peterx@redhat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>, Wei Zhang <wzam@amazon.com>,
        Matthew Wilcox <willy@infradead.org>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Gal Pressman <galpress@amazon.com>,
        Christoph Hellwig <hch@lst.de>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Jan Kara <jack@suse.cz>,
        Kirill Shutemov <kirill@shutemov.name>,
        David Gibson <david@gibson.dropbear.id.au>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Kirill Tkhai <ktkhai@virtuozzo.com>,
        Jann Horn <jannh@google.com>,
        Andrew Morton <akpm@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 3, 2021 at 1:08 PM Peter Xu <peterx@redhat.com> wrote:
>
> This is the last missing piece of the COW-during-fork effort when there're
> pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
> pinned pages during fork() for ptes", 2020-09-27) for more information, since
> we do similar things here rather than pte this time, but just for hugetlb.

No issues with the code itself, but..

Comments are good, but the comments inside this block of code actually
makes the code *much* harder to read, because now the actual logic is
much more spread out and you can't see what it does so well.

> +                       if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
> +                               /* This is very possibly a pinned huge page */
> +                               if (!prealloc) {
> +                                       /*
> +                                        * Preallocate the huge page without
> +                                        * tons of locks since we could sleep.
> +                                        * Note: we can't use any reservation
> +                                        * because the page will be exclusively
> +                                        * owned by the child later.
> +                                        */
> +                                       put_page(ptepage);
> +                                       spin_unlock(src_ptl);
> +                                       spin_unlock(dst_ptl);
> +                                       prealloc = alloc_huge_page(vma, addr, 0);
> +                                       if (!prealloc) {
> +                                               /*
> +                                                * hugetlb_cow() seems to be
> +                                                * more careful here than us.
> +                                                * However for fork() we could
> +                                                * be strict not only because
> +                                                * no one should be referencing
> +                                                * the child mm yet, but also
> +                                                * if resources are rare we'd
> +                                                * better simply fail the
> +                                                * fork() even earlier.
> +                                                */
> +                                               ret = -ENOMEM;
> +                                               break;
> +                                       }
> +                                       goto again;
> +                               }
> +                               /*
> +                                * We have page preallocated so that we can do
> +                                * the copy right now.
> +                                */
> +                               hugetlb_copy_page(vma, dst_pte, addr, ptepage,
> +                                                 prealloc);
> +                               put_page(ptepage);
> +                               spin_unlock(src_ptl);
> +                               spin_unlock(dst_ptl);
> +                               prealloc = NULL;
> +                               continue;
> +                       }

Can you move the comment above the code? And I _think_ the prealloc
conditional could be split up to a helper function (which would help
more), but maybe there are too many variables for that to be
practical.

           Linus