From: Hugh Dickins <hughd@google.com>
To: Alistair Popple <apopple@nvidia.com>
Cc: Hugh Dickins <hughd@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Yang Shi <shy828301@gmail.com>,
Wang Yugui <wangyugui@e16-tech.com>,
Matthew Wilcox <willy@infradead.org>,
Naoya Horiguchi <naoya.horiguchi@nec.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>,
Shakeel Butt <shakeelb@google.com>,
Oscar Salvador <osalvador@suse.de>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/10] mm/thp: fix THP splitting unmap BUGs and related
Date: Fri, 11 Jun 2021 13:56:36 -0700 (PDT) [thread overview]
Message-ID: <494c929d-5253-58b0-6329-dec8395199cc@google.com> (raw)
In-Reply-To: <1697169.ADtCEf3Xva@nvdebian>
On Fri, 11 Jun 2021, Alistair Popple wrote:
> On Friday, 11 June 2021 10:15:51 AM AEST Hugh Dickins wrote:
> >
> > Sorry to give you the bother, Alistair: it's worked out as a bad moment
> > to rewrite swapops.h and rmap.c, I'm afraid.
>
> Indeed, but I don't think it's too bad. I've just tried rebasing it on this
> series and it didn't run into too many problems. Obviously I ran into the same
> issue Andrew did but I was able to fix that up. It also means try_to_migrate()
> now returns 'void' instead of 'bool'.
Yes, void try_to_migrate().
>
> Which brings me to the only real question I had during the rebase - does
> migration also need to accept the TTU_SYNC flag? I think it does because if I
> understand correctly we can still hit the same race with zap_pte_range() when
> trying to establish migration entries which previously also returned the status
> of page_mapped().
Yes, try_to_migrate() needs to accept TTU_SYNC too.
>
> > And the only help I've had time to give you was pointing Peter at your
> > series - many thanks to Peter, and to Shakeel.
>
> Yes, thanks for the help there. I think the main questions I had for you were
> around checking vma flags under the ptl in try_to_munlock_one but Shakeel was
> able to clear that up for me. Thanks!
>
> > Several times I've been on the point of asking you to keep the familiar
> > migration_entry_to_page(), along with your new pfn_swap_entry_to_page();
> > but each time I've looked, seen that it's hard to retain it sensibly at
> > the same time as overdue cleanup of the device_private_entry_to_page()s.
>
> Yeah, it would make things a bit funny to retain it IMHO. At least any fixups
> should just be simple substitutions.
>
> > So I guess I'm resigned to losing it; but there are at least three
> > bugs currently under discussion or fixes in flight, which border on
> > migration_entry_to_page() - Jann Horn's smaps syzbot bug, Xu Yu's
> > __migration_entry_wait() fix, my __split_huge_pmd_locked() fix
> > (and page_vma_mapped_walk() cleanup).
> >
> > And regarding huge_memory.c's unmap_page(): I did not recognize the
> > "helps handle cases when i_size" comment you added there. What I
> > ended up with (and thought was in mmotm-adjust.tar but seems not):
> >
> > /*
> > * Anon pages need migration entries to preserve them, but file
> > * pages can simply be left unmapped, then faulted back on demand.
> > * If that is ever changed (perhaps for mlock), update remap_page().
> > */
>
> My comment was based somewhat on the commit message for the original change but
> yours is much clearer so will incorporate it into my rebase, thanks.
Oh, you did better than I, I didn't think to look there on this occasion.
But even so, the i_size business is just one detail, and the new comment
better I think (I also disliked comment on an else without { } around it).
>
> As to sending my rebased series I suppose it would be best to wait until
> linux-mm has been updated with whatever other fixes are needed before resending
> it based on top of that. So far rebasing on this series didn't require too many
> drastic changes to my v10 series. The most significant was to incorporate your
> changes to unmap_page(). The remaining were just adding the TTU_SYNC case to
> try_to_migrate{_one} and a single s/migration_entry_to_page/pfn_swap_entry_to_page/
> in huge_memory.c
Yes, I think that's it. But check your try_to_migrate_one(), it may
want the same range.end vma_address_end() mod I made in try_to_unmap_one().
And does try_to_migrate_one() still have a comment referring to
try_to_unmap() when it should say try_to_migrate() there?
I've now located the diffs I missed from sending akpm before,
and diffed the diffs, and those are the points I see there;
but sending them now will just be a waste of everyones time.
No substitute for me checking your end result when it comes,
though I fear to do so since there's much more in your series
than I can wrap my head around without a lot more education.
Hugh
next prev parent reply other threads:[~2021-06-11 20:57 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <af88612-1473-2eaa-903-8d1a448b26@google.com>
2021-06-09 4:08 ` [PATCH v2 02/10] mm/thp: make is_huge_zero_pmd() safe and quicker Hugh Dickins
2021-06-09 10:22 ` Kirill A. Shutemov
2021-06-09 16:56 ` Yang Shi
2021-06-09 4:14 ` [PATCH v2 04/10] mm/thp: fix vma_address() if virtual address below file offset Hugh Dickins
2021-06-09 4:16 ` [PATCH v2 05/10] mm/thp: fix page_address_in_vma() on file THP tails Hugh Dickins
2021-06-09 4:19 ` [PATCH v2 06/10] mm/thp: unmap_mapping_page() to fix THP truncate_cleanup_page() Hugh Dickins
2021-06-09 17:02 ` Yang Shi
2021-06-09 21:11 ` Hugh Dickins
2021-06-09 21:16 ` [PATCH v3 " Hugh Dickins
2021-06-09 21:51 ` Yang Shi
2021-06-09 4:22 ` [PATCH v2 07/10] mm: thp: replace DEBUG_VM BUG with VM_WARN when unmap fails for split Hugh Dickins
2021-06-09 4:25 ` [PATCH v2 08/10] mm: rmap: make try_to_unmap() void function Hugh Dickins
2021-06-10 7:57 ` HORIGUCHI NAOYA(堀口 直也)
2021-06-09 4:27 ` [PATCH v2 09/10] mm/thp: remap_page() is only needed on anonymous THP Hugh Dickins
2021-06-09 4:30 ` [PATCH v2 10/10] mm: hwpoison_user_mappings() try_to_unmap() with TTU_SYNC Hugh Dickins
2021-06-09 10:27 ` Kirill A. Shutemov
2021-06-10 7:38 ` HORIGUCHI NAOYA(堀口 直也)
[not found] ` <20210610151505.d0124033e55bda07fa3d4408@linux-foundation.org>
[not found] ` <2014832.e7zRqyNrDn@nvdebian>
2021-06-11 0:15 ` [PATCH v2 00/10] mm/thp: fix THP splitting unmap BUGs and related Hugh Dickins
2021-06-11 7:28 ` Alistair Popple
2021-06-11 20:56 ` Hugh Dickins [this message]
2021-06-12 7:34 ` Alistair Popple
2021-06-12 8:20 ` Hugh Dickins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=494c929d-5253-58b0-6329-dec8395199cc@google.com \
--to=hughd@google.com \
--cc=akpm@linux-foundation.org \
--cc=apopple@nvidia.com \
--cc=jack@suse.cz \
--cc=juew@google.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linmiaohe@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=minchan@kernel.org \
--cc=naoya.horiguchi@nec.com \
--cc=osalvador@suse.de \
--cc=peterx@redhat.com \
--cc=rcampbell@nvidia.com \
--cc=shakeelb@google.com \
--cc=shy828301@gmail.com \
--cc=wangyugui@e16-tech.com \
--cc=willy@infradead.org \
--cc=ziy@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.