On Tue, 4 Jul 2017 09:48:11 +0200
Christian Borntraeger <borntrae...@de.ibm.com> wrote:
> On 07/03/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> > * Michael S. Tsirkin (m...@redhat.com) wrote:
> >> On Fri, Jun 30, 2017 at 05:31:39PM +0100, Dr. David Alan Gilbert wrote:
> >>> * Christian Borntraeger (borntrae...@de.ibm.com) wrote:
> >>>> On 04/26/2017 01:45 PM, Christian Borntraeger wrote:
> >>>>
> >>>>>> Hmm, I have a theory, if the flags field has bit 1 set, i.e.
> >>>>>> RAM_SAVE_FLAG_COMPRESS
> >>>>>> then try changing ram_handle_compressed to always do the memset.
> >>>>>
> >>>>> FWIW, changing ram_handle_compressed to always memset makes the problem
> >>>>> go away.
> >>>>
> >>>> It is still running fine now with the "always memset change"
> >>>
> >>> Did we ever nail down a fix for this; as I remember Andrea said
> >>> we shouldn't need to do that memset, but we came to the conclusion
> >>> it was something specific to how s390 protection keys worked.
> >>>
> >>> Dave
> >>
> >> No we didn't. Let's merge that for now, with a comment that
> >> we don't really understand what's going on?
> >
> > Hmm no, I don't really want to change the !s390 behaviour, especially
> > since it causes allocation that we otherwise avoid and Andrea's
> > reply tothe original post pointed out it's not necessary.
>
>
> Since storage keys are per physical page we must not have shared pages.
> Therefore in s390_enable_skey we already do a break_ksm (via ksm_madvise),
> in other words we already allocate pages on the keyless->keyed switch.
>
> So why not do the same for zero pages - instead of invalidating the page
> table entry, lets just do a proper COW.
>
> Something like this maybe (Andrea, Martin any better way to do that?)
>
>
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 4fb3d3c..11475c7 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2149,13 +2149,18 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
> static int __s390_enable_skey(pte_t *pte, unsigned long addr,
> unsigned long next, struct mm_walk *walk)
> {
> + struct page *page;
> /*
> - * Remove all zero page mappings,
> + * Remove all zero page mappings with a COW
> * after establishing a policy to forbid zero page mappings
> * following faults for that page will get fresh anonymous pages
> */
> - if (is_zero_pfn(pte_pfn(*pte)))
> - ptep_xchg_direct(walk->mm, addr, pte, __pte(_PAGE_INVALID));
> + if (is_zero_pfn(pte_pfn(*pte))) {
> + if (get_user_pages(addr, 1, FOLL_WRITE, &page, NULL) == 1)
> + put_page(page);
> + else
> + return -ENOMEM;
> + }
> /* Clear storage key */
> ptep_zap_key(walk->mm, addr, pte);
> return 0;
I do not quite get the problem here. The zero-page mappings are always
marked as _PAGE_SPECIAL. These should be safe to replace with an empty
pte. We do mark all VMAs as unmergeable prior to the page table walk
that replaces all zero-page mappings. What is the get_user_pages() call
supposed to do?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
