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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Feb 25, 2020 at 6:39 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 24.02.2020 16:35, 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:
> >>> --- 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.
>
> This tells the tool stack how much memory a guest has in absolute
> numbers, but it doesn't tell it whether Xen would consider this
> "large".
>
> >>> +    {
> >>> +        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.
>
>
> In such a case, please put in a comment explaining why failure is
> impossible. In the general case e.g. a 2Mb page may need splitting,
> which may yield -ENOMEM. Such a comment will then also be useful in
> case a new failure mode gets added to ->set_entry(), where it then
> will need judging whether the assumption here still holds. (This is
> also why in general it'd be better to handle the error. It'll still
> be better to crash the guest than the host in case you can't. See
> the bottom of ./CODING_STYLE.)

The mem_sharing codebase uses ASSERT(!rc) on p2m->set_entry already
when removing pages like we do here (see relinquish_shared_pages).
This only gets called on shared pages that we know for sure are
present. Since these are shared pages we know that their size is 4k
thus there is no splitting. I can certainly add a comment to this
effect to spell it out why the ASSERT is appropriate.

Tamas

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