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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 29 January 2020 08:22
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxxxxx>;
> Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 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 <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.)
> 

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel