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: <alpine.LSU.2.11.2106011353270.2148@eggly.anvils>
 <alpine.LSU.2.11.2106011405510.2148@eggly.anvils> <CAHbLzkrUcNhGDmPstSNHhwbdoo3z2B=v-zb7__M3RqHL-Ct-EA@mail.gmail.com>
 <alpine.LSU.2.11.2106031923410.12760@eggly.anvils>
In-Reply-To: <alpine.LSU.2.11.2106031923410.12760@eggly.anvils>
From:   Yang Shi <shy828301@gmail.com>
Date:   Fri, 4 Jun 2021 11:24:38 -0700
Message-ID: <CAHbLzkoLnzyXra9MXmVVAkB5wdhSe-6j7TVunHXqHt6Bpzz7NA@mail.gmail.com>
Subject: Re: [PATCH 2/7] mm/thp: try_to_unmap() use TTU_SYNC for safe DEBUG_VM splitting
To:     Hugh Dickins <hughd@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Wang Yugui <wangyugui@e16-tech.com>,
        Matthew Wilcox <willy@infradead.org>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Alistair Popple <apopple@nvidia.com>,
        Ralph Campbell <rcampbell@nvidia.com>, Zi Yan <ziy@nvidia.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Minchan Kim <minchan@kernel.org>, Jue Wang <juew@google.com>,
        Peter Xu <peterx@redhat.com>, Jan Kara <jack@suse.cz>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 3, 2021 at 7:45 PM Hugh Dickins <hughd@google.com> wrote:
>
> On Thu, 3 Jun 2021, Yang Shi wrote:
> > On Tue, Jun 1, 2021 at 2:07 PM Hugh Dickins <hughd@google.com> wrote:
> > >
> > > Instead of abandoning the unsafe VM_BUG_ON_PAGE(), and the ones that
> > > follow, use PVMW_SYNC in try_to_unmap_one() in this case: adding TTU_SYNC
> > > to the options, and passing that from unmap_page() when CONFIG_DEBUG_VM=y.
> > > It could be passed in the non-debug case too, but that would sometimes add
> > > a little overhead, whereas it's rare for this race to result in failure.
> >
> > The above statement makes me feel this patch is just to relieve the
> > VM_BUG_ON, but my patch already changed it to VM_WARN, the race sounds
> > acceptable (at least not fatal) and the splitting code can handle the
> > failure case as well. So I'm wondering if we still need this patch or
> > not if it is just used to close the race when CONFIG_DEBUG_VM=y.
>
> I do agree that your 1/2 (what I'm calling 6.1/7) BUG->WARN patch
> is the most important of them; but it didn't get marked for stable,
> and has got placed behind conflicting mods never intended for stable.

Sorry for not marking it for stable. I do have no any objection to
having it in stable, just forgot to cc stable. :-(

>
> And a lot of the descriptions had been written in terms of the prior
> situation, with VM BUG there: it was easier to keep describing that way.

Understood.

>
> Whether your fix makes mine redundant is arguable (Wang Yugui thinks
> not).  It's easier to argue that it makes the racy ones (like this)
> redundant, than the persistent ones (like vma_address or pvm_walk).

The point is if we just close the race for DEBUG_VM case, it doesn't
sound worth it to me IMHO. How's about making it for !DEBUG_VM anyway?
How big the overhead could be? It looks like the heavy lifting (tlb
shootdown and page free) is done after pmd lock is released so it
should not block pvmw for a long time.

>
> Since I know of at least one customer who wants all these fixes in 5.10
> longterm, I'm fighting to get them that far at least.  But the further
> back they go, the less effort I'll make to backport them - will fall
> back to porting your BUG->WARN only.
>
> Hugh