[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork


  • To: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 24 Mar 2022 16:51:27 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=z3QB8PzOaTZX7b59ejLSk8DxnSxfLPNG/Ot33D3x6uE=; b=Kve7tZNj22aIC8B/Sn5sAn1o+y/YLvTpS/LSaOYkhb8T65VZ1JwPd6JAq2wpnoEe1vltZsIUpXSEKS89Icqut8LnGquqwybb9EDejEUBkwrSZWJGst8kqttYT4o4qqHLnT/Q6oG3RY3CfZU3YaaBouSfcxx0YLwb5BQwIRM+ZBUQ6R3Vwv8kmwgGJj296ZFk5RXwmje7noFHXcRAMEY7kuf1+y/0qfUx+cPG/h1PeutDZ1oDBHBLGvFFmINwKVdcIpEauUQDA5PUMbeTfR3k1D+UKtwpn/6JAzqSXGCahtK28MxYLS2dCZIA11NPLZAey7WTTu3UvUGTyWqG/XgVOw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MBELQIayx50GolIqjR6tfSmvk5qLSvMZrvvceRirIEVb4GBtErPss0FXVAwHgXBOtQ3uJVHfNqT+Lzz1d4NgCI+YpnhSlK0MpvcLVqz3qdMNjQb+Zb4zru1T4wz82gCwuwdG16iqBqWcYIMrxOE9kHh7L/VD/3mB/JF+DBJb/bKH9ulRWagOk+RM3b8uHLH/TG7+exAGiF1BCOo0jWx7mgzVra4Z/KPvMzmt+rRDKh4TSj/am/VFzJIToy3Ix4KNAS2Iq3cBCIIAHgIm+3rQTkqB6T8Qf/S5SM55CF77AOcgSpxQE/dbTSQ9bQR60rbbYtMLvafKCpg0IU3YxIJbgw==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Thu, 24 Mar 2022 15:51:45 +0000
  • Ironport-data: A9a23:OA0Al61JLJ8kFDfzpvbD5TFxkn2cJEfYwER7XKvMYLTBsI5bpzwDm zEYCDyPOqmIZ2rxL49wao238BsFvJ+GzNMxSwo9pC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkjk7xdOCn9xGQ7InQLlbGILes1htZGEk1EE/NtTo5w7Rj2tUy3YDja++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /1kh56hcAMVMpbQkd42QTgBLyogMIBvreqvzXiX6aR/zmXDenrohf5vEFs3LcsT/eMf7WNmr KJCbmpXN1ba2rzwkOnTpupE36zPKOHxO4wSoDd4xCzxBvc6W5HTBa7N4Le02R9u2JEfRqiDO aL1bxJBYS6bP0xmMW4tUsIRweT4rSGnWjRX/Qf9Sa0fvDGIkV0ZPKLWGMXRUsyHQ4NShEnwj mDb+2X0BDkKOdrZziCKmlq3nfPGly7/XIMUFZW7++RsjVnVwXYcYDU0f1ampfiyimalRslSb UcT/0IGsaE3/VeDUtr5Uhu3sXOA+BUbXrJ4D+Q/4RrLzqfS7BeUAkAFSCJMbJots8pebSwn0 BqFks3kARRrsaaJUjSN+7GMtzSwNCMJa2gYakc5oRAtuoe55ttp11SWE4glQPXdYsDJ9S/Y6 gKIvRE6u7kokccx/LeapGzM3T+Bj82cJuIq3Tn/UmWg5wJ/QYeqYY209FTWhcp9wJalokqp5 yZdxZXHhAwaJdTUzXHWHr1RdF28z6zdWAAwl2KDCHXIG96F33e4Nb5d7zhlTKuCGpZVIGS5C KM/VO442XOyAJdIRfIvC25SI55zpUQFKTgDfqqLBjapSsItHDJrBAk0OSatM5nFySDAa50XN 5aBatqLBn0HE6lhxzfeb75Dje9zmHhunj2LGMCTI/GbPVy2Pi79pVAtagbmUwzExPnc/FW9H yh3aaNmNCmzoMWhO3KKoOb/3HgBLGQhBICeliCkXrXrH+aSI0l4U6W56ep4I+RNxv0J/s+Vr iDVchIJkzLX2CyYQThmn1g+MdsDq74k9illVcHtVH71s0UejXGHsPhOLcdmLON7nAGhpNYtJ 8Q4lwy7Kq0nYhzM+igHbIm7q4pndR+xghmJMTbjaz86F6OMjSSTkjM4VmMDLBUzMxc=
  • Ironport-hdrordr: A9a23:dOI65KgFU2ZxC4hA+jbtPV0jqnBQXzh13DAbv31ZSRFFG/FwyP rAoB1L73PJYWgqNU3I+ergBEGBKUmskqKdxbNhR4tKOzOWxVdATbsSlrcKpgePJ8SQzJ8+6U 4NSdkaNDS0NykHsS+Y2njILz9D+qj/zEnAv463pB0MPGJXguNbnn9E426gYzNLrWJ9dPwE/f Snl656T23KQwVpUi33PAhMY8Hz4/nw0L72ax8PABAqrCGIkDOT8bb/VzyVxA0XXT9jyaortT GtqX2y2oyT99WAjjPM3W7a6Jpb3PPn19t4HcSJzuwYMC/lhAqEbJloH5eCoDc2iuey70tCqq iGnz4Qe+BIr1/BdGC8phXgnyHmzTYV8nfnjWSVhHPyyPaJMw4SOo5kv8Z0YxHZ400vsJVXy6 RQxV+UsJJREFfpgDn9z8KgbWAkqmOE5V4Z1cIDhX1WVoUTLJVLq5YEwU9TGJAcWArn9YEcFv V0Bs203ocbTbqjVQGZgoBT+q3tYpxqdS32AXTq+/blngS+pUoJgXfxn6ck7zU9HJFUcegx2w 2LCNUsqFh0dL5kUUtMPpZwfSKJMB2+ffvtChPlHb21LtBPB5ryw6SHlYndotvaPKA18A==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Mar 24, 2022 at 11:15:08AM -0400, Tamas K Lengyel wrote:
> On Thu, Mar 24, 2022 at 10:50 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote:
> > > Add option to the fork memop to skip populating the fork with special 
> > > pages.
> > > These special pages are only necessary when setting up forks to be fully
> > > functional with a toolstack. For short-lived forks where no toolstack is 
> > > active
> > > these pages are uneccesary.
> >
> > I'm not sure those are strictly related to having a toolstack. For
> > example the vcpu_info has nothing to do with having a toolstack, and
> > is only used by the guest in order to receive events or fetch time
> > related data. So while a short-lived fork might not make use of those,
> > that has nothing to do with having a toolstack or not.
> 
> Fair enough, the point is that the short live fork doesn't use these pages.
> 
> > >
> > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > > ---
> > >  xen/arch/x86/include/asm/hvm/domain.h |  4 +++-
> > >  xen/arch/x86/mm/mem_sharing.c         | 33 +++++++++++++++++----------
> > >  xen/include/public/memory.h           |  4 ++--
> > >  3 files changed, 26 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/include/asm/hvm/domain.h 
> > > b/xen/arch/x86/include/asm/hvm/domain.h
> > > index 698455444e..446cd06411 100644
> > > --- a/xen/arch/x86/include/asm/hvm/domain.h
> > > +++ b/xen/arch/x86/include/asm/hvm/domain.h
> > > @@ -31,7 +31,9 @@
> > >  #ifdef CONFIG_MEM_SHARING
> > >  struct mem_sharing_domain
> > >  {
> > > -    bool enabled, block_interrupts;
> > > +    bool enabled;
> > > +    bool block_interrupts;
> > > +    bool skip_special_pages;
> > >
> > >      /*
> > >       * When releasing shared gfn's in a preemptible manner, recall where
> > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > > index 15e6a7ed81..84c04ddfa3 100644
> > > --- a/xen/arch/x86/mm/mem_sharing.c
> > > +++ b/xen/arch/x86/mm/mem_sharing.c
> > > @@ -1643,7 +1643,8 @@ static int bring_up_vcpus(struct domain *cd, struct 
> > > domain *d)
> > >      return 0;
> > >  }
> > >
> > > -static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
> > > +static int copy_vcpu_settings(struct domain *cd, const struct domain *d,
> > > +                              bool skip_special_pages)
> > >  {
> > >      unsigned int i;
> > >      struct p2m_domain *p2m = p2m_get_hostp2m(cd);
> > > @@ -1660,7 +1661,7 @@ static int copy_vcpu_settings(struct domain *cd, 
> > > const struct domain *d)
> > >
> > >          /* Copy & map in the vcpu_info page if the guest uses one */
> > >          vcpu_info_mfn = d_vcpu->vcpu_info_mfn;
> > > -        if ( !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
> > > +        if ( !skip_special_pages && !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
> > >          {
> > >              mfn_t new_vcpu_info_mfn = cd_vcpu->vcpu_info_mfn;
> > >
> > > @@ -1807,17 +1808,18 @@ static int copy_special_pages(struct domain *cd, 
> > > struct domain *d)
> > >      return 0;
> > >  }
> > >
> > > -static int copy_settings(struct domain *cd, struct domain *d)
> > > +static int copy_settings(struct domain *cd, struct domain *d,
> > > +                         bool skip_special_pages)
> > >  {
> > >      int rc;
> > >
> > > -    if ( (rc = copy_vcpu_settings(cd, d)) )
> > > +    if ( (rc = copy_vcpu_settings(cd, d, skip_special_pages)) )
> > >          return rc;
> > >
> > >      if ( (rc = hvm_copy_context_and_params(cd, d)) )
> > >          return rc;
> > >
> > > -    if ( (rc = copy_special_pages(cd, d)) )
> > > +    if ( !skip_special_pages && (rc = copy_special_pages(cd, d)) )
> > >          return rc;
> > >
> > >      copy_tsc(cd, d);
> > > @@ -1826,9 +1828,11 @@ static int copy_settings(struct domain *cd, struct 
> > > domain *d)
> > >      return rc;
> > >  }
> > >
> > > -static int fork(struct domain *cd, struct domain *d)
> > > +static int fork(struct domain *cd, struct domain *d, uint16_t flags)
> > >  {
> > >      int rc = -EBUSY;
> > > +    bool block_interrupts = flags & XENMEM_FORK_BLOCK_INTERRUPTS;
> > > +    bool skip_special_pages = flags & XENMEM_FORK_SKIP_SPECIAL_PAGES;
> > >
> > >      if ( !cd->controller_pause_count )
> > >          return rc;
> > > @@ -1856,7 +1860,13 @@ static int fork(struct domain *cd, struct domain 
> > > *d)
> > >      if ( (rc = bring_up_vcpus(cd, d)) )
> > >          goto done;
> > >
> > > -    rc = copy_settings(cd, d);
> > > +    if ( !(rc = copy_settings(cd, d, skip_special_pages)) )
> >
> > Can you set
> > cd->arch.hvm.mem_sharing.{block_interrupts,skip_special_pages} earlier
> > so that you don't need to pass the options around to copy_settings and
> > copy_vcpu_settings?
> 
> Would be possible yes, but then we would have to clear them in case
> the forking failed at any point. Setting them only at the end when the
> fork finished ensures that those fields are only ever valid if the VM
> is a fork. Both are valid approaches and I prefer this over the other.

I think I'm confused, doesn't the fork get destroyed if there's a
failure? In which case the values
cd->arch.hvm.mem_sharing.{block_interrupts,skip_special_pages}
shouldn't really matter?

> >
> > > +    {
> > > +        cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
> > > +        cd->arch.hvm.mem_sharing.skip_special_pages = skip_special_pages;
> > > +        /* skip mapping the vAPIC page on unpause if skipping special 
> > > pages */
> > > +        cd->creation_finished = skip_special_pages;
> >
> > I think this is dangerous. While it might be true at the moment that
> > the arch_domain_creation_finished only maps the vAPIC page, there's no
> > guarantee it couldn't do other stuff in the future that could be
> > required for the VM to be started.
> 
> I understand this domain_creation_finished route could be expanded in
> the future to include other stuff, but IMHO we can evaluate what to do
> about that when and if it becomes necessary. This is all experimental
> to begin with.

Maybe you could check the skip_special_pages field from
domain_creation_finished in order to decide whether the vAPIC page
needs to be mapped?

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.