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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Jun 27, 2017 at 8:25 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 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.

That sounds right, since currently if a domain is created it is
guaranteed to have d->vm_event allocated. That is quite a large struct
so I think it would be a nice optimization to only allocate it when
needed. If we are reworking this code, I would prefer to see that
structure actually being removed altogether and just have three
pointers to vm_event_domain's (monitor/sharing/paging), which get
allocated when needed. The pointers for paging and sharing could then
further be wrapped in ifdef CONFIG checks, so we really only have
memory for what we need to.

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

Having this helper would be fine (even with my proposed changes
above). And yes, I can see this would involve touching a lot of code,
but I presume it will be mostly mechanical.

Tamas

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