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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

Janan

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