From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1erNmx-00004r-T0 for qemu-devel@nongnu.org; Thu, 01 Mar 2018 07:58:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1erNmu-0005Cy-Qe for qemu-devel@nongnu.org; Thu, 01 Mar 2018 07:58:23 -0500 Date: Thu, 1 Mar 2018 12:58:12 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20180301125811.GF2994@work-vm> References: <20180228195320.165230-1-borntraeger@de.ibm.com> <79f7059b-f2d3-a758-6bb9-29433b31b313@redhat.com> <20180301092442.GA2994@work-vm> <20180301114543.GC2994@work-vm> <69654fb2-f5ba-c23b-f6f5-1b559692cf37@de.ibm.com> <20180301122854.GD2994@work-vm> <416dbf71-c311-1f40-baa6-414cdf220f4d@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/1] s390/kvm: implement clearing part of IPL clear List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger Cc: Thomas Huth , qemu-s390x , qemu-devel , Cornelia Huck , David Hildenbrand , Halil Pasic , Janosch Frank , Paolo Bonzini * Christian Borntraeger (borntraeger@de.ibm.com) wrote: > > > On 03/01/2018 01:35 PM, Christian Borntraeger wrote: > > > > > > On 03/01/2018 01:28 PM, Dr. David Alan Gilbert wrote: > >> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: > >>> > >>> > >>> On 03/01/2018 12:45 PM, Dr. David Alan Gilbert wrote: > >>>> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: > >>>>> > >>>>> > >>>>> On 03/01/2018 10:24 AM, Dr. David Alan Gilbert wrote: > >>>>>> * Thomas Huth (thuth@redhat.com) wrote: > >>>>>>> On 28.02.2018 20:53, Christian Borntraeger wrote: > >>>>>>>> When a guests reboots with diagnose 308 subcode 3 it requests the memory > >>>>>>>> to be cleared. We did not do it so far. This does not only violate the > >>>>>>>> architecture, it also misses the chance to free up that memory on > >>>>>>>> reboot, which would help on host memory over commitment. By using > >>>>>>>> ram_block_discard_range we can cover both cases. > >>>>>>> > >>>>>>> Sounds like a good idea. I wonder whether that release_all_ram() > >>>>>>> function should maybe rather reside in exec.c, so that other machines > >>>>>>> that want to clear all RAM at reset time can use it, too? > >>>>>>> > >>>>>>>> Signed-off-by: Christian Borntraeger > >>>>>>>> --- > >>>>>>>> target/s390x/kvm.c | 19 +++++++++++++++++++ > >>>>>>>> 1 file changed, 19 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >>>>>>>> index 8f3a422288..2e145ad5c3 100644 > >>>>>>>> --- a/target/s390x/kvm.c > >>>>>>>> +++ b/target/s390x/kvm.c > >>>>>>>> @@ -34,6 +34,8 @@ > >>>>>>>> #include "qapi/error.h" > >>>>>>>> #include "qemu/error-report.h" > >>>>>>>> #include "qemu/timer.h" > >>>>>>>> +#include "qemu/rcu_queue.h" > >>>>>>>> +#include "sysemu/cpus.h" > >>>>>>>> #include "sysemu/sysemu.h" > >>>>>>>> #include "sysemu/hw_accel.h" > >>>>>>>> #include "hw/boards.h" > >>>>>>>> @@ -41,6 +43,7 @@ > >>>>>>>> #include "sysemu/device_tree.h" > >>>>>>>> #include "exec/gdbstub.h" > >>>>>>>> #include "exec/address-spaces.h" > >>>>>>>> +#include "exec/ram_addr.h" > >>>>>>>> #include "trace.h" > >>>>>>>> #include "qapi-event.h" > >>>>>>>> #include "hw/s390x/s390-pci-inst.h" > >>>>>>>> @@ -1841,6 +1844,14 @@ static int kvm_arch_handle_debug_exit(S390CPU *cpu) > >>>>>>>> return ret; > >>>>>>>> } > >>>>>>>> > >>>>>>>> +static void release_all_rams(void) > >>>>>>> > >>>>>>> s/rams/ram/ maybe? > >>>>>>> > >>>>>>>> +{ > >>>>>>>> + struct RAMBlock *rb; > >>>>>>>> + > >>>>>>>> + QLIST_FOREACH_RCU(rb, &ram_list.blocks, next) > >>>>>>>> + ram_block_discard_range(rb, 0, rb->used_length); > >>>>>>> > >>>>>>> From a coding style point of view, I think there should be curly braces > >>>>>>> around ram_block_discard_range() ? > >>>>>> > >>>>>> I think this might break if it happens during a postcopy migrate. > >>>>>> The destination CPU is running, so it can do a reboot at just the wrong > >>>>>> time; and then the pages (that are protected by userfaultfd) would get > >>>>>> deallocated and trigger userfaultfd requests if accessed. > >>>>> > >>>>> Yes, userfaultd/postcopy is really fragile and relies on things that are not > >>>>> necessarily true (e.g. virito-balloon can also invalidate pages). > >>>> > >>>> That's why we use qemu_balloon_inhibit around postcopy to stop > >>>> ballooning; I'm not aware of anything else that does the same. > >>> > >>> we also have at least the pte_unused thing in mm/rmap.c that clearly > >>> predates userfaultfd. We might need to look into this as well.... > >> > >> I've not come across that; what does that do? > > > > It can drop a page on page out if the page is no longer of value. It is used by > > the CMMA (guest page hinting) code of s390x. > > > > see kernel mm/rmap.c > > > > > > static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > > unsigned long address, void *arg) > > { > > [...] > > } else if (pte_unused(pteval)) { > > /* > > * The guest indicated that the page content is of no > > * interest anymore. Simply discard the pte, vmscan > > * will take care of the rest. > > */ > > dec_mm_counter(mm, mm_counter(page)); > > /* We have to invalidate as we cleared the pte */ > > mmu_notifier_invalidate_range(mm, address, > > address + PAGE_SIZE); > > } else if (IS_ENABLED(CONFIG_MIGRATION) && > > (flags & (TTU_MIGRATION|TTU_SPLIT_FREEZE))) { > > [...] > > > > > > Maybe something like this in the kernel > > diff --git a/mm/rmap.c b/mm/rmap.c > index 47db27f8049e..9bdf4d448987 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1483,7 +1483,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > set_pte_at(mm, address, pvmw.pte, pteval); > } > > - } else if (pte_unused(pteval)) { > + } else if (pte_unused(pteval) && !vma->vm_userfaultfd_ctx.ctx) { > /* > * The guest indicated that the page content is of no > * interest anymore. Simply discard the pte, vmscan > > > could help? I guess so, but please check with aarcange; I don't know the mm code. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK