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

From: Jan Beulich <jbeulich@suse.com>
To: paul@xen.org
Cc: 'Stefano Stabellini' <sstabellini@kernel.org>,
	'Julien Grall' <julien@xen.org>, 'Wei Liu' <wl@xen.org>,
	'Konrad Rzeszutek Wilk' <konrad.wilk@oracle.com>,
	'Andrew Cooper' <andrew.cooper3@citrix.com>,
	'Paul Durrant' <pdurrant@amazon.com>,
	'Ian Jackson' <ian.jackson@eu.citrix.com>,
	'George Dunlap' <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	'Volodymyr Babchuk' <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH v5 6/6] domain: use PGC_extra domheap page for shared_info
Date: Thu, 12 Mar 2020 09:34:28 +0100	[thread overview]
Message-ID: <90afe940-5e76-f3af-eb73-af808bac0733@suse.com> (raw)
In-Reply-To: <004501d5f7b9$b00e1120$102a3360$@xen.org>

On 11.03.2020 16:28, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 11 March 2020 09:17
>> To: paul@xen.org
>> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Stefano Stabellini'
>> <sstabellini@kernel.org>; 'Julien Grall' <julien@xen.org>; 'Volodymyr Babchuk'
>> <Volodymyr_Babchuk@epam.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'George Dunlap'
>> <george.dunlap@citrix.com>; 'Ian Jackson' <ian.jackson@eu.citrix.com>; 'Konrad Rzeszutek Wilk'
>> <konrad.wilk@oracle.com>; 'Wei Liu' <wl@xen.org>
>> 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@suse.com>
>>>> Sent: 09 March 2020 15:56
>>>>
>>>> On 09.03.2020 11:23, paul@xen.org 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.

Well, now that these pages sit on a separate list, it would
look even less problematic than before to me to also give them
special treatment here: You wouldn't even have to take them
off the list anymore, but just call domain_adjust_tot_pages()
with -d->extra_pages (and suitably deal with the return value;
perhaps for consistency then followed by also zeroing
d->extra_pages, so overall accounting still looks correct).
Actually taking these pages off the list could (for dumping
purposes) then be done alongside their actual freeing. Such a
transition would apparently imply clearing PGC_extra alongside
the new domain_adjust_tot_pages() call.

I realize though that the end result won't be much different
from the current PGC_xen_heap handling (at least as far as
domain cleanup goes, but after all that's what I'm in fact
trying to convince you of), so the question would be whether
the whole transition then is worth it. Without having seen at
least a sketch of the LU code that is affected by the current
behavior, it remains hard for me to tell what issues might
remain, despite your and David's explanations.

> 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?

That's an option, too, but imo less desirable.

Jan

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