From: Alistair Popple <apopple@nvidia.com>
To: Peter Xu <peterx@redhat.com>
Cc: <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
Axel Rasmussen <axelrasmussen@google.com>,
Nadav Amit <nadav.amit@gmail.com>,
Mike Rapoport <rppt@linux.vnet.ibm.com>,
Hugh Dickins <hughd@google.com>,
Mike Kravetz <mike.kravetz@oracle.com>,
"Kirill A . Shutemov" <kirill@shutemov.name>,
Jerome Glisse <jglisse@redhat.com>,
Matthew Wilcox <willy@infradead.org>,
Andrew Morton <akpm@linux-foundation.org>,
David Hildenbrand <david@redhat.com>,
Andrea Arcangeli <aarcange@redhat.com>
Subject: Re: [PATCH v6 01/23] mm: Introduce PTE_MARKER swap entry
Date: Tue, 7 Dec 2021 13:12:23 +1100 [thread overview]
Message-ID: <2190897.LADUpQAO2C@nvdebian> (raw)
In-Reply-To: <Yam9EezLTANRA+Rf@xz-m1.local>
On Friday, 3 December 2021 5:45:37 PM AEDT Peter Xu wrote:
> On Fri, Dec 03, 2021 at 04:35:38PM +1100, Alistair Popple wrote:
> > > > > +static inline pte_marker pte_marker_get(swp_entry_t entry)
> > > > > +{
> > > > > + return swp_offset(entry) & PTE_MARKER_MASK;
> > > >
> > > > I'm not sure the PTE_MARKER_MASK adds much, especially as we only have one
> > > > user. I don't see a problem with open-coding these kind of checks (ie.
> > >
> > > It's more or less a safety belt to make sure anything pte_marker_get() returned
> > > will be pte_marker defined bits only.
> > >
> > > > swp_offset(entry) & PTE_MARKER_UFFD_WP) as you kind of end up doing that anyway.
> > > > Alternatively if you want helper functions I think it would be better to define
> > > > them for each marker. Eg: is_pte_marker_uffd_wp().
> > >
> > > Yes we can have something like is_pte_marker_uffd_wp(), I didn't do that
> > > explicitly because I want us to be clear that pte_marker is a bitmask, so
> > > calling "is_*" will be slightly opaque - strictly speaking it should be
> > > "pte_marker_has_uffd_wp_bit()" if there will be more bits defined, but then the
> > > name of the helper will look a bit odd too. Hence I just keep the only
> > > interface to fetch the whole marker and use "&" in the call sites to check.
> >
> > Why does a caller need to care if it's a bitmask or not though? Isn't that an
> > implementation detail that could be left to the "is_*" functions? I must admit
> > I'm still working through the rest of this series though - is it because you
> > end up storing some kind of value in the upper bits of the PTE marker?
>
> Nop. I'm just afraid the caller could overlook the fact that it's a bitmask,
> then there can be code like:
>
> if (is_pte_marker_uffd_wp(*ptep) && drop_uffd_wp)
> pte_clear(ptep)
>
> While we should only do:
>
> if (is_pte_marker_uffd_wp(*ptep) && drop_uffd_wp)
> // remove uffd-wp bit in the pte_marker, keep the reset bitmask
I'm not sure how having the helper function prevents or changes this though? In
fact I just noticed this in patch 8:
if (uffd_wp_resolve &&
(pte_marker_get(entry) & PTE_MARKER_UFFD_WP)) {
pte_clear(vma->vm_mm, addr, pte);
pages++;
}
And if I'm understanding your point correctly isn't that wrong because if there
were other users of pte markers they would inadvertently get cleared? Unless of
course I've missed something - I haven't looked at patch 8 yet for context. To
help with the above situation I think you would need a helper for clearing
ptes.
> I could be worrying too much, there's no real user of it. If you prefer the
> helper a lot I can add it in the new version. Thanks,
It's not a massive issue, but I do think either defining a helper or open
coding the bit check is clearer. I think we can worry about other users if/when
they appear.
- Alistair
next prev parent reply other threads:[~2021-12-07 2:12 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-15 7:54 [PATCH v6 00/23] userfaultfd-wp: Support shmem and hugetlbfs Peter Xu
2021-11-15 7:55 ` [PATCH v6 01/23] mm: Introduce PTE_MARKER swap entry Peter Xu
2021-12-03 3:30 ` Alistair Popple
2021-12-03 4:21 ` Peter Xu
2021-12-03 5:35 ` Alistair Popple
2021-12-03 6:45 ` Peter Xu
2021-12-07 2:12 ` Alistair Popple [this message]
2021-12-07 2:30 ` Peter Xu
2021-11-15 7:55 ` [PATCH v6 02/23] mm: Teach core mm about pte markers Peter Xu
2021-11-15 7:55 ` [PATCH v6 03/23] mm: Check against orig_pte for finish_fault() Peter Xu
2021-12-16 5:01 ` Alistair Popple
2021-12-16 5:38 ` Peter Xu
2021-12-16 5:50 ` Peter Xu
2021-12-16 6:23 ` Alistair Popple
2021-12-16 7:06 ` Peter Xu
2021-12-16 7:45 ` Alistair Popple
2021-12-16 8:04 ` Peter Xu
2021-11-15 7:55 ` [PATCH v6 04/23] mm/uffd: PTE_MARKER_UFFD_WP Peter Xu
2021-12-16 5:18 ` Alistair Popple
2021-12-16 5:45 ` Peter Xu
2021-11-15 7:55 ` [PATCH v6 05/23] mm/shmem: Take care of UFFDIO_COPY_MODE_WP Peter Xu
2021-11-15 7:55 ` [PATCH v6 06/23] mm/shmem: Handle uffd-wp special pte in page fault handler Peter Xu
2021-12-16 5:56 ` Alistair Popple
2021-12-16 6:17 ` Peter Xu
2021-12-16 6:30 ` Alistair Popple
2021-11-15 7:55 ` [PATCH v6 07/23] mm/shmem: Persist uffd-wp bit across zapping for file-backed Peter Xu
2021-11-15 8:00 ` [PATCH v6 08/23] mm/shmem: Allow uffd wr-protect none pte for file-backed mem Peter Xu
2021-11-15 8:00 ` [PATCH v6 09/23] mm/shmem: Allows file-back mem to be uffd wr-protected on thps Peter Xu
2021-11-15 8:01 ` [PATCH v6 10/23] mm/shmem: Handle uffd-wp during fork() Peter Xu
2021-11-15 8:01 ` [PATCH v6 11/23] mm/hugetlb: Introduce huge pte version of uffd-wp helpers Peter Xu
2021-11-15 8:01 ` [PATCH v6 12/23] mm/hugetlb: Hook page faults for uffd write protection Peter Xu
2021-11-15 8:01 ` [PATCH v6 13/23] mm/hugetlb: Take care of UFFDIO_COPY_MODE_WP Peter Xu
2021-11-15 8:02 ` [PATCH v6 14/23] mm/hugetlb: Handle UFFDIO_WRITEPROTECT Peter Xu
2021-11-15 8:02 ` [PATCH v6 15/23] mm/hugetlb: Handle pte markers in page faults Peter Xu
2021-11-15 8:02 ` [PATCH v6 16/23] mm/hugetlb: Allow uffd wr-protect none ptes Peter Xu
2021-11-15 8:02 ` [PATCH v6 17/23] mm/hugetlb: Only drop uffd-wp special pte if required Peter Xu
2021-11-15 8:02 ` [PATCH v6 18/23] mm/hugetlb: Handle uffd-wp during fork() Peter Xu
2021-11-15 8:03 ` [PATCH v6 19/23] mm/khugepaged: Don't recycle vma pgtable if uffd-wp registered Peter Xu
2021-11-15 8:03 ` [PATCH v6 20/23] mm/pagemap: Recognize uffd-wp bit for shmem/hugetlbfs Peter Xu
2021-11-15 8:03 ` [PATCH v6 21/23] mm/uffd: Enable write protection for shmem & hugetlbfs Peter Xu
2021-11-15 8:03 ` [PATCH v6 22/23] mm: Enable PTE markers by default Peter Xu
2021-11-15 8:04 ` [PATCH v6 23/23] selftests/uffd: Enable uffd-wp for shmem/hugetlbfs Peter Xu
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=2190897.LADUpQAO2C@nvdebian \
--to=apopple@nvidia.com \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=axelrasmussen@google.com \
--cc=david@redhat.com \
--cc=hughd@google.com \
--cc=jglisse@redhat.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mike.kravetz@oracle.com \
--cc=nadav.amit@gmail.com \
--cc=peterx@redhat.com \
--cc=rppt@linux.vnet.ibm.com \
--cc=willy@infradead.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).