Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.01.2020 18:01, Durrant, Paul wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 28 January 2020 15:23
>>
>> On 24.01.2020 16:31, Paul Durrant wrote:
>>> Currently it is unsafe to assign a domheap page allocated with
>>> MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
>>> be incremented, but will be decrement when the page is freed (since
>>> free_domheap_pages() has no way of telling that the increment was
>> skipped).
>>>
>>> This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
>>> which is then used to mark domheap pages allocated with
>> MEMF_no_refcount.
>>> This then allows free_domheap_pages() to skip decrementing tot_pages
>> when
>>> appropriate and hence makes the pages safe to assign.
>>>
>>> NOTE: The patch sets MEMF_no_refcount directly in alloc_domheap_pages()
>>>       rather than in assign_pages() because the latter is called with
>>>       MEMF_no_refcount by memory_exchange() as an optimization, to avoid
>>>       too many calls to domain_adjust_tot_pages() (which acquires and
>>>       releases the global 'heap_lock').
>>
>> I don't think there were any optimization thoughts with this. The
>> MEMF_no_refcount use is because otherwise for a domain with
>> tot_pages == max_pages the assignment would fail.
>>
> 
> That would not be the case if the calls to steal_page() further up didn't
> pass MEMF_no_refcount (which would be the correct thing to do if not
> passing it to assign_pages().

No, that's not an option either: steal_page() would otherwise decrement
->tot_pages, allowing the domain to allocate new memory on another vCPU.
This would again result in the exchange failing for no reason. (And no,
I don't think a guest should be required to serialize e.g. ballooning
operations with exchanges.)

>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain
>> *d, long pages)
>>>  {
>>>      long dom_before, dom_after, dom_claimed, sys_before, sys_after;
>>>
>>> +    if ( !pages )
>>> +        goto out;
>>
>> Unrelated change? Are there, in fact, any callers passing in 0?
>> Oh, further down you add one which may do so, but then perhaps
>> better to make the caller not call here (as is done e.g. in
>> memory_exchange())?
> 
> I think it's preferable for domain_adjust_tot_pages() to handle zero
> gracefully.

That's an option, but imo would then better be a separate change (to
also drop present guards of calls to the function).

Jan

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