From: David Hildenbrand <david@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Janosch Frank <frankja@linux.ibm.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
Heiko Carstens <hca@linux.ibm.com>,
Vasily Gorbik <gor@linux.ibm.com>,
Andrew Morton <akpm@linux-foundation.org>,
Alexander Gordeev <agordeev@linux.ibm.com>,
Sven Schnelle <svens@linux.ibm.com>,
Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
Andrea Arcangeli <aarcange@redhat.com>,
kvm@vger.kernel.org, linux-s390@vger.kernel.org
Subject: Re: [PATCH v1 1/2] mm/userfaultfd: don't place zeropages when zeropages are disallowed
Date: Thu, 21 Mar 2024 23:29:45 +0100 [thread overview]
Message-ID: <48d1282c-e4db-4b55-ab3f-3344af2440c4@redhat.com> (raw)
In-Reply-To: <ZfyyodKYWtGki7MO@x1n>
On 21.03.24 23:20, Peter Xu wrote:
> On Thu, Mar 21, 2024 at 10:59:53PM +0100, David Hildenbrand wrote:
>> s390x must disable shared zeropages for processes running VMs, because
>> the VMs could end up making use of "storage keys" or protected
>> virtualization, which are incompatible with shared zeropages.
>>
>> Yet, with userfaultfd it is possible to insert shared zeropages into
>> such processes. Let's fallback to simply allocating a fresh zeroed
>> anonymous folio and insert that instead.
>>
>> mm_forbids_zeropage() was introduced in commit 593befa6ab74 ("mm: introduce
>> mm_forbids_zeropage function"), briefly before userfaultfd went
>> upstream.
>>
>> Note that we don't want to fail the UFFDIO_ZEROPAGE request like we do
>> for hugetlb, it would be rather unexpected. Further, we also
>> cannot really indicated "not supported" to user space ahead of time: it
>> could be that the MM disallows zeropages after userfaultfd was already
>> registered.
>>
>> Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Still, a few comments below.
>
>> ---
>> mm/userfaultfd.c | 35 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
>> index 712160cd41eca..1d1061ccd1dea 100644
>> --- a/mm/userfaultfd.c
>> +++ b/mm/userfaultfd.c
>> @@ -316,6 +316,38 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd,
>> goto out;
>> }
>>
>> +static int mfill_atomic_pte_zeroed_folio(pmd_t *dst_pmd,
>> + struct vm_area_struct *dst_vma, unsigned long dst_addr)
>> +{
>> + struct folio *folio;
>> + int ret;
>
> nitpick: we can set -ENOMEM here, then
>
>> +
>> + folio = vma_alloc_zeroed_movable_folio(dst_vma, dst_addr);
>> + if (!folio)
>> + return -ENOMEM;
>
> return ret;
>
>> +
>> + ret = -ENOMEM;
>
> drop.
Sure!
>
>> + if (mem_cgroup_charge(folio, dst_vma->vm_mm, GFP_KERNEL))
>> + goto out_put;
>> +
>> + /*
>> + * The memory barrier inside __folio_mark_uptodate makes sure that
>> + * preceding stores to the page contents become visible before
>> + * the set_pte_at() write.
>> + */
>
> This comment doesn't apply. We can drop it.
>
I thought the same until I spotted that comment (where uffd originally
copied this from I strongly assume) in do_anonymous_page().
"Preceding stores" here are: zeroing out the memory.
Thanks for the fast review!
--
Cheers,
David / dhildenb
next prev parent reply other threads:[~2024-03-21 22:29 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-21 21:59 [PATCH v1 0/2] s390/mm: shared zeropage + KVM fix and optimization David Hildenbrand
2024-03-21 21:59 ` [PATCH v1 1/2] mm/userfaultfd: don't place zeropages when zeropages are disallowed David Hildenbrand
2024-03-21 22:20 ` Peter Xu
2024-03-21 22:29 ` David Hildenbrand [this message]
2024-03-21 22:46 ` Peter Xu
2024-03-22 8:13 ` David Hildenbrand
2024-03-21 21:59 ` [PATCH v1 2/2] s390/mm: re-enable the shared zeropage for !PV and !skeys KVM guests David Hildenbrand
2024-03-22 10:22 ` Christian Borntraeger
2024-03-22 17:08 ` David Hildenbrand
2024-03-21 22:13 ` [PATCH v1 0/2] s390/mm: shared zeropage + KVM fix and optimization Andrew Morton
2024-03-26 7:38 ` Heiko Carstens
2024-03-26 8:28 ` David Hildenbrand
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=48d1282c-e4db-4b55-ab3f-3344af2440c4@redhat.com \
--to=david@redhat.com \
--cc=aarcange@redhat.com \
--cc=agordeev@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=borntraeger@linux.ibm.com \
--cc=frankja@linux.ibm.com \
--cc=gerald.schaefer@linux.ibm.com \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-s390@vger.kernel.org \
--cc=peterx@redhat.com \
--cc=svens@linux.ibm.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.