> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 29 January 2020 08:22
> To: Durrant, Paul <pdurr...@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@eu.citrix.com>;
> Ian Jackson <ian.jack...@eu.citrix.com>; Julien Grall <jul...@xen.org>;
> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; Volodymyr Babchuk
> <volodymyr_babc...@epam.com>; Roger Pau Monné <roger....@citrix.com>
> Subject: Re: [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign
>
> On 28.01.2020 18:01, Durrant, Paul wrote:
> >> From: Jan Beulich <jbeul...@suse.com>
> >> 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.)
>
Ok, yes it does make it non-atomic but my view would be that the guest should
not be simultaneously ballooning; however, we clearly differ there.
> >>> --- 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).
Ok, I'll split it out into a separate patch.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
