Re: PoD code killing domain before it really gets started

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/08/12 14:29, Jan Beulich wrote:
>>>> On 07.08.12 at 15:13, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Aug 7, 2012 at 1:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 06.08.12 at 18:03, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>>>> 2. Allocate the PoD cache before populating the p2m table
>>> So this doesn't work, the call simply has no effect (and never
>>> reaches p2m_pod_set_cache_target()). Apparently because
>>> of
>>>
>>>      /* P == B: Nothing to do. */
>>>      if ( p2md->pod.entry_count == 0 )
>>>          goto out;
>>>
>>> in p2m_pod_set_mem_target(). Now I'm not sure about the
>>> proper adjustment here: Entirely dropping the conditional is
>>> certainly wrong. Would
>>>
>>>      if ( p2md->pod.entry_count == 0 && d->tot_pages > 0 )
>>>          goto out;
>>>
>>> be okay?
>>>
>>> But then later in that function we also have
>>>
>>>      /* B < T': Set the cache size equal to # of outstanding entries,
>>>       * let the balloon driver fill in the rest. */
>>>      if ( pod_target > p2md->pod.entry_count )
>>>          pod_target = p2md->pod.entry_count;
>>>
>>> which in the case at hand would set pod_target to 0, and the
>>> whole operation would again not have any effect afaict. So
>>> maybe this was the reason to do this operation _after_ the
>>> normal address space population?
>> Snap -- forgot about that.
>>
>> The main thing is for set_mem_target() to be simple for the toolstack
>> -- it's just supposed to say how much memory it wants the guest to
>> use, and Xen is supposed to figure out how much memory the PoD cache
>> needs.  The interface is that the toolstack is just supposed to call
>> set_mem_target() after each time it changes the balloon target.  The
>> idea was to be robust against the user setting arbitrary new targets
>> before the balloon driver had reached the old target.  So the problem
>> with allowing (pod_target > entry_count) is that that's the condition
>> that happens when you are ballooning up.
>>
>> Maybe the best thing to do is to introduce a specific call to
>> initialize the PoD cache that would ignore entry_count?
> Hmm, would looks more like a hack to me.
>
> How about doing the initial check as suggested earlier
>
>      if ( p2md->pod.entry_count == 0 && d->tot_pages > 0 )
>          goto out;
>
> and the latter check in a similar way
>
>      if ( pod_target > p2md->pod.entry_count && d->tot_pages > 0 )
>          pod_target = p2md->pod.entry_count;
>
> (which would still take care of any ballooning activity)? Or are
> there any other traps to fall into?
The "d->tot_pages > 0" seems more like a hack to me. :-) What's hackish 
about having an interface like this?
* allocate_pod_mem()
* for() { populate_pod_mem() }
* [Boot VM]
* set_pod_target()

Right now set_pod_mem() is used both for initial allocation and for 
adjustments.  But it seems like there's good reason to make a distinction.

  -George