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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.02.2020 14:45, Tamas K Lengyel wrote:
> 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.

Well, if this is a pre-existing pattern in the file, then - you
being the maintainer - so be it. I consider this bad practice though,
and I would suggest that every such site needs a comment (possibly
all but one simply referring to the one where things get actually
explained).

Jan

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