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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Feb 24, 2020 at 8:42 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> 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.

OK, just to give you numbers, in the usecase this function is targeted
at we call it about ~100/s. Even in our worst case scenario we've seen
so far we haven't had a domain with enough memory deduplication where
this function took longer then 1s to finish. But again, at this point
we spent more time arguing about continuation then it takes to add it
so lets just move on.

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

In that case I don't think the current setup would really bother anyone.

>
> > > > +    {
> > > > +        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.

Sure

Thanks,
Tamas

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