Re: [Xen-devel] [PATCH] common/vm_event: Initialize vm_event lists on domain creation

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 06/27/2017 02:45 PM, Jan Beulich wrote:
>>>> Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 06/27/17 1:38 PM >>>
>> On 06/27/2017 02:26 PM, Jan Beulich wrote:
>>>>>> Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 06/27/17 10:32 AM >>>
>>>> On 06/27/2017 09:21 AM, Jan Beulich wrote:
>>>>>>>> Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 06/26/17 5:11 PM >>>
>>>>>> There is also a difference in timing; vm_event_init_domain() is called
>>>>>> when vm_event is started on the domain, not when the domain is
>>>>>> constructed.  IMO, the two should happen at the same time to be
>>>>>> consistent.  I'm not fussed at which point, as it would be fine (in
>>>>>> principle) for d->vm_event to be NULL in most cases.
>>>>>
>>>>> I very much support that last aspect - there's shouldn't be any memory
>>>>> allocated here if the domain isn't going to make use of VM events.
>>>>
>>>> Unfortunately it's not as simple as that.
>>>>
>>>> While I didn't write that code, it would seem that on domain creation
>>>> that data is being allocated because it holds information about 3
>>>> different vm_event subsystems - monitor, paging, and memshare.
>>>
>>> But it can't be that difficult to make it work the suggested way?
>>
>> We can lazy-allocate that the first time any one of the three gets
>> initialized (monitor, paging, share), and update all the code that
>> checks d->vm_event->member to check that d->vm_event is not NULL before
>> that.
>>
>> Any objections?
> 
> None here.

That's actually much more involved than I thought: almost every vm_event
function assumes that for a created domain d->vm_event is not NULL, and
takes a struct vm_event_domain *ved parameter to differentiate between
monitor, mem paging and mem sharing. So while this technically is not a
hard thing to fix at all, it would touch a lot of ARM and x86 code and
be quite an overhaul of vm_event.

My immediate reaction was to add small helper functions such as:

 42 static struct vm_event_domain *vm_event_get_ved(
 43     struct domain *d,
 44     enum vm_event_domain_type domain_type)
 45 {
 46     if ( !d->vm_event )
 47         return NULL;
 48
 49     switch ( domain_type )
 50     {
 51 #ifdef CONFIG_HAS_MEM_PAGING
 52     case VM_EVENT_DOMAIN_TYPE_MEM_PAGING:
 53         return &d->vm_event->paging;
 54 #endif
 55     case VM_EVENT_DOMAIN_TYPE_MONITOR:
 56         return &d->vm_event->monitor;
 57 #ifdef CONFIG_HAS_MEM_SHARING
 58     case VM_EVENT_DOMAIN_TYPE_MEM_SHARING:
 59         return &d->vm_event->share;
 60 #endif
 61     default:
 62         return NULL;
 63     }
 64 }

and try to get all places to use that line of thinking, but there's
quite a lot of them.

Tamas, any thoughts on this before going deeper?


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel