| Message ID | 20230218002819.1486479-2-jthoughton@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hugetlb: introduce HugeTLB high-granularity mapping | expand |
On Fri, Feb 17, 2023 at 4:28 PM James Houghton <jthoughton@google.com> wrote: > > If would be bad if we actually set PageUptodate with UFFDIO_CONTINUE; > PageUptodate indicates that the page has been zeroed, and we don't want > to give a non-zeroed page to the user. > > The reason this change is being made now is because UFFDIO_CONTINUEs on > subpages definitely shouldn't set this page flag on the head page. > > Signed-off-by: James Houghton <jthoughton@google.com> > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 07abcb6eb203..792cb2e67ce5 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -6256,7 +6256,16 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > * preceding stores to the page contents become visible before > * the set_pte_at() write. > */ > - __folio_mark_uptodate(folio); > + if (!is_continue) > + __folio_mark_uptodate(folio); > + else if (!folio_test_uptodate(folio)) { > + /* > + * This should never happen; HugeTLB pages are always Uptodate > + * as soon as they are allocated. > + */ if (is_continue) then we grab a page from the page cache, no? Are pages in page caches always uptodate? Why? I guess that means they're mapped hence uptodate? Also this comment should explain why pages in the page cache are always uptodate, no? Because this error branch is hit if (is_continue && !folio_test_uptodate()), not when pages are freshly allocated. > + ret = -EFAULT; > + goto out_release_nounlock; > + } > > /* Add shared, newly allocated pages to the page cache. */ > if (vm_shared && !is_continue) { > -- > 2.39.2.637.g21b0678d19-goog >
On Fri, Feb 17, 2023 at 4:42 PM Mina Almasry <almasrymina@google.com> wrote: > > On Fri, Feb 17, 2023 at 4:28 PM James Houghton <jthoughton@google.com> wrote: > > > > If would be bad if we actually set PageUptodate with UFFDIO_CONTINUE; > > PageUptodate indicates that the page has been zeroed, and we don't want > > to give a non-zeroed page to the user. > > > > The reason this change is being made now is because UFFDIO_CONTINUEs on > > subpages definitely shouldn't set this page flag on the head page. > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index 07abcb6eb203..792cb2e67ce5 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -6256,7 +6256,16 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > * preceding stores to the page contents become visible before > > * the set_pte_at() write. > > */ > > - __folio_mark_uptodate(folio); > > + if (!is_continue) > > + __folio_mark_uptodate(folio); > > + else if (!folio_test_uptodate(folio)) { > > + /* > > + * This should never happen; HugeTLB pages are always Uptodate > > + * as soon as they are allocated. > > + */ > > if (is_continue) then we grab a page from the page cache, no? Are > pages in page caches always uptodate? Why? I guess that means they're > mapped hence uptodate? > > Also this comment should explain why pages in the page cache are > always uptodate, no? Because this error branch is hit if (is_continue > && !folio_test_uptodate()), not when pages are freshly allocated. There was some discussion about it here[1]. Without even thinking about how the pages become uptodate, I think this patch is justified like this: UFFDIO_CONTINUE => we aren't actually changing the contents of the page, so we shouldn't be changing the uptodate-ness of the page. HugeTLB pages in the page cache are always uptodate: 1. fallocate -- the page is allocated, zeroed, marked as uptodate, and then placed in the page cache. 2. hugetlb_no_page -- same as above. So uptodate <=> "the page has been zeroed", so it would be very bad if we gave a !uptodate page to userspace via UFFDIO_CONTINUE. I'll update the comment to something like: "HugeTLB pages are always Uptodate as soon as they are added to the page cache. Given that we aren't changing the contents of the page, we shouldn't be updating the Uptodate-ness of the page." [1]: https://lore.kernel.org/linux-mm/Y5JrS4o5Detzid9V@monkey/ Thanks, Mina. :) - James
On 02/21/23 07:59, James Houghton wrote: > On Fri, Feb 17, 2023 at 4:42 PM Mina Almasry <almasrymina@google.com> wrote: > > > > On Fri, Feb 17, 2023 at 4:28 PM James Houghton <jthoughton@google.com> wrote: > > > > > > If would be bad if we actually set PageUptodate with UFFDIO_CONTINUE; > > > PageUptodate indicates that the page has been zeroed, and we don't want > > > to give a non-zeroed page to the user. > > > > > > The reason this change is being made now is because UFFDIO_CONTINUEs on > > > subpages definitely shouldn't set this page flag on the head page. > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > index 07abcb6eb203..792cb2e67ce5 100644 > > > --- a/mm/hugetlb.c > > > +++ b/mm/hugetlb.c > > > @@ -6256,7 +6256,16 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > > * preceding stores to the page contents become visible before > > > * the set_pte_at() write. > > > */ > > > - __folio_mark_uptodate(folio); > > > + if (!is_continue) > > > + __folio_mark_uptodate(folio); > > > + else if (!folio_test_uptodate(folio)) { > > > + /* > > > + * This should never happen; HugeTLB pages are always Uptodate > > > + * as soon as they are allocated. > > > + */ > > > > if (is_continue) then we grab a page from the page cache, no? Are > > pages in page caches always uptodate? Why? I guess that means they're > > mapped hence uptodate? > > > > Also this comment should explain why pages in the page cache are > > always uptodate, no? Because this error branch is hit if (is_continue > > && !folio_test_uptodate()), not when pages are freshly allocated. > > There was some discussion about it here[1]. > > Without even thinking about how the pages become uptodate, I think > this patch is justified like this: UFFDIO_CONTINUE => we aren't > actually changing the contents of the page, so we shouldn't be > changing the uptodate-ness of the page. Agree! > HugeTLB pages in the page cache are always uptodate: > 1. fallocate -- the page is allocated, zeroed, marked as uptodate, and > then placed in the page cache. > 2. hugetlb_no_page -- same as above. > > So uptodate <=> "the page has been zeroed", so it would be very bad if > we gave a !uptodate page to userspace via UFFDIO_CONTINUE. > > I'll update the comment to something like: > > "HugeTLB pages are always Uptodate as soon as they are added to the > page cache. Given that we aren't changing the contents of the page, we > shouldn't be updating the Uptodate-ness of the page." Perhaps a better way of saying is that hugetlb pages are marked uptodate shortly after allocation when their contents are initialized. Initialized data could be zero, or it could be contents copied from another location (such as in the UFFDIO_COPY case also handled in this routine). Saying "PageUptodate indicates that the page has been zeroed" as in the commit message is technically not correct. Ack to the patch.
On Tue, Feb 21, 2023 at 11:34 AM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 02/21/23 07:59, James Houghton wrote: > > On Fri, Feb 17, 2023 at 4:42 PM Mina Almasry <almasrymina@google.com> wrote: > > > > > > On Fri, Feb 17, 2023 at 4:28 PM James Houghton <jthoughton@google.com> wrote: > > > > > > > > If would be bad if we actually set PageUptodate with UFFDIO_CONTINUE; > > > > PageUptodate indicates that the page has been zeroed, and we don't want > > > > to give a non-zeroed page to the user. > > > > > > > > The reason this change is being made now is because UFFDIO_CONTINUEs on > > > > subpages definitely shouldn't set this page flag on the head page. > > > > > > > > Signed-off-by: James Houghton <jthoughton@google.com> > > > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > > index 07abcb6eb203..792cb2e67ce5 100644 > > > > --- a/mm/hugetlb.c > > > > +++ b/mm/hugetlb.c > > > > @@ -6256,7 +6256,16 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, > > > > * preceding stores to the page contents become visible before > > > > * the set_pte_at() write. > > > > */ > > > > - __folio_mark_uptodate(folio); > > > > + if (!is_continue) > > > > + __folio_mark_uptodate(folio); > > > > + else if (!folio_test_uptodate(folio)) { > > > > + /* > > > > + * This should never happen; HugeTLB pages are always Uptodate > > > > + * as soon as they are allocated. > > > > + */ > > > > > > if (is_continue) then we grab a page from the page cache, no? Are > > > pages in page caches always uptodate? Why? I guess that means they're > > > mapped hence uptodate? > > > > > > Also this comment should explain why pages in the page cache are > > > always uptodate, no? Because this error branch is hit if (is_continue > > > && !folio_test_uptodate()), not when pages are freshly allocated. > > > > There was some discussion about it here[1]. > > > > Without even thinking about how the pages become uptodate, I think > > this patch is justified like this: UFFDIO_CONTINUE => we aren't > > actually changing the contents of the page, so we shouldn't be > > changing the uptodate-ness of the page. > > Agree! > > > HugeTLB pages in the page cache are always uptodate: > > 1. fallocate -- the page is allocated, zeroed, marked as uptodate, and > > then placed in the page cache. > > 2. hugetlb_no_page -- same as above. > > > > So uptodate <=> "the page has been zeroed", so it would be very bad if > > we gave a !uptodate page to userspace via UFFDIO_CONTINUE. > > > > I'll update the comment to something like: > > > > "HugeTLB pages are always Uptodate as soon as they are added to the > > page cache. Given that we aren't changing the contents of the page, we > > shouldn't be updating the Uptodate-ness of the page." > > Perhaps a better way of saying is that hugetlb pages are marked uptodate > shortly after allocation when their contents are initialized. Initialized > data could be zero, or it could be contents copied from another location > (such as in the UFFDIO_COPY case also handled in this routine). I'll write something like this. Thank you! > > Saying "PageUptodate indicates that the page has been zeroed" as in the > commit message is technically not correct. And I'll make sure to update the commit description as well. > > Ack to the patch. Thanks, Mike!
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 07abcb6eb203..792cb2e67ce5 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -6256,7 +6256,16 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm, * preceding stores to the page contents become visible before * the set_pte_at() write. */ - __folio_mark_uptodate(folio); + if (!is_continue) + __folio_mark_uptodate(folio); + else if (!folio_test_uptodate(folio)) { + /* + * This should never happen; HugeTLB pages are always Uptodate + * as soon as they are allocated. + */ + ret = -EFAULT; + goto out_release_nounlock; + } /* Add shared, newly allocated pages to the page cache. */ if (vm_shared && !is_continue) {
If would be bad if we actually set PageUptodate with UFFDIO_CONTINUE; PageUptodate indicates that the page has been zeroed, and we don't want to give a non-zeroed page to the user. The reason this change is being made now is because UFFDIO_CONTINUEs on subpages definitely shouldn't set this page flag on the head page. Signed-off-by: James Houghton <jthoughton@google.com>