Re: [Xen-devel] [PATCH v9 4/5] x86/mem_sharing: reset a fork

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 24, 2020 at 08:35:09AM -0700, Tamas K Lengyel wrote:
> On Mon, Feb 24, 2020 at 8:13 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Fri, Feb 21, 2020 at 10:49:22AM -0800, Tamas K Lengyel wrote:
> > > Implement hypercall that allows a fork to shed all memory that got 
> > > allocated
> > > for it during its execution and re-load its vCPU context from the parent 
> > > VM.
> > > This allows the forked VM to reset into the same state the parent VM is 
> > > in a
> > > faster way then creating a new fork would be. Measurements show about a 2x
> > > speedup during normal fuzzing operations. Performance may vary depending 
> > > how
> >                                                                           ^ 
> > on
> > > much memory got allocated for the forked VM. If it has been completely
> > > deduplicated from the parent VM then creating a new fork would likely be 
> > > more
> > > performant.
> > >
> > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > > ---
> > >  xen/arch/x86/mm/mem_sharing.c | 76 +++++++++++++++++++++++++++++++++++
> > >  xen/include/public/memory.h   |  1 +
> > >  2 files changed, 77 insertions(+)
> > >
> > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > > index ad5db9d8d5..fb6892aaa6 100644
> > > --- a/xen/arch/x86/mm/mem_sharing.c
> > > +++ b/xen/arch/x86/mm/mem_sharing.c
> > > @@ -1636,6 +1636,59 @@ static int mem_sharing_fork(struct domain *d, 
> > > struct domain *cd)
> > >      return rc;
> > >  }
> > >
> > > +/*
> > > + * The fork reset operation is intended to be used on short-lived forks 
> > > only.
> > > + * There is no hypercall continuation operation implemented for this 
> > > reason.
> > > + * For forks that obtain a larger memory footprint it is likely going to 
> > > be
> > > + * more performant to create a new fork instead of resetting an existing 
> > > one.
> > > + *
> > > + * TODO: In case this hypercall would become useful on forks with larger 
> > > memory
> > > + * footprints the hypercall continuation should be implemented.
> >
> > I'm afraid this is not safe, as users don't have an easy way to know
> > whether a fork will have a large memory footprint or not.
> 
> They do, getdomaininfo tells a user exactly how much memory has been
> allocated for a domain.
> 
> >
> > IMO you either need some kind of check that prevents this function
> > from being executed when the domain as too much memory assigned, or
> > you need to implement continuations.
> 
> I really don't think we need continuation here with the usecase we
> have for this function but I'm also tired of arguing about it, so I'll
> just add it even if its going to be dead code.
> 
> >
> > Or else this is very likely to trip over the watchdog.
> 
> The watchdog?

Yes, Xen has a watchdog and this loop is likely to trigger it if it
takes > 5s to complete. The watchdog triggering is a fatal event and
leads to host crash.

Note that watchdog is not enabled by default, you need to enable it on
the Xen command line.

> > > +    {
> > > +        p2m_type_t p2mt;
> > > +        p2m_access_t p2ma;
> > > +        gfn_t gfn;
> > > +        mfn_t mfn = page_to_mfn(page);
> > > +
> > > +        if ( !mfn_valid(mfn) )
> > > +            continue;
> > > +
> > > +        gfn = mfn_to_gfn(cd, mfn);
> > > +        mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &p2mt, &p2ma,
> > > +                                    0, NULL, false);
> > > +
> > > +        if ( !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) )
> > > +            continue;
> > > +
> > > +        /* take an extra reference */
> > > +        if ( !get_page(page, cd) )
> > > +            continue;
> > > +
> > > +        rc = p2m->set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_4K,
> > > +                            p2m_invalid, p2m_access_rwx, -1);
> > > +        ASSERT(!rc);
> >
> > Can you handle this gracefully?
> 
> Nope. This should never happen, so if it does, something is very wrong
> in some other part of Xen.

OK, please switch it to BUG_ON then instead of ASSERT. It's better to
crash here than to misbehave later.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel