Re: [Xen-devel] [PATCH v5 6/6] domain: use PGC_extra domheap page for shared_info

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 11 March 2020 09:17
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Paul Durrant' <pdurrant@xxxxxxxxxx>; 
> 'Stefano Stabellini'
> <sstabellini@xxxxxxxxxx>; 'Julien Grall' <julien@xxxxxxx>; 'Volodymyr Babchuk'
> <Volodymyr_Babchuk@xxxxxxxx>; 'Andrew Cooper' <andrew.cooper3@xxxxxxxxxx>; 
> 'George Dunlap'
> <george.dunlap@xxxxxxxxxx>; 'Ian Jackson' <ian.jackson@xxxxxxxxxxxxx>; 
> 'Konrad Rzeszutek Wilk'
> <konrad.wilk@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>
> Subject: Re: [PATCH v5 6/6] domain: use PGC_extra domheap page for shared_info
> 
> On 10.03.2020 18:33, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 09 March 2020 15:56
> >>
> >> On 09.03.2020 11:23, paul@xxxxxxx wrote:
> >>> --- a/xen/common/time.c
> >>> +++ b/xen/common/time.c
> >>> @@ -99,6 +99,18 @@ void update_domain_wallclock_time(struct domain *d)
> >>>      uint32_t *wc_version;
> >>>      uint64_t sec;
> >>>
> >>> +    if ( d != current->domain )
> >>> +    {
> >>> +        /*
> >>> +         * We need to check is_dying here as, if it is set, the
> >>> +         * shared_info may have been freed. To do this safely we need
> >>> +         * hold the domain lock.
> >>> +         */
> >>> +        domain_lock(d);
> >>> +        if ( d->is_dying )
> >>> +            goto unlock;
> >>> +    }
> >>
> >> This shouldn't happen very often, but it's pretty heavy a lock.
> >> It's a fundamental aspect of xenheap pages that their disposal
> >> can b e delay until almost the last moment of guest cleanup. I
> >> continue to think it's not really a good ideal to have special
> >> purpose allocation (and mapping) accompanied by these pages
> >> getting taken care of by the generic relinquish-resources logic
> >> here (from a more general pov such is of course often nice to
> >> have). Instead of freeing these pages there, couldn't they just
> >> be taken off d->page_list, with the unmapping and freeing left
> >> as it was?
> >
> > I don't think this can be achieved without being able de-assign
> > pages and I don't really want to have to invent new logic to do
> > that (basically re-implementing what happens to xenheap pages).
> 
> Where's the connection to being able to de-assign pages here?
> There'll be one when the same conversion is to be done for
> gnttab code, but I don't see it here - the shared info page is
> never to be de-assigned. As to gnttab code, I think it was
> noted before that we may be better off not "unpopulating"
> status pages when switching back from v2 to v1. At which point
> the de-assignment need would go away there, too.

Ok, maybe I'm misunderstanding something then. We need to call 
free_domheap_pages() on all pages assigned to a domain so that the domain 
references get dropped. The xenpage ref is dropped when d->xenheap_pages == 0. 
The domheap ref is dropped when domain_adjust_tot_pages() returns zero. (This 
is what I meant by de-assigning... but that was probably a poor choice of 
words). So, because domain_adjust_tot_pages() returns d->tot_pages (which 
includes the extra_pages count) it won't fall to zero until the last put_page() 
on any PGC_extra page. So how is it possible to free shared_info in domain 
destroy? We'll never get that far, because the domheap ref will never get 
dropped. I guess this could be fixed by having domain_adjust_tot_pages() return 
the same values as domain_tot_pages() (i.e. tot_pages - extra_pages). Is that 
what you're suggesting?

> 
> > I really don't think it is that bad to deal with shared info
> > and grant table pages as domheap pages. Yes, we have to be
> > careful, but in this case the lock is only taken in a
> > toolstack update of the wallclock and, apart from start of
> > day access, I think this is limited to XEN_DOMCTL_settimeoffset
> > and XEN_DOMCTL_setvcpucontext neither of which I believe is
> > particularly performance-critical.
> 
> It's not, I agree (and hence I had started my previous reply
> also with "This shouldn't happen very often"). How all of this
> is going to look like with the new extra_page_list I'll have
> to see anyway. But for now I remain unconvinced of the want /
> need to de-allocate the shared info page early.
> 

Well hopefully I've explained above why that is currently necessary if it 
becomes a domheap page.

  Paul



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