Re: [PATCH RFCv3 3/8] xen: introduce XEN_DOMCTL_set_recipient

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"Jan Beulich" <JBeulich@suse.com> writes:

Thanks for the review!

>>>> On 07.10.14 at 15:10, <vkuznets@redhat.com> wrote:
>> New operation sets the 'recipient' domain which will recieve all
>> memory pages from a particular domain when these pages are freed.
>
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -1152,6 +1152,33 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      }
>>      break;
>>  
>> +    case XEN_DOMCTL_set_recipient:
>> +    {
>> +        struct domain *recipient_dom;
>> +
>> +        if ( op->u.recipient.recipient == DOMID_INVALID )
>> +        {
>> +            if ( d->recipient )
>> +            {
>> +                put_domain(d->recipient);
>> +            }
>
> Please drop pointless braces like these and ...
>
>> +            d->recipient = NULL;
>> +            break;
>> +        }
>> +
>> +        recipient_dom = get_domain_by_id(op->u.recipient.recipient);
>> +        if ( recipient_dom == NULL )
>> +        {
>> +            ret = -ESRCH;
>> +            break;
>> +        }
>> +        else
>> +        {
>> +            d->recipient = recipient_dom;
>> +        }
>
> ... the pointless else-s/break-s like here.
>
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1707,6 +1707,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>  {
>>      struct domain *d = page_get_owner(pg);
>>      unsigned int i;
>> +    unsigned long mfn, gmfn;
>>      bool_t drop_dom_ref;
>>  
>>      ASSERT(!in_irq());
>> @@ -1764,11 +1765,28 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>              scrub = 1;
>>          }
>>  
>> -        if ( unlikely(scrub) )
>> -            for ( i = 0; i < (1 << order); i++ )
>> -                scrub_one_page(&pg[i]);
>> +        if ( !d || !d->recipient || d->recipient->is_dying )
>> +        {
>> +            if ( unlikely(scrub) )
>> +                for ( i = 0; i < (1 << order); i++ )
>> +                    scrub_one_page(&pg[i]);
>>  
>> -        free_heap_pages(pg, order);
>> +            free_heap_pages(pg, order);
>> +        }
>> +        else
>> +        {
>> +            mfn = page_to_mfn(pg);
>> +            gmfn = mfn_to_gmfn(d, mfn);
>> +
>> +            page_set_owner(pg, NULL);
>> +            assign_pages(d->recipient, pg, order, 0);
>
> This can fail.

.. if the recipient is dying or we're over-allocating. Shouldn't happen
(if toolstack does its job right) but I'll add a check.

>
>> +
>> +            if ( guest_physmap_add_page(d->recipient, gmfn, mfn, order) )
>> +            {
>> +                gdprintk(XENLOG_INFO, "Failed to add page to domain's physmap"
>> +                         " mfn: %lx\n", mfn);
>
> The current domain/vCPU don't matter here afaict, hence no need
> for gdprintk(). The source and/or recipient domain do matter though,
> hence printing either or both would seem desirable. The gMFN may
> be relevant for diagnostic purposes too. Hence e.g.
>
>                 printk(XENLOG_G_INFO "Failed to add MFN %lx (GFN %lx) to Dom%d's physmap\n"
>                        mfn, gmfn, d->recipient->domain_id);
>
> What's worse though is that you don't guard against
> XEN_DOMCTL_set_recipient zapping d->recipient. If that really is
> necessary to be supported, some synchronization will be needed.
>
> And finally - what's the intended protocol for the tool stack to
> know that all pages got transferred (and hence the new domain
> can be launched)?
>

(hope this answers both questions above)

Now the protocol is:
1) Toolstack queries for all page frames (with xc_get_pfn_type_batch())
for the old domain.
2) Toolstack issues XEN_DOMCTL_set_recipient with the recipient domain
3) Toolstack kills the source domain
4) Toolstack waits for awhile (loop keeps checking while we see new pages
arriving + some timeout).
5) Toolstack issues XEN_DOMCTL_set_recipient with DOMID_INVALID
resetting recipient.
6) Toolstack checks if all pages were transferred
7) If some pages are missing (e.g. source domain became zombie holding
something) request new empty pages instead.
8) Toolstack starts new domain

So we don't actually need XEN_DOMCTL_set_recipient to switch from one
recipient to the other, we just need a way to reset it.

>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -965,6 +965,23 @@ struct xen_domctl_vnuma {
>>  typedef struct xen_domctl_vnuma xen_domctl_vnuma_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vnuma_t);
>>  
>> +/*
>> + * XEN_DOMCTL_set_recipient - sets the 'recipient' domain which will recieve
>> + * all the domain's memory after its death or when and memory page from
>> + * domain's heap is being freed.
>
> So this specifically allows for this to happen on a non-dying domain.
> Why, i.e. what's the intended usage scenario? If not really needed,
> please remove this and verify in the handling that the source domain
> is dying.

Sorry if I didn't get this comment right. We need a way to tell which
domain will recieve memory and XEN_DOMCTL_set_recipient sets (and
resets) this target domain. We call it from toolstack before we start
killing old domain so it is not dying yet. We can't do it
hypervistor-side when our source domain exits doing SHUTDOWN_soft_reset
as there is no recipient domain created yet.

>From a general (hypervisor) point of view we don't actually care if the
domain is dying or not. We just want to recieve all freed pages from
this domain (so they go to some other domain instead of trash).

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -366,6 +366,7 @@ struct domain
>>      bool_t           is_privileged;
>>      /* Which guest this guest has privileges on */
>>      struct domain   *target;
>> +    struct domain   *recipient;
>
> Please add a brief comment similar to the one for "target".
>

Thanks again,

-- 
  Vitaly