Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 May 2023 15:06:27 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Peter Xu <peterx@redhat.com>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Mike Rapoport <rppt@kernel.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Matthew Wilcox <willy@infradead.org>,
        David Hildenbrand <david@redhat.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Qi Zheng <zhengqi.arch@bytedance.com>,
        Yang Shi <shy828301@gmail.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Peter Zijlstra <peterz@infradead.org>,
        Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>,
        Alistair Popple <apopple@nvidia.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Steven Price <steven.price@arm.com>,
        SeongJae Park <sj@kernel.org>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Zack Rusin <zackr@vmware.com>, Jason Gunthorpe <jgg@ziepe.ca>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Minchan Kim <minchan@kernel.org>,
        Christoph Hellwig <hch@infradead.org>,
        Song Liu <song@kernel.org>,
        Thomas Hellstrom <thomas.hellstrom@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 15/31] mm/userfaultfd: allow pte_offset_map_lock() to
 fail
In-Reply-To: <ZG6TOyH7NJIgXxGX@x1n>
Message-ID: <8f2131ac-8996-e4b3-2aad-7a4d11bd538f@google.com>
References: <68a97fbe-5c1e-7ac6-72c-7b9c6290b370@google.com> <49d92b15-3442-4e84-39bd-c77c316bf844@google.com> <ZG6TOyH7NJIgXxGX@x1n>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 24 May 2023, Peter Xu wrote:
> On Sun, May 21, 2023 at 10:07:35PM -0700, Hugh Dickins wrote:
> > mfill_atomic_install_pte() and mfill_atomic_pte_zeropage() treat
> > failed pte_offset_map_lock() as -EFAULT, with no attempt to retry.
> 
> Could you help explain why it should be -EFAULT, not -EAGAIN or -EEXIST?

Thanks a lot for looking, Peter.

No good justification for -EFAULT: I just grabbed the closest, fairly
neutral, error code that I could see already being in use there: but now
that you mention -EAGAIN, which I can see being used from mfill_atomic(),
yes, that would be ideal - and consistent with how it's already being used.

I'll make that change, thanks for suggesting.  (And it had bugged me how
my fs/userfaultfd.c was electing to retry, but this one electing to fail.)

> 
> IIUC right now if pte existed we have -EEXIST returned as part of the
> userfault ABI, no matter whether it's pte or thp.

It might or might not correspond to -EEXIST - it might even end up as
-EFAULT on a retry after -EAGAIN: I see mfill_atomic() contains both
-EEXIST and -EFAULT cases for pmd_trans_huge().  Actually, I could
say that the -EFAULT case there corresponds to the -EFAULT in this
15/31 patch, but that would be by coincidence not design: I'm happier
with your -EAGAIN suggestion.

> 
> IMHO it may boil down to my limited knowledge on how pte_offset_map_lock()
> is used after this part 2 series, and I assume the core changes will be in
> your 3rd series (besides this one and the arch one).
> 
> Please shed some light if there's quick answers (IIUC this is for speeding
> up collapsing shmem thps, but still no much clue here), or I can also wait
> for reading the 3rd part if it'll come soon in any form.

It wouldn't be particularly easy to deduce from the third series of
patches, rather submerged in implementation details.  Just keep in mind
that, like in the "old" pmd_trans_unstable() cases, there may be instants
at which, when trying to get the lock on a page table, that page table
might already have gone, or been replaced by something else e.g. a THP,
and a retry necessary at the outer level (if it's important to persist).

Hugh