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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan

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