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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.10.14 at 15:10, <vkuznets@xxxxxxxxxx> 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 ( 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)?

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

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

Jan

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